Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2217623002:
Adding filter to the OffscreenCanvas-2d (Closed)
|
Created:
4 years, 4 months ago by zakerinasab Modified:
4 years, 4 months ago CC:
chromium-reviews, blink-reviews, haraken Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding filter to the OffscreenCanvas-2d
Adding access to the filter through adding exposure to
OffscreenCanvasRenderingContext2D.idl and also adding two layout tests.
BUG=593838
Committed: https://crrev.com/537f3ea83cb261831ec3bcbc0a931acff522d483
Cr-Commit-Position: refs/heads/master@{#410478}
Patch Set 1 #Patch Set 2 : Addressing filter exposure problem with webexposed/global-interface-listing.html #
Total comments: 15
Patch Set 3 : Addressing the comments on taint test. #
Total comments: 3
Patch Set 4 : Addressing final comments on layout tests. #
Total comments: 2
Patch Set 5 : Addressing exact color match in the layout tests #
Messages
Total messages: 38 (14 generated)
Description was changed from ========== Adding filter to the OfflineCanvas-2d Adding access to the filter through adding exposure to OffscreenCanvasRenderingContext2D.idl and also adding two layout tests. BUG= ========== to ========== Adding filter to the OfflineCanvas-2d Adding access to the filter through adding exposure to OffscreenCanvasRenderingContext2D.idl and also adding two layout tests. BUG= ==========
zakerinasab@chromium.org changed reviewers: + junov@chromium.org, xidachen@chromium.org, xlai@chromium.org
PTAL
lgtm lgtm with nits. Please wait for xlai@'s approval. https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter-origin-clean.html (right): https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter-origin-clean.html:35: var canvas = new OffscreenCanvas(200,200); nits: space between comma and 200, do it for other places too. https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter.html (right): https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter.html:60: var ocanvas = new OffscreenCanvas(300, 300); nits: change to oCanvas.
lgtm with nits Please wait for xlai@'s approval.
https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter-origin-clean.html (right): https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter-origin-clean.html:3: <filter id="drop-shadow"> You only use filter "merge-clean" in this test, right? Could you remove the other svg filters that are not used? https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter-origin-clean.html:53: }, "CSS shorthand filters never taint the canvas"); If you want to test taintedness, it should be a security test (which sits in LayoutTests/http/tests/security/ folder). Also, it should be asserting whether security exception is properly thrown. https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter-origin-clean.html:69: assert_not_equals(ctx.getImageData(0, 0, 1, 1), null); Can you assert the pixel with an exact number? not just assert not equals to null. The same for every other assert statements in this test. https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter.html (right): https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter.html:2: <svg style="display: block; width: 0; height: 0"> What's the purpose of putting an svg here? https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter.html:42: ctx.fillRect(125, 125, 250, 200); You're filling the color outside of the canvas boundary. Why? https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter.html:51: // the color of pixel (225, 301) must be black This pixel is outside your 300X300 canvas https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter.html:60: var ocanvas = new OffscreenCanvas(300, 300); Can you use a smaller canvas? It makes the test faster while serving the same purpose.
New patch submitted. Hopefully all the comments are addressed. https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter-origin-clean.html (right): https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter-origin-clean.html:35: var canvas = new OffscreenCanvas(200,200); On 2016/08/04 19:54:09, xidachen wrote: > nits: space between comma and 200, do it for other places too. Done. https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter-origin-clean.html:69: assert_not_equals(ctx.getImageData(0, 0, 1, 1), null); On 2016/08/05 15:28:19, xlai (Olivia) wrote: > Can you assert the pixel with an exact number? not just assert not equals to > null. The same for every other assert statements in this test. Done. https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter.html (right): https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter.html:2: <svg style="display: block; width: 0; height: 0"> On 2016/08/05 15:28:19, xlai (Olivia) wrote: > What's the purpose of putting an svg here? Removed. https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter.html:42: ctx.fillRect(125, 125, 250, 200); On 2016/08/05 15:28:19, xlai (Olivia) wrote: > You're filling the color outside of the canvas boundary. Why? Corrected. https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter.html:51: // the color of pixel (225, 301) must be black On 2016/08/05 15:28:19, xlai (Olivia) wrote: > This pixel is outside your 300X300 canvas Corrected. https://codereview.chromium.org/2217623002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter.html:60: var ocanvas = new OffscreenCanvas(300, 300); On 2016/08/05 15:28:19, xlai (Olivia) wrote: > Can you use a smaller canvas? It makes the test faster while serving the same > purpose. Done.
Please specify a BUG id in the description
Description was changed from ========== Adding filter to the OfflineCanvas-2d Adding access to the filter through adding exposure to OffscreenCanvasRenderingContext2D.idl and also adding two layout tests. BUG= ========== to ========== Adding filter to the OfflineCanvas-2d Adding access to the filter through adding exposure to OffscreenCanvasRenderingContext2D.idl and also adding two layout tests. BUG=2217623002 ==========
On 2016/08/05 20:16:22, junov (slow until August 8) wrote: > Please specify a BUG id in the description Done.
On 2016/08/05 20:30:44, zakerinasab wrote: > On 2016/08/05 20:16:22, junov (slow until August 8) wrote: > > Please specify a BUG id in the description > > Done. This is not a valid Bug ID. Please use 593838.
Description was changed from ========== Adding filter to the OfflineCanvas-2d Adding access to the filter through adding exposure to OffscreenCanvasRenderingContext2D.idl and also adding two layout tests. BUG=2217623002 ========== to ========== Adding filter to the OfflineCanvas-2d Adding access to the filter through adding exposure to OffscreenCanvasRenderingContext2D.idl and also adding two layout tests. BUG=593838 ==========
On 2016/08/05 20:41:32, xlai (Olivia) wrote: > On 2016/08/05 20:30:44, zakerinasab wrote: > > On 2016/08/05 20:16:22, junov (slow until August 8) wrote: > > > Please specify a BUG id in the description > > > > Done. > > This is not a valid Bug ID. Please use 593838. Corrected.
On 2016/08/05 21:09:46, zakerinasab wrote: > On 2016/08/05 20:41:32, xlai (Olivia) wrote: > > On 2016/08/05 20:30:44, zakerinasab wrote: > > > On 2016/08/05 20:16:22, junov (slow until August 8) wrote: > > > > Please specify a BUG id in the description > > > > > > Done. > > > > This is not a valid Bug ID. Please use 593838. > > Corrected. lgtm
The CQ bit was checked by zakerinasab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xidachen@chromium.org Link to the patchset: https://codereview.chromium.org/2217623002/#ps40001 (title: "Addressing the comments on taint test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I still have some comments on the test... https://codereview.chromium.org/2217623002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/security/OffscreenCanvas-2d-filter-taint.html (right): https://codereview.chromium.org/2217623002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/OffscreenCanvas-2d-filter-taint.html:27: function() { At which statement of this group of code do you expect to throw a Security exception? Please put assert_not_reached after that statement. https://codereview.chromium.org/2217623002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/OffscreenCanvas-2d-filter-taint.html:34: assert_not_equals(colorData[0], 0); Can you assert the colorData to a specific number, instead of assert_not_equals to 0? Likewise for all the other statements in this test. https://codereview.chromium.org/2217623002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/OffscreenCanvas-2d-filter-taint.html:59: }, "CSS shorthand filters never taint the canvas"); If you are testing taintedness, change the test message to reflect so. If not, change the assert_throws to assert_not_throws.
Description was changed from ========== Adding filter to the OfflineCanvas-2d Adding access to the filter through adding exposure to OffscreenCanvasRenderingContext2D.idl and also adding two layout tests. BUG=593838 ========== to ========== Adding filter to the OffscreenCanvas-2d Adding access to the filter through adding exposure to OffscreenCanvasRenderingContext2D.idl and also adding two layout tests. BUG=593838 ==========
On 2016/08/08 14:25:27, xlai (Olivia) wrote: > I still have some comments on the test... > > https://codereview.chromium.org/2217623002/diff/40001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/http/tests/security/OffscreenCanvas-2d-filter-taint.html > (right): > > https://codereview.chromium.org/2217623002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/security/OffscreenCanvas-2d-filter-taint.html:27: > function() { > At which statement of this group of code do you expect to throw a Security > exception? Please put assert_not_reached after that statement. > > https://codereview.chromium.org/2217623002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/security/OffscreenCanvas-2d-filter-taint.html:34: > assert_not_equals(colorData[0], 0); > Can you assert the colorData to a specific number, instead of assert_not_equals > to 0? Likewise for all the other statements in this test. > > https://codereview.chromium.org/2217623002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/security/OffscreenCanvas-2d-filter-taint.html:59: > }, "CSS shorthand filters never taint the canvas"); > If you are testing taintedness, change the test message to reflect so. If not, > change the assert_throws to assert_not_throws. Comments are addressed according to our recent conversation.
lgtm with nits
lgtm with nits https://codereview.chromium.org/2217623002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter.html (right): https://codereview.chromium.org/2217623002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter.html:4: <filter id="drop-shadow"> Pls remove this filter def. You didn't use it as an url in this test. https://codereview.chromium.org/2217623002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter.html:75: colorData = ctx.getImageData(0, 0, 1, 1).data; You might want to pick a pixel that's more meaningful to this test--e.g. the pixel that's covered by the blue shadow. Likewise for the test below.
On 2016/08/08 17:25:42, xlai (Olivia) wrote: > lgtm with nits > > https://codereview.chromium.org/2217623002/diff/60001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter.html > (right): > > https://codereview.chromium.org/2217623002/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter.html:4: > <filter id="drop-shadow"> > Pls remove this filter def. You didn't use it as an url in this test. > > https://codereview.chromium.org/2217623002/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-2d-filter.html:75: > colorData = ctx.getImageData(0, 0, 1, 1).data; > You might want to pick a pixel that's more meaningful to this test--e.g. the > pixel that's covered by the blue shadow. Likewise for the test below. Exact color check added.
The CQ bit was checked by zakerinasab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xidachen@chromium.org, junov@chromium.org, xlai@chromium.org Link to the patchset: https://codereview.chromium.org/2217623002/#ps80001 (title: "Addressing exact color match in the layout tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Rename OfflineCanvas -> OffscreenCanvas in the title of this CL before committing. Otherwise lgtm
On 2016/08/08 18:56:37, junov (slow until August 8) wrote: > Rename OfflineCanvas -> OffscreenCanvas in the title of this CL before > committing. Otherwise lgtm Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by zakerinasab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Adding filter to the OffscreenCanvas-2d Adding access to the filter through adding exposure to OffscreenCanvasRenderingContext2D.idl and also adding two layout tests. BUG=593838 ========== to ========== Adding filter to the OffscreenCanvas-2d Adding access to the filter through adding exposure to OffscreenCanvasRenderingContext2D.idl and also adding two layout tests. BUG=593838 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Adding filter to the OffscreenCanvas-2d Adding access to the filter through adding exposure to OffscreenCanvasRenderingContext2D.idl and also adding two layout tests. BUG=593838 ========== to ========== Adding filter to the OffscreenCanvas-2d Adding access to the filter through adding exposure to OffscreenCanvasRenderingContext2D.idl and also adding two layout tests. BUG=593838 Committed: https://crrev.com/537f3ea83cb261831ec3bcbc0a931acff522d483 Cr-Commit-Position: refs/heads/master@{#410478} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/537f3ea83cb261831ec3bcbc0a931acff522d483 Cr-Commit-Position: refs/heads/master@{#410478} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
