[CSS Paint API] Pass snapped concrete size to paint function#518
Merged
Conversation
chromium-wpt-export-bot
pushed a commit
to web-platform-tests/wpt
that referenced
this pull request
Nov 14, 2017
Right now when we set the PaintSize in CSSPaintDefinition::Paint(), the size is the concrete object size. There has been some discussion around here: w3c/css-houdini-drafts#508 and the solution is to pass the snapped concrete object size divided by device scale factor. The ongoing pull request to change the spec is here: w3c/css-houdini-drafts#518 This CL corrects the PaintSize to reflect the spec changes. It also adds a layout test to ensure the correctness. Bug: 774810 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I42b57db6ac0d40be2b408cb733a659f5ea14dfa0
chromium-wpt-export-bot
pushed a commit
to web-platform-tests/wpt
that referenced
this pull request
Nov 14, 2017
Right now when we set the PaintSize in CSSPaintDefinition::Paint(), the size is the concrete object size. There has been some discussion around here: w3c/css-houdini-drafts#508 and the solution is to pass the snapped concrete object size divided by device scale factor. The ongoing pull request to change the spec is here: w3c/css-houdini-drafts#518 This CL corrects the PaintSize to reflect the spec changes. It also adds a layout test to ensure the correctness. Bug: 774810 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I42b57db6ac0d40be2b408cb733a659f5ea14dfa0
bfgeek
reviewed
Nov 14, 2017
|
|
||
| 8. Let |renderingContext| be the result of <a>create a PaintRenderingContext2D object</a> given: | ||
| - "width" - The width given by |concreteObjectSize|. | ||
| - "width" - The width given by |concreteObjectSize|. |
bfgeek
approved these changes
Nov 14, 2017
flackr
reviewed
Nov 14, 2017
| negotiation</a> algorithm which is responsible for rendering an <<image>>, with |concreteObjectSize| | ||
| set to the <a>concrete object size</a> of the <a>box</a>. | ||
| negotiation</a> algorithm which is responsible for rendering an <<image>>, with |snappedConcreteObjectSize| | ||
| set to the rounded <a>concrete object size</a> of the <a>box</a>, divided by the |
Contributor
There was a problem hiding this comment.
I don't think this phrasing captures pixel snapping correctly. Also, AFAICT, concrete object size is in CSS pixels. Rounding the concrete object size would suggests that
- this only affects objects whose size is non-integral - this is not true as it affects integral sizes when they are rastered into a non-integral transformed space (i.e. non-integer devicePixelRatio).
- we always snap an object of a particular size in the same direction, but it actually depends on the object's location.
- all browsers snap the same way. This seems to be the case but should probably not be required.
Instead, I think we should specify this in a way that allows the browser to use its own subpixel snapping determination.
chromium-wpt-export-bot
pushed a commit
to web-platform-tests/wpt
that referenced
this pull request
Nov 15, 2017
Right now when we set the PaintSize in CSSPaintDefinition::Paint(), the size is the concrete object size. There has been some discussion around here: w3c/css-houdini-drafts#508 and the solution is to pass the snapped concrete object size divided by device scale factor. The ongoing pull request to change the spec is here: w3c/css-houdini-drafts#518 This CL corrects the PaintSize to reflect the spec changes. It also adds a layout test to ensure the correctness. Bug: 774810 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I42b57db6ac0d40be2b408cb733a659f5ea14dfa0
chromium-wpt-export-bot
pushed a commit
to web-platform-tests/wpt
that referenced
this pull request
Nov 15, 2017
Right now when we set the PaintSize in CSSPaintDefinition::Paint(), the size is the concrete object size. There has been some discussion around here: w3c/css-houdini-drafts#508 and the solution is to pass the snapped concrete object size divided by device scale factor. The ongoing pull request to change the spec is here: w3c/css-houdini-drafts#518 This CL corrects the PaintSize to reflect the spec changes. It also adds a layout test to ensure the correctness. Bug: 774810 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I42b57db6ac0d40be2b408cb733a659f5ea14dfa0 Reviewed-on: https://chromium-review.googlesource.com/733862 Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Commit-Queue: Xida Chen <xidachen@chromium.org> Cr-Commit-Position: refs/heads/master@{#516872}
chromium-wpt-export-bot
pushed a commit
to web-platform-tests/wpt
that referenced
this pull request
Nov 15, 2017
Right now when we set the PaintSize in CSSPaintDefinition::Paint(), the size is the concrete object size. There has been some discussion around here: w3c/css-houdini-drafts#508 and the solution is to pass the snapped concrete object size divided by device scale factor. The ongoing pull request to change the spec is here: w3c/css-houdini-drafts#518 This CL corrects the PaintSize to reflect the spec changes. It also adds a layout test to ensure the correctness. Bug: 774810 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I42b57db6ac0d40be2b408cb733a659f5ea14dfa0 Reviewed-on: https://chromium-review.googlesource.com/733862 Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Commit-Queue: Xida Chen <xidachen@chromium.org> Cr-Commit-Position: refs/heads/master@{#516872}
MXEBot
pushed a commit
to mirror/chromium
that referenced
this pull request
Nov 16, 2017
Right now when we set the PaintSize in CSSPaintDefinition::Paint(), the size is the concrete object size. There has been some discussion around here: w3c/css-houdini-drafts#508 and the solution is to pass the snapped concrete object size divided by device scale factor. The ongoing pull request to change the spec is here: w3c/css-houdini-drafts#518 This CL corrects the PaintSize to reflect the spec changes. It also adds a layout test to ensure the correctness. Bug: 774810 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I42b57db6ac0d40be2b408cb733a659f5ea14dfa0 Reviewed-on: https://chromium-review.googlesource.com/733862 Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Commit-Queue: Xida Chen <xidachen@chromium.org> Cr-Commit-Position: refs/heads/master@{#516872}
Contributor
Author
|
flackr@: I changed the description of |snappedConcreteSize|, let me know if it is better or worse. |
flackr
reviewed
Nov 16, 2017
| negotiation</a> algorithm which is responsible for rendering an <<image>>, with |snappedConcreteObjectSize| | ||
| defined as follows. Let |concreateObjectSize| be the <a>concrete object size</a> of the <a>box</a>, and | ||
| |deviceObjectSize| be the |concreateObjectSize| multiplied by the {{PaintWorkletGlobalScope/devicePixelRatio}}. | ||
| If the |deviceObjectSize| is non-integral, then adjust it to align with pixel boundary. Finally, set |
Contributor
There was a problem hiding this comment.
It's not a requirement to snap, but the browser is allowed to. How about something like this?
The |snappedConcreteObjectSize| is usually the same as the |concreteObjectSize|; however, the user agent may adjust the size such that it paints to pixel boundaries. If it does, the user agent should adjust the |snappedConcreteObjectSize| by the proportional change from its original size so that the paint function can adjust the drawing accordingly.
flackr
reviewed
Nov 16, 2017
| If the |deviceObjectSize| is non-integral, then adjust it to align with pixel boundary. Finally, set | ||
| |snappedConcreateObjectSize| to the adjusted |deviceObjectSize| divided by the | ||
| {{PaintWorkletGlobalScope/devicePixelRatio}}. | ||
| defined as follows. Let |concreateObjectSize| be the <a>concrete object size</a> of the <a>box</a>. |
Contributor
There was a problem hiding this comment.
s/concreateObjectSize/concreteObjectSize/ here and below.
flackr
reviewed
Nov 16, 2017
| defined as follows. Let |concreateObjectSize| be the <a>concrete object size</a> of the <a>box</a>. | ||
| The |snappedConcreateObjectSize| is usually the same as the |concreateObjectSize|. However, the | ||
| defined as follows. Let |concreteObjectSize| be the <a>concrete object size</a> of the <a>box</a>. | ||
| The |snappedConcreateObjectSize| is usually the same as the |concreteObjectSize|. However, the |
Contributor
There was a problem hiding this comment.
s/snappedConcreateObjectSize/snappedConcreteObjectSize here and below
flackr
approved these changes
Nov 16, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request address the issue here:
#508
Basically, instead of passing the concrete object size to the paint function, we let it pass the round(concrete object size) / device pixel ratio.
Please review. @bfgeek @flackr