Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1890613002:
Support createImageBitmap from a SVG image (Closed)
|
Created:
4 years, 8 months ago by xidachen Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport createImageBitmap from a SVG image
Right now when call createImageBitmap from a SVG image, it will throw a
InvalidStateError. This CL changes such that creating an ImageBitmap from
a SVG image is supported.
This CL includes two layout tests for the case of an SVG image with
and without intrinsic size. The spec here:
https://html.spec.whatwg.org/#imagebitmapfactories
states that createImageBitmap should throw an InvalidStateError
when the SVG image doesn't have an intrinsic size.
BUG=599228
Committed: https://crrev.com/04c55bfcb5eeb705f7d29a16c72fea6e9737a5b1
Cr-Commit-Position: refs/heads/master@{#387612}
Patch Set 1 #Patch Set 2 : cover test for SVG image without intrinsic size #
Total comments: 4
Patch Set 3 : address comments, update layout tests results #
Total comments: 1
Patch Set 4 : const #
Total comments: 1
Patch Set 5 : address sof's commnets #
Total comments: 4
Patch Set 6 : address davve's comments #
Total comments: 1
Patch Set 7 : nits #Messages
Total messages: 39 (17 generated)
Description was changed from ========== Support createImageBitmap from a SVG image Right now when call createImageBitmap from a SVG image, it will throw a InvalidStateError. This CL changes such that creating an ImageBitmap from a SVG image is supported. BUG=599228 ========== to ========== Support createImageBitmap from a SVG image Right now when call createImageBitmap from a SVG image, it will throw a InvalidStateError. This CL changes such that creating an ImageBitmap from a SVG image is supported. This CL includes two layout tests for the case of an SVG image with and without intrinsic size. The spec here: https://html.spec.whatwg.org/#imagebitmapfactories states that createImageBitmap should throw an InvalidStateEoor when the SVG image doesn't have an intrinsic size. BUG=599228 ==========
xidachen@chromium.org changed reviewers: + jbroman@chromium.org
PTAL
xidachen@chromium.org changed reviewers: + junov@chromium.org
https://codereview.chromium.org/1890613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right): https://codereview.chromium.org/1890613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLImageElement.cpp:693: SVGImage* image = static_cast<SVGImage*>(cachedImage()->getImage()); Prefer using toSVGImage: SVGImage* image = toSVGImage(cachedImage()->getImage()); https://codereview.chromium.org/1890613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLImageElement.cpp:695: exceptionState.throwDOMException(InvalidStateError, "The image element is an SVG image without intrinsic size."); nit: technically the image element isn't an SVG image, it contains one. Please change "is" back to "contains".
https://codereview.chromium.org/1890613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right): https://codereview.chromium.org/1890613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLImageElement.cpp:693: SVGImage* image = static_cast<SVGImage*>(cachedImage()->getImage()); On 2016/04/13 21:13:22, jbroman wrote: > Prefer using toSVGImage: > > SVGImage* image = toSVGImage(cachedImage()->getImage()); Done. https://codereview.chromium.org/1890613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLImageElement.cpp:695: exceptionState.throwDOMException(InvalidStateError, "The image element is an SVG image without intrinsic size."); On 2016/04/13 21:13:22, jbroman wrote: > nit: technically the image element isn't an SVG image, it contains one. Please > change "is" back to "contains". Done.
LGTM https://codereview.chromium.org/1890613002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/graphics/SVGImage.h (right): https://codereview.chromium.org/1890613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/graphics/SVGImage.h:89: bool hasIntrinsicSize(); nit: Can this method be const?
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/1890613002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1890613002/60001
https://codereview.chromium.org/1890613002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-svg.html (right): https://codereview.chromium.org/1890613002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-svg.html:14: shouldBe("d[0]", "0"); shouldBeEqualToNumber("d[0]", 0); etc.
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/1890613002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1890613002/80001
junov@chromium.org changed reviewers: + davve@opera.com
@davve: Please take a look at SVGImage changes.
https://codereview.chromium.org/1890613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/1890613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:182: layoutObject->computeIntrinsicSizingInfo(intrinsicSizingInfo); Is this call at all expensive? I am concerned that we will end up calling it twice (here, and in concreteObjectSize immediately after). Perhaps the concrete sizing info should be cached? Or perhaps you could instead have a method bool getSizingInfo(LayoutReplaced::IntrinsicSizingInfo* result); Then concreteObjectSize could take an optional sizingInfo argument and only compute the sizingInfo if the argument was omitted. If computeIntrinsicSizingInfo is trivially cheap, then never mind.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1890613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/1890613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:182: layoutObject->computeIntrinsicSizingInfo(intrinsicSizingInfo); On 2016/04/14 20:45:14, Justin Novosad wrote: > Is this call at all expensive? I am concerned that we will end up calling it > twice (here, and in concreteObjectSize immediately after). Perhaps the concrete > sizing info should be cached? Or perhaps you could instead have a method > bool getSizingInfo(LayoutReplaced::IntrinsicSizingInfo* result); > Then concreteObjectSize could take an optional sizingInfo argument and only > compute the sizingInfo if the argument was omitted. > > If computeIntrinsicSizingInfo is trivially cheap, then never mind. I don't think it's particularly expensive. But at the surface it seems unnecessary to go through the layout object. Might as well go straight for: SVGSVGElement* svg = svgRootElement(m_page.get()); if (!svg) return false; return svg->hasIntrinsicWidth() && svg->hasIntrinsicHeight(); That said, it's very strict to require both intrinsic width and intrinsic height for SVG with an aspect ratio (from the viewBox attribute). Example: <svg width="10" viewBox="0 0 1 1"/> This SVG can be considered to have an intrinsic height 10 even though the attribute is not there, since the viewBox attribute specifies a 1:1 aspect ratio. It would probably work just fine to let through this case. I can't load the spec link at the moment, so I'm not sure what the spec says about it. Personally I think it's a bit too strict to disallow it (and the corresponding case for width.) Regardless, it may be something we should add a test for.
Description was changed from ========== Support createImageBitmap from a SVG image Right now when call createImageBitmap from a SVG image, it will throw a InvalidStateError. This CL changes such that creating an ImageBitmap from a SVG image is supported. This CL includes two layout tests for the case of an SVG image with and without intrinsic size. The spec here: https://html.spec.whatwg.org/#imagebitmapfactories states that createImageBitmap should throw an InvalidStateEoor when the SVG image doesn't have an intrinsic size. BUG=599228 ========== to ========== Support createImageBitmap from a SVG image Right now when call createImageBitmap from a SVG image, it will throw a InvalidStateError. This CL changes such that creating an ImageBitmap from a SVG image is supported. This CL includes two layout tests for the case of an SVG image with and without intrinsic size. The spec here: https://html.spec.whatwg.org/#imagebitmapfactories states that createImageBitmap should throw an InvalidStateError when the SVG image doesn't have an intrinsic size. BUG=599228 ==========
According to Anne, the spec link should now be: https://html.spec.whatwg.org/#windoworworkerglobalscope (it was a mixin that was merged with other mixins that apply to Window and WorkerGlobalScope objects) https://html.spec.whatwg.org/#dom-createimagebitmap might work too.
https://codereview.chromium.org/1890613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/1890613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:182: layoutObject->computeIntrinsicSizingInfo(intrinsicSizingInfo); On 2016/04/15 07:24:31, David Vest wrote: > On 2016/04/14 20:45:14, Justin Novosad wrote: > > Is this call at all expensive? I am concerned that we will end up calling it > > twice (here, and in concreteObjectSize immediately after). Perhaps the > concrete > > sizing info should be cached? Or perhaps you could instead have a method > > bool getSizingInfo(LayoutReplaced::IntrinsicSizingInfo* result); > > Then concreteObjectSize could take an optional sizingInfo argument and only > > compute the sizingInfo if the argument was omitted. > > > > If computeIntrinsicSizingInfo is trivially cheap, then never mind. > > I don't think it's particularly expensive. But at the surface it seems > unnecessary to go through the layout object. Might as well go straight > for: > > SVGSVGElement* svg = svgRootElement(m_page.get()); > if (!svg) > return false; > return svg->hasIntrinsicWidth() && svg->hasIntrinsicHeight(); > > That said, it's very strict to require both intrinsic width and > intrinsic height for SVG with an aspect ratio (from the viewBox > attribute). Example: > > <svg width="10" viewBox="0 0 1 1"/> > > This SVG can be considered to have an intrinsic height 10 even though > the attribute is not there, since the viewBox attribute specifies a > 1:1 aspect ratio. It would probably work just fine to let through this > case. Now being able to read the spec text, it says "... has no intrinsic dimensions". I take that as wanting to allow the case I posted above. As I understand it, when createImageBitmap is called the size of the SVG has already been calculated and saved using concreteObjectSize(defaultObjectSize=300x150) (called when the SVG finishes loading in SVGImage::dataChanged()). The sizes we wish to reject is size based on that 300x150 rectangle. (side-note: I wish to someday get rid of that 300x150 rect that in most cases, as in this one, is just wrong), One idea is to use SVGImage::concreteObjectSize() here with an empty default size and just check the result for being non-empty. So adding and using something like: bool SVGImage::hasIntrinsicDimensions() const { return !concreteObjectSize(FloatSize()).isEmpty(); } This leaves the issue with how to handle SVG with explicit zero size: <svg width="0" height="0" viewBox="0 0 1 1"/> I think this qualifies as having no intrinsic dimensions, but I'm not sure. https://github.com/whatwg/html/issues/1057 filed for clarification. WDYT?
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/1890613002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1890613002/100001
https://codereview.chromium.org/1890613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/1890613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:182: layoutObject->computeIntrinsicSizingInfo(intrinsicSizingInfo); Thanks David. I am going with return !concreteObjectSize(FloatSize()).isEmpty(); I will land this CL for now, but for sure needs more test cases later on.
lgtm https://codereview.chromium.org/1890613002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right): https://codereview.chromium.org/1890613002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLImageElement.cpp:695: exceptionState.throwDOMException(InvalidStateError, "The image element contains an SVG image without intrinsic size."); nit: To align with the specification, you might want to word this with "intrinsic dimensions" instead of "intrinsic size".
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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1890613002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1890613002/120001
The CQ bit was unchecked by xidachen@chromium.org
The CQ bit was checked by xidachen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org, davve@opera.com Link to the patchset: https://codereview.chromium.org/1890613002/#ps120001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1890613002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1890613002/120001
Message was sent while issue was closed.
Description was changed from ========== Support createImageBitmap from a SVG image Right now when call createImageBitmap from a SVG image, it will throw a InvalidStateError. This CL changes such that creating an ImageBitmap from a SVG image is supported. This CL includes two layout tests for the case of an SVG image with and without intrinsic size. The spec here: https://html.spec.whatwg.org/#imagebitmapfactories states that createImageBitmap should throw an InvalidStateError when the SVG image doesn't have an intrinsic size. BUG=599228 ========== to ========== Support createImageBitmap from a SVG image Right now when call createImageBitmap from a SVG image, it will throw a InvalidStateError. This CL changes such that creating an ImageBitmap from a SVG image is supported. This CL includes two layout tests for the case of an SVG image with and without intrinsic size. The spec here: https://html.spec.whatwg.org/#imagebitmapfactories states that createImageBitmap should throw an InvalidStateError when the SVG image doesn't have an intrinsic size. BUG=599228 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Support createImageBitmap from a SVG image Right now when call createImageBitmap from a SVG image, it will throw a InvalidStateError. This CL changes such that creating an ImageBitmap from a SVG image is supported. This CL includes two layout tests for the case of an SVG image with and without intrinsic size. The spec here: https://html.spec.whatwg.org/#imagebitmapfactories states that createImageBitmap should throw an InvalidStateError when the SVG image doesn't have an intrinsic size. BUG=599228 ========== to ========== Support createImageBitmap from a SVG image Right now when call createImageBitmap from a SVG image, it will throw a InvalidStateError. This CL changes such that creating an ImageBitmap from a SVG image is supported. This CL includes two layout tests for the case of an SVG image with and without intrinsic size. The spec here: https://html.spec.whatwg.org/#imagebitmapfactories states that createImageBitmap should throw an InvalidStateError when the SVG image doesn't have an intrinsic size. BUG=599228 Committed: https://crrev.com/04c55bfcb5eeb705f7d29a16c72fea6e9737a5b1 Cr-Commit-Position: refs/heads/master@{#387612} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/04c55bfcb5eeb705f7d29a16c72fea6e9737a5b1 Cr-Commit-Position: refs/heads/master@{#387612}
Message was sent while issue was closed.
Catching-up on reviews... post-commit lgtm |
