Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2251493003:
Adding nullability support to ImageBitmapRenderingContext.transferImageBitmap (Closed)
|
Created:
4 years, 4 months ago by zakerinasab Modified:
4 years, 4 months ago Reviewers:
Justin Novosad 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 nullability support for the ImageBitmap parameter passed to ImageBitmapRenderingContext.transferImageBitmap
BUG=588243
Committed: https://crrev.com/0da9df35d5d1873066f7908110a3c44e8f0cf244
Cr-Commit-Position: refs/heads/master@{#414173}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressing comments from patch set 1. #
Total comments: 7
Patch Set 3 : Layout tests and code update for resetting the canvas #
Total comments: 9
Patch Set 4 : Addressing comments from patch set 3. #
Total comments: 2
Patch Set 5 : Addressing final comments. #
Messages
Total messages: 24 (9 generated)
zakerinasab@chromium.org changed reviewers: + junov@chromium.org
https://codereview.chromium.org/2251493003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap.html (right): https://codereview.chromium.org/2251493003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap.html:15: testRunner.notifyDone(); This will cause the test to end before consumeImageBitmap is called. Write a separate standalone test for this https://codereview.chromium.org/2251493003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.cpp (right): https://codereview.chromium.org/2251493003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.cpp:31: return; This is not the correct behavior. Passing null is supposed to reset the contents of the canvas. https://codereview.chromium.org/2251493003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.idl (right): https://codereview.chromium.org/2251493003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.idl:12: void transferFromImageBitmap([EnforceRange] ImageBitmap? bitmap); Why EnforceRange? I don't even understand what that does in this case, I thought this was just for when converting numbers to integer types.
Comments are addressed. https://codereview.chromium.org/2251493003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap.html (right): https://codereview.chromium.org/2251493003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap.html:15: testRunner.notifyDone(); On 2016/08/16 19:38:55, Justin Novosad wrote: > This will cause the test to end before consumeImageBitmap is called. Write a > separate standalone test for this Done. https://codereview.chromium.org/2251493003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.cpp (right): https://codereview.chromium.org/2251493003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.cpp:31: return; On 2016/08/16 19:38:55, Justin Novosad wrote: > This is not the correct behavior. Passing null is supposed to reset the contents > of the canvas. Done. https://codereview.chromium.org/2251493003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.idl (right): https://codereview.chromium.org/2251493003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.idl:12: void transferFromImageBitmap([EnforceRange] ImageBitmap? bitmap); On 2016/08/16 19:38:55, Justin Novosad wrote: > Why EnforceRange? I don't even understand what that does in this case, I > thought this was just for when converting numbers to integer types. Corrected.
https://codereview.chromium.org/2251493003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability.html (right): https://codereview.chromium.org/2251493003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability.html:8: var colorData = ctx.getImageData(50, 50, 1, 1).data; This test is going to crash. There is no 'getImageData' method on ImageBitmapRenderingContext. https://codereview.chromium.org/2251493003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability.html:16: var colorData = ctx.getImageData(50, 50, 1, 1).data; Same here https://codereview.chromium.org/2251493003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability.html:43: createImageBitmap(canvas).then(testNullParameterTransferFromImageBitmap); The test will end before testNullParameterTransferFromImageBitmap get called. You need to use async_test(). See docs: https://github.com/w3c/testharness.js/blob/master/docs/api.md#asynchronous-tests https://codereview.chromium.org/2251493003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap.html (right): https://codereview.chromium.org/2251493003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap.html:22: </script> Revert this file. No real change here except whitespace
https://codereview.chromium.org/2251493003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability.html (right): https://codereview.chromium.org/2251493003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability.html:8: var colorData = ctx.getImageData(50, 50, 1, 1).data; On 2016/08/16 20:52:07, Justin Novosad wrote: > This test is going to crash. There is no 'getImageData' method on > ImageBitmapRenderingContext. Corrected. https://codereview.chromium.org/2251493003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability.html:16: var colorData = ctx.getImageData(50, 50, 1, 1).data; On 2016/08/16 20:52:07, Justin Novosad wrote: > Same here Acknowledged. https://codereview.chromium.org/2251493003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability.html:43: createImageBitmap(canvas).then(testNullParameterTransferFromImageBitmap); On 2016/08/16 20:52:07, Justin Novosad wrote: > The test will end before testNullParameterTransferFromImageBitmap get called. > You need to use async_test(). See docs: > https://github.com/w3c/testharness.js/blob/master/docs/api.md#asynchronous-tests Acknowledged.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/2251493003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability-harness.html (right): https://codereview.chromium.org/2251493003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability-harness.html:1: <!DOCTYPE html> the "-harness" in the filename is not great. The name should indicate *what* is being tested, not how. https://codereview.chromium.org/2251493003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability-harness.html:2: <canvas id = 'dstCanvas' width='100' height='100'></canvas> These canvases do not need to be in the HTML markup since this is not a test that verifies document rendering. The test would be more efficient if you just created the canvases procedurally. In the code below, replace document.getElementById(...) with document.createElement('canvas') https://codereview.chromium.org/2251493003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability-harness.html:24: function promiseCreateImage(ctx) { This function is pointless. Just use createImageBitmap directly in the code below https://codereview.chromium.org/2251493003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability-harness.html:37: //myCtx.drawImage(dstCanvas, 0, 0); Do not commit the test in this state. It looks like we have a separate bug here: using a canvas with a bitmap as a drawImage source does not work. File that bug on crbug.com, fix it. The come back to this CL. https://codereview.chromium.org/2251493003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability.html (right): https://codereview.chromium.org/2251493003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability.html:18: dstCtx.transferFromImageBitmap(null); This test seems redundant with the other one. Testharness.js tests are preferable. You should get the other test to work and eliminate this one.
New patch submitted. https://codereview.chromium.org/2251493003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability-harness.html (right): https://codereview.chromium.org/2251493003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability-harness.html:1: <!DOCTYPE html> On 2016/08/18 15:27:27, Justin Novosad wrote: > the "-harness" in the filename is not great. The name should indicate *what* is > being tested, not how. Done. https://codereview.chromium.org/2251493003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability-harness.html:2: <canvas id = 'dstCanvas' width='100' height='100'></canvas> On 2016/08/18 15:27:27, Justin Novosad wrote: > These canvases do not need to be in the HTML markup since this is not a test > that verifies document rendering. The test would be more efficient if you just > created the canvases procedurally. In the code below, replace > document.getElementById(...) with document.createElement('canvas') Done. https://codereview.chromium.org/2251493003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability-harness.html:24: function promiseCreateImage(ctx) { On 2016/08/18 15:27:27, Justin Novosad wrote: > This function is pointless. Just use createImageBitmap directly in the code > below Done. https://codereview.chromium.org/2251493003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/imagebitmap/transferFromImageBitmap-nullability-harness.html:37: //myCtx.drawImage(dstCanvas, 0, 0); On 2016/08/18 15:27:27, Justin Novosad wrote: > Do not commit the test in this state. It looks like we have a separate bug > here: using a canvas with a bitmap as a drawImage source does not work. File > that bug on http://crbug.com, fix it. The come back to this CL. Done.
lgtm with nit https://codereview.chromium.org/2251493003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/2251493003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:261: if (renderingContext()->getImage()) Just "return m_context->getImage();" will suffice
https://codereview.chromium.org/2251493003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/2251493003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:261: if (renderingContext()->getImage()) On 2016/08/24 15:39:16, Justin Novosad wrote: > Just "return m_context->getImage();" will suffice Used "return renderingContext()->getImage().get();" as PassRefPtr cannot be converted to bool.
The CQ bit was checked by zakerinasab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org Link to the patchset: https://codereview.chromium.org/2251493003/#ps140001 (title: "Addressing final comments.")
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Adding nullability support for the ImageBitmap parameter passed to ImageBitmapRenderingContext.transferImageBitmap BUG=588243 ========== to ========== Adding nullability support for the ImageBitmap parameter passed to ImageBitmapRenderingContext.transferImageBitmap BUG=588243 Committed: https://crrev.com/0da9df35d5d1873066f7908110a3c44e8f0cf244 Cr-Commit-Position: refs/heads/master@{#414173} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0da9df35d5d1873066f7908110a3c44e8f0cf244 Cr-Commit-Position: refs/heads/master@{#414173} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
