Replace alpha flag with a PaintRenderingContext2DSettings object#448
Conversation
| static get inputProperties() { return ['--foo']; } | ||
| static get inputArguments() { return ['<color>']; } | ||
| static get alpha() { return true; } | ||
| static get ContextAttributes() { return {alpha: true}; } |
There was a problem hiding this comment.
I think should be lowerCamelCase, "contextAttributes"
| 2. <a for=list>Append</a> |parsedSyntax| to |inputArgumentSyntaxes|. | ||
|
|
||
| 12. Let |alphaValue| be the result of <a>Get</a>(|paintCtor|, "alpha"). | ||
| 12. Let |alphaValue| be the result of <a>Get</a>(|paintCtor|, "ContextAttributes.alpha"). |
There was a problem hiding this comment.
This should be...
-
Let |contextAttributesValue| be the result of Get(|paintCtor|, "contextAttributes").
-
Let |contextAttributes| be the result of converting |contextAttributesValue| to a {{PaintContextAttributes}}, .... etc etc etc
| - <a for="paint definition">input properties</a> being |inputProperties|. | ||
|
|
||
| - <a for="paint definition">context alpha flag</a> being |alpha|. | ||
| - <a for="paint definition">ContextAttributes object</a> being {alpha: |alpha|}. |
There was a problem hiding this comment.
... being |contextAttributes|.
| |inputArgumentSyntaxes|. | ||
|
|
||
| - <a for="document paint definition">context alpha flag</a> being |alpha|. | ||
| - <a for="document paint definition">ContextAttributes object</a> being {alpha: |alpha|}. |
| with the paint canvas. Right now, the {{ContextAttributes}} has a boolean flag | ||
| indicating if the paint canvas contains an alpha channel. | ||
| <pre class='idl'> | ||
| dictionary ContextAttributes { |
There was a problem hiding this comment.
might be nice to namespace this for the spec e.g. {{PaintContextAttributes}}, but doesn't really matter as not user visible.
There was a problem hiding this comment.
I am not very certain about this. I notice that there is a proposal for CanvasRenderingContext2DSettings in here: https://github.com/WICG/canvas-color-space/blob/master/CanvasColorSpaceProposal.md
Currently there is only one alpha, but the proposal make it including color space attributes. Maybe in the future we should make ContextAttributes inherit from it? Or maybe just directly using it for PaintRenderingContext2D?
There was a problem hiding this comment.
It makes sense that we might eventually want to support settings like color space. However, in the event that we do this I think we might still only want to support a subset of CanvasRenderingContext2DSettings just like PaintRenderingContext2D implements a subset of CanvasRenderingContext2D. How about PaintRenderingContext2DSettings for consistency?
There was a problem hiding this comment.
Yes, I'd support that. PaintRenderingContext2DSettings is a better name, because it is more clear that it is used for PaintRenderingContext2D.
Question: do we need to start a discussion somewhere and bring other people onboard, like CSS discussion group?
|
@bfgeek I have addressed almost all comments in the new commit. Please review. |
| 2. <a for=list>Append</a> |parsedSyntax| to |inputArgumentSyntaxes|. | ||
|
|
||
| 12. Let |alphaValue| be the result of <a>Get</a>(|paintCtor|, "alpha"). | ||
| 12. Let |contextAttributesValue| be the result of <a>Get</a>(|paintCtor|, "ContextAttributes"). |
| with the paint canvas. Right now, the {{ContextAttributes}} has a boolean flag | ||
| indicating if the paint canvas contains an alpha channel. | ||
| <pre class='idl'> | ||
| dictionary ContextAttributes { |
There was a problem hiding this comment.
PaintContextAttributes to match below.
| @@ -605,7 +614,7 @@ When the user agent wants to <dfn>invoke a paint callback</dfn> given |name|, |i | |||
| 8. Let |renderingContext| be the result of <a>create a PaintRenderingContext2D object</a> given: | |||
There was a problem hiding this comment.
has the "create a PaintRenderingContext2D object" algorithm been updated to receive a contextAttributes object?
There was a problem hiding this comment.
Good catch, I have updated that section.
| with the paint canvas. Right now, the {{ContextAttributes}} has a boolean flag | ||
| indicating if the paint canvas contains an alpha channel. | ||
| <pre class='idl'> | ||
| dictionary ContextAttributes { |
| }; | ||
| </pre> | ||
|
|
||
| The {{PaintRenderingContext2DSettings}} allows the settings for the rendering context associated |
There was a problem hiding this comment.
This sentence is confusing, what do you mean "allows the settings"? Maybe contains or provides?
| </pre> | ||
|
|
||
| The {{PaintRenderingContext2DSettings}} allows the settings for the rendering context associated | ||
| with the paint canvas. Right now, the {{PaintRenderingContext2DSettings}} has a boolean flag |
There was a problem hiding this comment.
I think it would be more clear to have a bulleted list of what each value means. Also, perhaps instead of implying that we may add more settings we should say this provides a supported subset of the canvas context 2d settings (ideally with a link)
There was a problem hiding this comment.
Changed, please review the new commit.
|
Yay, thanks for reviewing it :) |
|
I have changed the function name to contextOptions, please review. |
@bfgeek @flackr: Please review.