Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1901153002:
Implement createImageBitmap(SVG) of intrinsic size = 0 (Closed)
DescriptionImplement createImageBitmap(SVG) of intrinsic size = 0
When we have a SVG image whose intrinsic size is 0, then
createImageBitmap
from the above SVG image should throw InvalidStateError. Also,
createImageBitmap with a cropRect should not throw. This CL adds a
layout to verify that.
BUG=604510
Committed: https://crrev.com/e1f01e9d0d2fadd0f3004833597486314a30287d
Cr-Commit-Position: refs/heads/master@{#389805}
Patch Set 1 #
Total comments: 3
Patch Set 2 : layout tests updated #Patch Set 3 : null check #
Messages
Total messages: 31 (11 generated)
xidachen@chromium.org changed reviewers: + davve@opera.com, junov@chromium.org
Could you please take a look? https://codereview.chromium.org/1901153002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-svg-no-intrinsic-size.html (right): https://codereview.chromium.org/1901153002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-svg-no-intrinsic-size.html:23: console.log("image2 loaded"); The test doesn't even get to this point.
> The test doesn't even get to this point. What you are saying is that the image will not even load? What if you add viewBox="0 0 1 1" to the svg tag?
Also, the spec PR has changed: If a crop rect is specified, the ImageBitmap creation should succeed, as long as the crop rect has non-zero size.
On 2016/04/19 16:59:25, Justin Novosad wrote: > > The test doesn't even get to this point. > > What you are saying is that the image will not even load? What if you add > viewBox="0 0 1 1" to the svg tag? Just tried adding this, the test still times out. I am sure that it is because the image is not loaded, because I tried to change image2.src = 'resources/zeroSize.svg' to image2.src = 'resources/empty.svg', then the test won't time out.
On 2016/04/19 17:03:59, xidachen wrote: > On 2016/04/19 16:59:25, Justin Novosad wrote: > > > The test doesn't even get to this point. > > > > What you are saying is that the image will not even load? What if you add > > viewBox="0 0 1 1" to the svg tag? > > Just tried adding this, the test still times out. I am sure that it is because > the image is not loaded, because I tried to change > image2.src = 'resources/zeroSize.svg' to image2.src = 'resources/empty.svg', > then the test won't time out. Sifting through the spec, I found a way to get an image with intrinsic dimension of zero, and it is not specific to svg: an <img> that uses srcset, with an entry that has a width descriptor of 0.
https://codereview.chromium.org/1901153002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-svg-no-intrinsic-size.html (right): https://codereview.chromium.org/1901153002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-svg-no-intrinsic-size.html:23: console.log("image2 loaded"); On 2016/04/19 16:52:03, xidachen wrote: > The test doesn't even get to this point. Oh, right. Such an image probably has Image::isNull() returning true, causing the loading code somewhere to drop it, IIRC.
On 2016/04/19 17:22:29, Justin Novosad wrote: > Sifting through the spec, I found a way to get an image with intrinsic dimension > of zero, and it is not specific to svg: an <img> that uses srcset, with an entry > that has a width descriptor of 0. I'm not that familiar with this part of the spec but isn't a zero width descriptor an error? "If the descriptor consists of a valid non-negative integer followed by a U+0077 LATIN SMALL LETTER W character ... If the result is zero, let error be yes. ...."
Right. I am not familiar with this part of the spec either. I misread it. It is the "source size" that needs to be 0, to end up with an intrinsic size of 0. As per: https://html.spec.whatwg.org/multipage/embedded-content.html#normalise-the-so... I am not sure how one would make that happen On Wed, Apr 20, 2016 at 7:48 AM, <davve@opera.com> wrote: > On 2016/04/19 17:22:29, Justin Novosad wrote: > > > Sifting through the spec, I found a way to get an image with intrinsic > dimension > > of zero, and it is not specific to svg: an <img> that uses srcset, with > an > entry > > that has a width descriptor of 0. > > I'm not that familiar with this part of the spec but isn't a zero width > descriptor an error? > > "If the descriptor consists of a valid non-negative integer followed by a > U+0077 > LATIN SMALL LETTER W character > ... If the result is zero, let error be yes. ...." > > https://codereview.chromium.org/1901153002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Right. I am not familiar with this part of the spec either. I misread it. It is the "source size" that needs to be 0, to end up with an intrinsic size of 0. As per: https://html.spec.whatwg.org/multipage/embedded-content.html#normalise-the-so... I am not sure how one would make that happen On Wed, Apr 20, 2016 at 7:48 AM, <davve@opera.com> wrote: > On 2016/04/19 17:22:29, Justin Novosad wrote: > > > Sifting through the spec, I found a way to get an image with intrinsic > dimension > > of zero, and it is not specific to svg: an <img> that uses srcset, with > an > entry > > that has a width descriptor of 0. > > I'm not that familiar with this part of the spec but isn't a zero width > descriptor an error? > > "If the descriptor consists of a valid non-negative integer followed by a > U+0077 > LATIN SMALL LETTER W character > ... If the result is zero, let error be yes. ...." > > https://codereview.chromium.org/1901153002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1901153002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-svg-no-intrinsic-size.html (right): https://codereview.chromium.org/1901153002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-svg-no-intrinsic-size.html:23: console.log("image2 loaded"); On 2016/04/19 19:31:47, David Vest wrote: > On 2016/04/19 16:52:03, xidachen wrote: > > The test doesn't even get to this point. > > Oh, right. Such an image probably has Image::isNull() returning true, causing > the loading code somewhere to drop it, IIRC. If you see value in getting around this issue, change your zeroSize.svg to look something like this: <svg xmlns="http://www.w3.org/2000/svg"> <script> onload = function() { document.documentElement.setAttribute('width', '0'); document.documentElement.setAttribute('height', '0'); } </script> </svg> I uncommented the rest of your changes and it seemed to work fine. If you like, the test could use some general love too. (<!DOCTYPE html> and remove unnecessary <html>, <head>, <body> tags comes to mind)
On 2016/04/21 09:50:48, David Vest wrote: > https://codereview.chromium.org/1901153002/diff/1/third_party/WebKit/LayoutTe... > File > third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-svg-no-intrinsic-size.html > (right): > > https://codereview.chromium.org/1901153002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-svg-no-intrinsic-size.html:23: > console.log("image2 loaded"); > On 2016/04/19 19:31:47, David Vest wrote: > > On 2016/04/19 16:52:03, xidachen wrote: > > > The test doesn't even get to this point. > > > > Oh, right. Such an image probably has Image::isNull() returning true, causing > > the loading code somewhere to drop it, IIRC. > > If you see value in getting around this issue, change your zeroSize.svg to look > something like this: > > <svg xmlns="http://www.w3.org/2000/svg"> > <script> > onload = function() { > document.documentElement.setAttribute('width', '0'); > document.documentElement.setAttribute('height', '0'); > } > </script> > </svg> > > I uncommented the rest of your changes and it seemed to work fine. > > If you like, the test could use some general love too. (<!DOCTYPE html> and > remove unnecessary <html>, <head>, <body> tags comes to mind) I saw the recent specs change of supporting createImageBitmap from a 0-size image. My question here is: does it make sense to have a 0-size image? In theory, this kind of image exist, but my personal opinion is that it doesn't make much sense, nor it is useful. If a 0-size image isn't really useful and not much meanful, then it may not worth the effort to support that? WDYT?
Description was changed from ========== Add layout test for createImageBitmap(SVG) of intrinsic size = 0 When we have a SVG image looks like this: <svg width="0" height="0" viewBox="0 0 1 1"> createImageBitmap from the above SVG image should throw InvalidStateError. This CL adds a layout test to verify that. BUG=604510 ========== to ========== Implement createImageBitmap(SVG) of intrinsic size = 0 When we have a SVG image whose intrinsic size is 0, then createImageBitmap from the above SVG image should throw InvalidStateError. Also, createImageBitmap with a cropRect should not throw. This CL adds a layout to verify that. BUG=604510 ==========
Description was changed from ========== Implement createImageBitmap(SVG) of intrinsic size = 0 When we have a SVG image whose intrinsic size is 0, then createImageBitmap from the above SVG image should throw InvalidStateError. Also, createImageBitmap with a cropRect should not throw. This CL adds a layout to verify that. BUG=604510 ========== to ========== Implement createImageBitmap(SVG) of intrinsic size = 0 When we have a SVG image whose intrinsic size is 0, then createImageBitmap from the above SVG image should throw InvalidStateError. Also, createImageBitmap with a cropRect should not throw. This CL adds a layout to verify that. BUG=604510 ==========
another gentle ping :) createImageBitmap(SVG) throws, createImageBitmap(SVG, cropRect) doesn't throw.
On 2016/04/21 12:15:36, xidachen wrote: > I saw the recent specs change of supporting createImageBitmap from a 0-size > image. My question here is: does it make sense to have a 0-size image? In > theory, this kind of image exist, but my personal opinion is that it doesn't > make much sense, nor it is useful. If a 0-size image isn't really useful and not > much meanful, then it may not worth the effort to support that? WDYT? I agree that this is not particularly useful, but the more important issue is about standardizing the behavior so that we don't end-up with websites that have different behaviors or bugs depending on the browser.
lgtm
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1901153002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1901153002/20001
The CQ bit was unchecked by xidachen@chromium.org
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1901153002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1901153002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xidachen@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/1901153002/#ps40001 (title: "null check")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1901153002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1901153002/40001
Message was sent while issue was closed.
Description was changed from ========== Implement createImageBitmap(SVG) of intrinsic size = 0 When we have a SVG image whose intrinsic size is 0, then createImageBitmap from the above SVG image should throw InvalidStateError. Also, createImageBitmap with a cropRect should not throw. This CL adds a layout to verify that. BUG=604510 ========== to ========== Implement createImageBitmap(SVG) of intrinsic size = 0 When we have a SVG image whose intrinsic size is 0, then createImageBitmap from the above SVG image should throw InvalidStateError. Also, createImageBitmap with a cropRect should not throw. This CL adds a layout to verify that. BUG=604510 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Implement createImageBitmap(SVG) of intrinsic size = 0 When we have a SVG image whose intrinsic size is 0, then createImageBitmap from the above SVG image should throw InvalidStateError. Also, createImageBitmap with a cropRect should not throw. This CL adds a layout to verify that. BUG=604510 ========== to ========== Implement createImageBitmap(SVG) of intrinsic size = 0 When we have a SVG image whose intrinsic size is 0, then createImageBitmap from the above SVG image should throw InvalidStateError. Also, createImageBitmap with a cropRect should not throw. This CL adds a layout to verify that. BUG=604510 Committed: https://crrev.com/e1f01e9d0d2fadd0f3004833597486314a30287d Cr-Commit-Position: refs/heads/master@{#389805} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e1f01e9d0d2fadd0f3004833597486314a30287d Cr-Commit-Position: refs/heads/master@{#389805} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
