Support fragment URLs for all kinds of SVG images

Support fragment URLs for all kinds of SVG images

Add support for SVG fragment URLs in CSS backgrounds, video poster
images, image inputs and svg:image scenarios.

This patch does so by storing fragment URLs for each
SVGImageForContainer. Previous to this patch the URL was stored in the
SVGImage. This is non-optimal since the same SVGImage represents all
URLs where only the fragment string differ. This required us to update
the URL for each different draw for different containers. This patch
removes the hack in ImageResource::svgImageForLayoutObject which made
this work previously and instead stores the fragment URL in the
SVGImageForContainer.

But just storing the URL in SVGImageForContainer is not enough since
those objects are recreated all the time. See
ImageResource::setContainerParametersForLayoutObject (prior to this
patch named ImageResource::setContainerSizeForLayoutObject). Thus we
also need to store the fragment URLs one level up the ownership
hierarchy from ImageResource: in StyleFetchedImage,
StyleFetchedImageSet and LayoutImageResource. These are the
long-living objects that'll hold the URL fragment over the life-time
of their respective DOM counterparts.

[davve, 2015-09-07 12:12:56]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-07 12:13:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1314793010/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1314793010/1

[commit-bot: I haz the power, 2015-09-07 12:14:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-07 12:14:30]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[davve, 2015-09-07 12:57:55]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-07 12:58:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1314793010/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1314793010/20001

[davve, 2015-09-07 14:14:59]

davve@opera.com changed reviewers:
+ fmalita@chromium.org, fs@opera.com, pdr@chromium.org

[davve, 2015-09-07 14:15:00]

Rather different (and slightly more complex) than the WebKit fix:

http://trac.webkit.org/changeset/185395

and I'm not sure why. Perhaps WebKit does things differently that
makes it possible to store the fragment URL in the SVGImage, but
I couldn't wrap my head around how that would work in Blink.
Basically ran into

https://bugs.webkit.org/show_bug.cgi?id=91790#c9

right from the start when I did the corresponding change to
blink.

Or I'm overlooking something... WDYT?

[commit-bot: I haz the power, 2015-09-07 14:50:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-07 14:50:08]

Dry run: This issue passed the CQ dry run.

[fs, 2015-09-07 16:07:30]

On 2015/09/07 14:15:00, David Vest wrote:
> Rather different (and slightly more complex) than the WebKit fix:
> 
> http://trac.webkit.org/changeset/185395
> 
> and I'm not sure why. Perhaps WebKit does things differently that
> makes it possible to store the fragment URL in the SVGImage, but
> I couldn't wrap my head around how that would work in Blink.
> Basically ran into
> 
> https://bugs.webkit.org/show_bug.cgi?id=91790#c9
> 
> right from the start when I did the corresponding change to
> blink.

Yes, I also failed to see how an SVGImage would have a unique URL (w/ fragment)
wrt the ImageResource it stems from. Maybe there was a change separately that
made sure of that...
 
> Or I'm overlooking something... WDYT?

I did not manage to think any more "elegant" way either, so let's wait for
cross-Atlantic input...

[fs, 2015-09-07 16:07:44]

https://codereview.chromium.org/1314793010/diff/20001/LayoutTests/svg/css/svg...
File LayoutTests/svg/css/svg-resource-fragment-identifier-background.html
(right):

https://codereview.chromium.org/1314793010/diff/20001/LayoutTests/svg/css/svg...
LayoutTests/svg/css/svg-resource-fragment-identifier-background.html:1: <style>
Missing doctype here and in a few more places.

https://codereview.chromium.org/1314793010/diff/20001/Source/core/fetch/Image...
File Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1314793010/diff/20001/Source/core/fetch/Image...
Source/core/fetch/ImageResource.cpp:33: #include "core/html/HTMLImageElement.h"
This one is no longer now, or? (Also in DEPS)

[fs, 2015-09-07 16:10:28]

On 2015/09/07 16:07:44, fs wrote:
>
https://codereview.chromium.org/1314793010/diff/20001/LayoutTests/svg/css/svg...
> File LayoutTests/svg/css/svg-resource-fragment-identifier-background.html
> (right):
> 
>
https://codereview.chromium.org/1314793010/diff/20001/LayoutTests/svg/css/svg...
> LayoutTests/svg/css/svg-resource-fragment-identifier-background.html:1:
<style>
> Missing doctype here and in a few more places.
> 
>
https://codereview.chromium.org/1314793010/diff/20001/Source/core/fetch/Image...
> File Source/core/fetch/ImageResource.cpp (right):
> 
>
https://codereview.chromium.org/1314793010/diff/20001/Source/core/fetch/Image...
> Source/core/fetch/ImageResource.cpp:33: #include
"core/html/HTMLImageElement.h"
> This one is no longer now, or? (Also in DEPS)

s/longer/longer needed/

[davve, 2015-09-08 06:54:14]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-08 06:54:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1314793010/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1314793010/60001

[commit-bot: I haz the power, 2015-09-08 08:23:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-08 08:23:28]

Dry run: This issue passed the CQ dry run.

[davve, 2015-09-08 09:46:45]

Thanks for review, fs!

https://codereview.chromium.org/1314793010/diff/20001/LayoutTests/svg/css/svg...
File LayoutTests/svg/css/svg-resource-fragment-identifier-background.html
(right):

https://codereview.chromium.org/1314793010/diff/20001/LayoutTests/svg/css/svg...
LayoutTests/svg/css/svg-resource-fragment-identifier-background.html:1: <style>
On 2015/09/07 16:07:44, fs wrote:
> Missing doctype here and in a few more places.

Done.

https://codereview.chromium.org/1314793010/diff/20001/Source/core/fetch/Image...
File Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1314793010/diff/20001/Source/core/fetch/Image...
Source/core/fetch/ImageResource.cpp:33: #include "core/html/HTMLImageElement.h"
On 2015/09/07 16:07:44, fs wrote:
> This one is no longer now, or? (Also in DEPS)

Done.

[f(malita), 2015-09-08 18:41:19]

fmalita@chromium.org changed reviewers:
+ japhet@chromium.org

[f(malita), 2015-09-08 18:41:19]

I think having per-SVGImageForContainer url fragments makes sense, but plumbing
all that info through SVG-unrelated classes is odd.

I'm not familiar with image/resource loading, someone better equipped should
take a look at those parts.

https://codereview.chromium.org/1314793010/diff/60001/Source/core/fetch/Image...
File Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1314793010/diff/60001/Source/core/fetch/Image...
Source/core/fetch/ImageResource.cpp:214: void
ImageResource::setContainerParametersForLayoutObject(const ImageResourceClient*
layoutObject, const IntSize& containerSize, float containerZoom, const KURL&
urlWithFragment)
Is the fragment not visible at the Resource level? There's code to track it, at
least temporarily:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

If the super class has access to the fragment maybe we can use that instead or
relying on an explicit argument - then we could drop all the related plumbing.

https://codereview.chromium.org/1314793010/diff/60001/Source/core/style/Style...
File Source/core/style/StyleFetchedImageSet.h (right):

https://codereview.chromium.org/1314793010/diff/60001/Source/core/style/Style...
Source/core/style/StyleFetchedImageSet.h:86: KURL m_urlWithFragment;
Plumbing the fragment through several general classes for the sole benefit of
SVG images seems unfortunate.

[davve, 2015-09-08 19:12:25]

https://codereview.chromium.org/1314793010/diff/60001/Source/core/fetch/Image...
File Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1314793010/diff/60001/Source/core/fetch/Image...
Source/core/fetch/ImageResource.cpp:214: void
ImageResource::setContainerParametersForLayoutObject(const ImageResourceClient*
layoutObject, const IntSize& containerSize, float containerZoom, const KURL&
urlWithFragment)
On 2015/09/08 18:41:19, f(malita) wrote:
> Is the fragment not visible at the Resource level? There's code to track it,
at
> least temporarily:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> If the super class has access to the fragment maybe we can use that instead or
> relying on an explicit argument - then we could drop all the related plumbing.

m_fragmentIdentifierForRequest is emptied on ::load so not generally accessible
at paint time as the code looks today. Having the fragment at the resource level
seems wrong in general to me, since we want to use the same resource regardless
of how many urls with fragments that point to the same actual resource.

https://codereview.chromium.org/1314793010/diff/60001/Source/core/style/Style...
File Source/core/style/StyleFetchedImageSet.h (right):

https://codereview.chromium.org/1314793010/diff/60001/Source/core/style/Style...
Source/core/style/StyleFetchedImageSet.h:86: KURL m_urlWithFragment;
On 2015/09/08 18:41:19, f(malita) wrote:
> Plumbing the fragment through several general classes for the sole benefit of
> SVG images seems unfortunate.

Yes. The feature seems to have a significant cost. We might come up with a
better way of solving it or we may drop the feature if we deem it not worth the
complexity. Personally I didn't find the cost particularly high for the author
gains but I might have a skewed view begin the one who wrote the code.

If we wouldn't trash SVGImageForContainer objects all the time, we might not
have to carry the URL in parent classes like this. I can't assess at this point
the trade-offs and doability of doing so though.

[pdr., 2015-09-08 19:59:27]

Very nice. I think this is a reasonable approach.

[f(malita), 2015-09-08 20:45:51]

https://codereview.chromium.org/1314793010/diff/60001/Source/core/fetch/Image...
File Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1314793010/diff/60001/Source/core/fetch/Image...
Source/core/fetch/ImageResource.cpp:214: void
ImageResource::setContainerParametersForLayoutObject(const ImageResourceClient*
layoutObject, const IntSize& containerSize, float containerZoom, const KURL&
urlWithFragment)
On 2015/09/08 19:12:25, David Vest wrote:
> m_fragmentIdentifierForRequest is emptied on ::load so not generally
accessible
> at paint time as the code looks today.

Yup, but I can't tell whether that part is needed/important (why only observe it
on the first load? do we even call load more than once?).

> Having the fragment at the resource level
> seems wrong in general to me, since we want to use the same resource
regardless
> of how many urls with fragments that point to the same actual resource.

Makes sense.

https://codereview.chromium.org/1314793010/diff/60001/Source/core/style/Style...
File Source/core/style/StyleFetchedImageSet.h (right):

https://codereview.chromium.org/1314793010/diff/60001/Source/core/style/Style...
Source/core/style/StyleFetchedImageSet.h:86: KURL m_urlWithFragment;
On 2015/09/08 19:12:25, David Vest wrote:
> On 2015/09/08 18:41:19, f(malita) wrote:
> > Plumbing the fragment through several general classes for the sole benefit
of
> > SVG images seems unfortunate.
> 
> Yes. The feature seems to have a significant cost. We might come up with a
> better way of solving it or we may drop the feature if we deem it not worth
the
> complexity. Personally I didn't find the cost particularly high for the author
> gains but I might have a skewed view begin the one who wrote the code.

I also think the tradeoff is worth it, and I'm certainly not opposed to landing
as-is if a better approach is not obvious.

[davve, 2015-09-09 14:22:54]

https://codereview.chromium.org/1314793010/diff/60001/Source/core/fetch/Image...
File Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1314793010/diff/60001/Source/core/fetch/Image...
Source/core/fetch/ImageResource.cpp:214: void
ImageResource::setContainerParametersForLayoutObject(const ImageResourceClient*
layoutObject, const IntSize& containerSize, float containerZoom, const KURL&
urlWithFragment)
On 2015/09/08 20:45:51, f(malita) wrote:
> On 2015/09/08 19:12:25, David Vest wrote:
> > m_fragmentIdentifierForRequest is emptied on ::load so not generally
> accessible
> > at paint time as the code looks today.
> 
> Yup, but I can't tell whether that part is needed/important (why only observe
it
> on the first load? do we even call load more than once?).

Fwiw, on a simple test like
svg-resource-fragment-identifier-background.html attached in the CL, I
can't observe m_fragmentIdentifierForRequest actually being set.

Already in ResourceFetcher::requestResource there is a call to
removeFragmentIdentifierIfNeeded before hitting/checking the cache,
right before the resource is created (when not found in cache) so I'm
not sure when m_fragmentIdentifierForRequest is used.

https://codereview.chromium.org/1314793010/diff/60001/Source/core/style/Style...
File Source/core/style/StyleFetchedImageSet.h (right):

https://codereview.chromium.org/1314793010/diff/60001/Source/core/style/Style...
Source/core/style/StyleFetchedImageSet.h:86: KURL m_urlWithFragment;
On 2015/09/08 19:12:25, David Vest wrote:

> If we wouldn't trash SVGImageForContainer objects all the time, we might not
> have to carry the URL in parent classes like this. I can't assess at this
point
> the trade-offs and doability of doing so though.

I looked into keeping the fragment URL isolated to
SVGImageForContainer today but failed to find a way of not
contaminating all the layers in between style and SVG.
SVGImageForContainers are created on demand by the ImageResource
during layout or paint. They're indexed on LayoutObjects so they can't
really exist before the LayoutObject has been created.

For background images it looks like we must carry the fragment URL
from CSSImageValue (where it's stored as a string m_absoluteURL)
computed style time to layout/paint time when we create the
SVGImageForContainer.

StyleFetchedImage{,Set} seemed like one reasonable place to do that,
but yes we'll pay the price for all background images that doesn't
support fragment URLs. (For normal images, it might be easier since we
can access the src URL using imageSourceURL() during layout/paint or
so.)

https://codereview.chromium.org/1314793010/diff/60001/Source/core/svg/graphic...
File Source/core/svg/graphics/SVGImageForContainer.h (right):

https://codereview.chromium.org/1314793010/diff/60001/Source/core/svg/graphic...
Source/core/svg/graphics/SVGImageForContainer.h:79: KURL m_urlWithFragment;
I've come to realize that associating url fragments with SVGImageForContainer
isn't as a clean fit as I initially thought.

Since SVGImageForContainer is associated per layout object, this system will
break down if there is more than one fragment URL per layout object. I.e. adding
a "background-image: url('some.svg#red');" to an img with src=some.svg#green
will make them compete for the URL in SVGImageForContainer. (In current patch,
the background seems to win)

May have to go back to the drawing board on this one.

[fs, 2015-09-09 22:19:22]

On 2015/09/09 14:22:54, David Vest wrote:
...
>
https://codereview.chromium.org/1314793010/diff/60001/Source/core/svg/graphic...
> File Source/core/svg/graphics/SVGImageForContainer.h (right):
> 
>
https://codereview.chromium.org/1314793010/diff/60001/Source/core/svg/graphic...
> Source/core/svg/graphics/SVGImageForContainer.h:79: KURL m_urlWithFragment;
> I've come to realize that associating url fragments with SVGImageForContainer
> isn't as a clean fit as I initially thought.
> 
> Since SVGImageForContainer is associated per layout object, this system will
> break down if there is more than one fragment URL per layout object. I.e.
adding
> a "background-image: url('some.svg#red');" to an img with src=some.svg#green
> will make them compete for the URL in SVGImageForContainer. (In current patch,
> the background seems to win)

Don't we potentially have this case already for something like:

<img src="some.svg#blue" style="background-image; url(some.svg#green);
object-fit: contain; width: ...">

(contrived you say?!)

Or maybe a bell comes running to save us there?

[f(malita), 2015-09-09 23:55:41]

On 2015/09/09 14:22:54, David Vest wrote:
> Since SVGImageForContainer is associated per layout object, this system will
> break down if there is more than one fragment URL per layout object. I.e.
adding
> a "background-image: url('some.svg#red');" to an img with src=some.svg#green
> will make them compete for the URL in SVGImageForContainer. (In current patch,
> the background seems to win)
> 
> May have to go back to the drawing board on this one.

It occurred to me that all this plumbing is to make fragment info persistent and
available at SVGImage::draw() time.  Rather than trying to do it at resource
load/cache time, would it be easier to figure fragment info at paint time?

Presumably the paint code knows exactly what decoration piece is being drawn, so
discriminating between different fragments of the same resource should not be a
problem.  Then there is no persistent mapping to worry about, we just need some
way to pass this transient info down to the SVGImage draw method.

Does this make any sense?

[fs, 2015-09-10 07:45:47]

On 2015/09/09 23:55:41, f(malita) wrote:
> On 2015/09/09 14:22:54, David Vest wrote:
> > Since SVGImageForContainer is associated per layout object, this system will
> > break down if there is more than one fragment URL per layout object. I.e.
> adding
> > a "background-image: url('some.svg#red');" to an img with src=some.svg#green
> > will make them compete for the URL in SVGImageForContainer. (In current
patch,
> > the background seems to win)
> > 
> > May have to go back to the drawing board on this one.
> 
> It occurred to me that all this plumbing is to make fragment info persistent
and
> available at SVGImage::draw() time.  Rather than trying to do it at resource
> load/cache time, would it be easier to figure fragment info at paint time?
> 
> Presumably the paint code knows exactly what decoration piece is being drawn,
so
> discriminating between different fragments of the same resource should not be
a
> problem.  Then there is no persistent mapping to worry about, we just need
some
> way to pass this transient info down to the SVGImage draw method.
> 
> Does this make any sense?

Yeah, that makes sense to me - it's still a bit of a bundle to pass around
though.

[davve, 2015-09-10 13:09:18]

On 2015/09/10 07:45:47, fs wrote:
> On 2015/09/09 23:55:41, f(malita) wrote:
> > On 2015/09/09 14:22:54, David Vest wrote:
> > > Since SVGImageForContainer is associated per layout object, this system
will
> > > break down if there is more than one fragment URL per layout object. I.e.
> > adding
> > > a "background-image: url('some.svg#red');" to an img with
src=some.svg#green
> > > will make them compete for the URL in SVGImageForContainer. (In current
> patch,
> > > the background seems to win)
> > > 
> > > May have to go back to the drawing board on this one.
> > 
> > It occurred to me that all this plumbing is to make fragment info persistent
> and
> > available at SVGImage::draw() time.  Rather than trying to do it at resource
> > load/cache time, would it be easier to figure fragment info at paint time?
> > 
> > Presumably the paint code knows exactly what decoration piece is being
drawn,
> so
> > discriminating between different fragments of the same resource should not
be
> a
> > problem.  Then there is no persistent mapping to worry about, we just need
> some
> > way to pass this transient info down to the SVGImage draw method.
> > 
> > Does this make any sense?
> 
> Yeah, that makes sense to me - it's still a bit of a bundle to pass around
> though.

Right. I've experimented with passing the URL through Image::draw here:

https://codereview.chromium.org/1334703002

Only fragments for SVG-in-img and SVG in backgrounds supported there.
It's intrusive too, but in another way. And a somewhat cleaner and
explicit design. Was it something like this you had in mind?

[f(malita), 2015-09-10 13:37:40]

On 2015/09/10 13:09:18, David Vest wrote:
> On 2015/09/10 07:45:47, fs wrote:
> > On 2015/09/09 23:55:41, f(malita) wrote:
> > > On 2015/09/09 14:22:54, David Vest wrote:
> > > > Since SVGImageForContainer is associated per layout object, this system
> will
> > > > break down if there is more than one fragment URL per layout object.
I.e.
> > > adding
> > > > a "background-image: url('some.svg#red');" to an img with
> src=some.svg#green
> > > > will make them compete for the URL in SVGImageForContainer. (In current
> > patch,
> > > > the background seems to win)
> > > > 
> > > > May have to go back to the drawing board on this one.
> > > 
> > > It occurred to me that all this plumbing is to make fragment info
persistent
> > and
> > > available at SVGImage::draw() time.  Rather than trying to do it at
resource
> > > load/cache time, would it be easier to figure fragment info at paint time?
> > > 
> > > Presumably the paint code knows exactly what decoration piece is being
> drawn,
> > so
> > > discriminating between different fragments of the same resource should not
> be
> > a
> > > problem.  Then there is no persistent mapping to worry about, we just need
> > some
> > > way to pass this transient info down to the SVGImage draw method.
> > > 
> > > Does this make any sense?
> > 
> > Yeah, that makes sense to me - it's still a bit of a bundle to pass around
> > though.
> 
> Right. I've experimented with passing the URL through Image::draw here:
> 
> https://codereview.chromium.org/1334703002
> 
> Only fragments for SVG-in-img and SVG in backgrounds supported there.
> It's intrusive too, but in another way. And a somewhat cleaner and
> explicit design. Was it something like this you had in mind?

Yeah, that's what I was thinking of.

One idea to reduce intrusiveness: instead of making the KURL arg explicit on the
Image interface, we could introduce an (SVG)Image specialization
(SVGImageFragment?) that wraps an SVGImage + fragment ID and create these on the
fly, if/when needed.

Something like

static PassRefPtr<Image> imageForFragment(PassRefPtr<Image> image, const KURL&
url)
{
    if (!image->isSVGImage() || !url.hasFragmentIdentifier())
        return image;

    return SVGImageFragment::create(image, url.fragmentIdentifier());
}

then maybe clients could call context->drawImage(imageForFragment(image,
resource_url), ...) unconditionally, and we don't have to touch the GC/Image
draw interface.

The downside is allocating these temp wrappers when fragments are involved, but
I don't think that's something to worry about now that SP caches the world.

[davve, 2015-09-11 07:22:56]

On 2015/09/10 13:37:40, f(malita) wrote:
> One idea to reduce intrusiveness: instead of making the KURL arg explicit on
the
> Image interface, we could introduce an (SVG)Image specialization
> (SVGImageFragment?) that wraps an SVGImage + fragment ID and create these on
the
> fly, if/when needed.

Interesting idea! One detail is that we would have to wrap
SVGImageForContainer, not an SVGImage. In a sense the result would end
up similar to the initial patch, but the URL would be added to the
SVGImageForContainer "on demand" during paint instead of stored in
SVGImageForContainer always.

[davve, 2015-09-15 08:59:41]

On 2015/09/10 13:37:40, f(malita) wrote:
> On 2015/09/10 13:09:18, David Vest wrote:
> > On 2015/09/10 07:45:47, fs wrote:
> > > On 2015/09/09 23:55:41, f(malita) wrote:
> > > > On 2015/09/09 14:22:54, David Vest wrote:
> > > > > Since SVGImageForContainer is associated per layout object, this
system
> > will
> > > > > break down if there is more than one fragment URL per layout object.
> I.e.
> > > > adding
> > > > > a "background-image: url('some.svg#red');" to an img with
> > src=some.svg#green
> > > > > will make them compete for the URL in SVGImageForContainer. (In
current
> > > patch,
> > > > > the background seems to win)
> > > > > 
> > > > > May have to go back to the drawing board on this one.
> > > > 
> > > > It occurred to me that all this plumbing is to make fragment info
> persistent
> > > and
> > > > available at SVGImage::draw() time.  Rather than trying to do it at
> resource
> > > > load/cache time, would it be easier to figure fragment info at paint
time?
> > > > 
> > > > Presumably the paint code knows exactly what decoration piece is being
> > drawn,
> > > so
> > > > discriminating between different fragments of the same resource should
not
> > be
> > > a
> > > > problem.  Then there is no persistent mapping to worry about, we just
need
> > > some
> > > > way to pass this transient info down to the SVGImage draw method.
> > > > 
> > > > Does this make any sense?
> > > 
> > > Yeah, that makes sense to me - it's still a bit of a bundle to pass around
> > > though.
> > 
> > Right. I've experimented with passing the URL through Image::draw here:
> > 
> > https://codereview.chromium.org/1334703002
> > 
> > Only fragments for SVG-in-img and SVG in backgrounds supported there.
> > It's intrusive too, but in another way. And a somewhat cleaner and
> > explicit design. Was it something like this you had in mind?
> 
> Yeah, that's what I was thinking of.
> 
> One idea to reduce intrusiveness: instead of making the KURL arg explicit on
the
> Image interface, we could introduce an (SVG)Image specialization
> (SVGImageFragment?) that wraps an SVGImage + fragment ID and create these on
the
> fly, if/when needed.
> 
> Something like
> 
> static PassRefPtr<Image> imageForFragment(PassRefPtr<Image> image, const KURL&
> url)
> {
>     if (!image->isSVGImage() || !url.hasFragmentIdentifier())
>         return image;
> 
>     return SVGImageFragment::create(image, url.fragmentIdentifier());
> }
> 
> then maybe clients could call context->drawImage(imageForFragment(image,
> resource_url), ...) unconditionally, and we don't have to touch the GC/Image
> draw interface.
> 
> The downside is allocating these temp wrappers when fragments are involved,
but
> I don't think that's something to worry about now that SP caches the world.

Just to break the silence, I still do some work on this when time
permits.

Florin's suggestion got me curious why we couldn't allocate the entire
SVGImageForContainer on demand during paint time. I created

https://codereview.chromium.org/1339183003

as a limited POC to see what issues I would run into. I suspiciously
enough haven't uncovered anything fundamental besides the obvious cost
of allocating the SVGImageForContainer each paint.

If you have any insight why this would be a bad idea, I'd be happy to
hear it. There seems to be significant advantages untying
SVGImageForContainer from ImageResouce so comparing to the
disadvantages would be really interesting.

[fs, 2015-09-15 09:12:35]

On 2015/09/15 08:59:41, David Vest wrote:
> On 2015/09/10 13:37:40, f(malita) wrote:
> > On 2015/09/10 13:09:18, David Vest wrote:
> > > On 2015/09/10 07:45:47, fs wrote:
> > > > On 2015/09/09 23:55:41, f(malita) wrote:
> > > > > On 2015/09/09 14:22:54, David Vest wrote:
> > > > > > Since SVGImageForContainer is associated per layout object, this
> system
> > > will
> > > > > > break down if there is more than one fragment URL per layout object.
> > I.e.
> > > > > adding
> > > > > > a "background-image: url('some.svg#red');" to an img with
> > > src=some.svg#green
> > > > > > will make them compete for the URL in SVGImageForContainer. (In
> current
> > > > patch,
> > > > > > the background seems to win)
> > > > > > 
> > > > > > May have to go back to the drawing board on this one.
> > > > > 
> > > > > It occurred to me that all this plumbing is to make fragment info
> > persistent
> > > > and
> > > > > available at SVGImage::draw() time.  Rather than trying to do it at
> > resource
> > > > > load/cache time, would it be easier to figure fragment info at paint
> time?
> > > > > 
> > > > > Presumably the paint code knows exactly what decoration piece is being
> > > drawn,
> > > > so
> > > > > discriminating between different fragments of the same resource should
> not
> > > be
> > > > a
> > > > > problem.  Then there is no persistent mapping to worry about, we just
> need
> > > > some
> > > > > way to pass this transient info down to the SVGImage draw method.
> > > > > 
> > > > > Does this make any sense?
> > > > 
> > > > Yeah, that makes sense to me - it's still a bit of a bundle to pass
around
> > > > though.
> > > 
> > > Right. I've experimented with passing the URL through Image::draw here:
> > > 
> > > https://codereview.chromium.org/1334703002
> > > 
> > > Only fragments for SVG-in-img and SVG in backgrounds supported there.
> > > It's intrusive too, but in another way. And a somewhat cleaner and
> > > explicit design. Was it something like this you had in mind?
> > 
> > Yeah, that's what I was thinking of.
> > 
> > One idea to reduce intrusiveness: instead of making the KURL arg explicit on
> the
> > Image interface, we could introduce an (SVG)Image specialization
> > (SVGImageFragment?) that wraps an SVGImage + fragment ID and create these on
> the
> > fly, if/when needed.
> > 
> > Something like
> > 
> > static PassRefPtr<Image> imageForFragment(PassRefPtr<Image> image, const
KURL&
> > url)
> > {
> >     if (!image->isSVGImage() || !url.hasFragmentIdentifier())
> >         return image;
> > 
> >     return SVGImageFragment::create(image, url.fragmentIdentifier());
> > }
> > 
> > then maybe clients could call context->drawImage(imageForFragment(image,
> > resource_url), ...) unconditionally, and we don't have to touch the GC/Image
> > draw interface.
> > 
> > The downside is allocating these temp wrappers when fragments are involved,
> but
> > I don't think that's something to worry about now that SP caches the world.
> 
> Just to break the silence, I still do some work on this when time
> permits.
> 
> Florin's suggestion got me curious why we couldn't allocate the entire
> SVGImageForContainer on demand during paint time. I created
> 
> https://codereview.chromium.org/1339183003
> 
> as a limited POC to see what issues I would run into. I suspiciously
> enough haven't uncovered anything fundamental besides the obvious cost
> of allocating the SVGImageForContainer each paint.
> 
> If you have any insight why this would be a bad idea, I'd be happy to
> hear it. There seems to be significant advantages untying
> SVGImageForContainer from ImageResouce so comparing to the
> disadvantages would be really interesting.

That does look pretty nice! (Disregarding encapsulation for a bit.) I guess it
remains to be seen if the remaining cases will cause any major problems. I don't
really see any significant disadvantages with this approach, or that any
"expressive power" is lost (rather the opposite.)

[f(malita), 2015-09-17 09:48:15]

On 2015/09/15 09:12:35, fs wrote:
> On 2015/09/15 08:59:41, David Vest wrote:
> > On 2015/09/10 13:37:40, f(malita) wrote:
> > > On 2015/09/10 13:09:18, David Vest wrote:
> > > > On 2015/09/10 07:45:47, fs wrote:
> > > > > On 2015/09/09 23:55:41, f(malita) wrote:
> > > > > > On 2015/09/09 14:22:54, David Vest wrote:
> > > > > > > Since SVGImageForContainer is associated per layout object, this
> > system
> > > > will
> > > > > > > break down if there is more than one fragment URL per layout
object.
> > > I.e.
> > > > > > adding
> > > > > > > a "background-image: url('some.svg#red');" to an img with
> > > > src=some.svg#green
> > > > > > > will make them compete for the URL in SVGImageForContainer. (In
> > current
> > > > > patch,
> > > > > > > the background seems to win)
> > > > > > > 
> > > > > > > May have to go back to the drawing board on this one.
> > > > > > 
> > > > > > It occurred to me that all this plumbing is to make fragment info
> > > persistent
> > > > > and
> > > > > > available at SVGImage::draw() time.  Rather than trying to do it at
> > > resource
> > > > > > load/cache time, would it be easier to figure fragment info at paint
> > time?
> > > > > > 
> > > > > > Presumably the paint code knows exactly what decoration piece is
being
> > > > drawn,
> > > > > so
> > > > > > discriminating between different fragments of the same resource
should
> > not
> > > > be
> > > > > a
> > > > > > problem.  Then there is no persistent mapping to worry about, we
just
> > need
> > > > > some
> > > > > > way to pass this transient info down to the SVGImage draw method.
> > > > > > 
> > > > > > Does this make any sense?
> > > > > 
> > > > > Yeah, that makes sense to me - it's still a bit of a bundle to pass
> around
> > > > > though.
> > > > 
> > > > Right. I've experimented with passing the URL through Image::draw here:
> > > > 
> > > > https://codereview.chromium.org/1334703002
> > > > 
> > > > Only fragments for SVG-in-img and SVG in backgrounds supported there.
> > > > It's intrusive too, but in another way. And a somewhat cleaner and
> > > > explicit design. Was it something like this you had in mind?
> > > 
> > > Yeah, that's what I was thinking of.
> > > 
> > > One idea to reduce intrusiveness: instead of making the KURL arg explicit
on
> > the
> > > Image interface, we could introduce an (SVG)Image specialization
> > > (SVGImageFragment?) that wraps an SVGImage + fragment ID and create these
on
> > the
> > > fly, if/when needed.
> > > 
> > > Something like
> > > 
> > > static PassRefPtr<Image> imageForFragment(PassRefPtr<Image> image, const
> KURL&
> > > url)
> > > {
> > >     if (!image->isSVGImage() || !url.hasFragmentIdentifier())
> > >         return image;
> > > 
> > >     return SVGImageFragment::create(image, url.fragmentIdentifier());
> > > }
> > > 
> > > then maybe clients could call context->drawImage(imageForFragment(image,
> > > resource_url), ...) unconditionally, and we don't have to touch the
GC/Image
> > > draw interface.
> > > 
> > > The downside is allocating these temp wrappers when fragments are
involved,
> > but
> > > I don't think that's something to worry about now that SP caches the
world.
> > 
> > Just to break the silence, I still do some work on this when time
> > permits.
> > 
> > Florin's suggestion got me curious why we couldn't allocate the entire
> > SVGImageForContainer on demand during paint time. I created
> > 
> > https://codereview.chromium.org/1339183003
> > 
> > as a limited POC to see what issues I would run into. I suspiciously
> > enough haven't uncovered anything fundamental besides the obvious cost
> > of allocating the SVGImageForContainer each paint.
> > 
> > If you have any insight why this would be a bad idea, I'd be happy to
> > hear it. There seems to be significant advantages untying
> > SVGImageForContainer from ImageResouce so comparing to the
> > disadvantages would be really interesting.
> 
> That does look pretty nice! (Disregarding encapsulation for a bit.) I guess it
> remains to be seen if the remaining cases will cause any major problems. I
don't
> really see any significant disadvantages with this approach, or that any
> "expressive power" is lost (rather the opposite.)

+1, I think this is a good direction too.

[davve, 2015-09-17 13:21:30]

On 2015/09/17 09:48:15, f(malita) wrote:
> On 2015/09/15 09:12:35, fs wrote:
> > On 2015/09/15 08:59:41, David Vest wrote:
> > > On 2015/09/10 13:37:40, f(malita) wrote:
> > > > On 2015/09/10 13:09:18, David Vest wrote:
> > > > > On 2015/09/10 07:45:47, fs wrote:
> > > > > > On 2015/09/09 23:55:41, f(malita) wrote:
> > > > > > > On 2015/09/09 14:22:54, David Vest wrote:
> > > > > > > > Since SVGImageForContainer is associated per layout object, this
> > > system
> > > > > will
> > > > > > > > break down if there is more than one fragment URL per layout
> object.
> > > > I.e.
> > > > > > > adding
> > > > > > > > a "background-image: url('some.svg#red');" to an img with
> > > > > src=some.svg#green
> > > > > > > > will make them compete for the URL in SVGImageForContainer. (In
> > > current
> > > > > > patch,
> > > > > > > > the background seems to win)
> > > > > > > > 
> > > > > > > > May have to go back to the drawing board on this one.
> > > > > > > 
> > > > > > > It occurred to me that all this plumbing is to make fragment info
> > > > persistent
> > > > > > and
> > > > > > > available at SVGImage::draw() time.  Rather than trying to do it
at
> > > > resource
> > > > > > > load/cache time, would it be easier to figure fragment info at
paint
> > > time?
> > > > > > > 
> > > > > > > Presumably the paint code knows exactly what decoration piece is
> being
> > > > > drawn,
> > > > > > so
> > > > > > > discriminating between different fragments of the same resource
> should
> > > not
> > > > > be
> > > > > > a
> > > > > > > problem.  Then there is no persistent mapping to worry about, we
> just
> > > need
> > > > > > some
> > > > > > > way to pass this transient info down to the SVGImage draw method.
> > > > > > > 
> > > > > > > Does this make any sense?
> > > > > > 
> > > > > > Yeah, that makes sense to me - it's still a bit of a bundle to pass
> > around
> > > > > > though.
> > > > > 
> > > > > Right. I've experimented with passing the URL through Image::draw
here:
> > > > > 
> > > > > https://codereview.chromium.org/1334703002
> > > > > 
> > > > > Only fragments for SVG-in-img and SVG in backgrounds supported there.
> > > > > It's intrusive too, but in another way. And a somewhat cleaner and
> > > > > explicit design. Was it something like this you had in mind?
> > > > 
> > > > Yeah, that's what I was thinking of.
> > > > 
> > > > One idea to reduce intrusiveness: instead of making the KURL arg
explicit
> on
> > > the
> > > > Image interface, we could introduce an (SVG)Image specialization
> > > > (SVGImageFragment?) that wraps an SVGImage + fragment ID and create
these
> on
> > > the
> > > > fly, if/when needed.
> > > > 
> > > > Something like
> > > > 
> > > > static PassRefPtr<Image> imageForFragment(PassRefPtr<Image> image, const
> > KURL&
> > > > url)
> > > > {
> > > >     if (!image->isSVGImage() || !url.hasFragmentIdentifier())
> > > >         return image;
> > > > 
> > > >     return SVGImageFragment::create(image, url.fragmentIdentifier());
> > > > }
> > > > 
> > > > then maybe clients could call context->drawImage(imageForFragment(image,
> > > > resource_url), ...) unconditionally, and we don't have to touch the
> GC/Image
> > > > draw interface.
> > > > 
> > > > The downside is allocating these temp wrappers when fragments are
> involved,
> > > but
> > > > I don't think that's something to worry about now that SP caches the
> world.
> > > 
> > > Just to break the silence, I still do some work on this when time
> > > permits.
> > > 
> > > Florin's suggestion got me curious why we couldn't allocate the entire
> > > SVGImageForContainer on demand during paint time. I created
> > > 
> > > https://codereview.chromium.org/1339183003
> > > 
> > > as a limited POC to see what issues I would run into. I suspiciously
> > > enough haven't uncovered anything fundamental besides the obvious cost
> > > of allocating the SVGImageForContainer each paint.
> > > 
> > > If you have any insight why this would be a bad idea, I'd be happy to
> > > hear it. There seems to be significant advantages untying
> > > SVGImageForContainer from ImageResouce so comparing to the
> > > disadvantages would be really interesting.
> > 
> > That does look pretty nice! (Disregarding encapsulation for a bit.) I guess
it
> > remains to be seen if the remaining cases will cause any major problems. I
> don't
> > really see any significant disadvantages with this approach, or that any
> > "expressive power" is lost (rather the opposite.)
> 
> +1, I think this is a good direction too.

Thanks for the feedback. I'll update this CL when I have something a little more
complete.

[davve, 2015-09-25 15:18:51]

On 2015/09/17 13:21:30, David Vest wrote:

> Thanks for the feedback. I'll update this CL when I have something a little
more
> complete.

I still haven't found time to make all tests pass with the new strategy yet.
There seems to be some coordinate system confusion.

https://codereview.chromium.org/1367193003 is the POC post-merge.

I will be away for a week but will continue investigate when I'm back.

[fs, 2015-09-25 15:34:19]

On 2015/09/25 at 15:18:51, davve wrote:
> On 2015/09/17 13:21:30, David Vest wrote:
> 
> > Thanks for the feedback. I'll update this CL when I have something a little
more
> > complete.
> 
> I still haven't found time to make all tests pass with the new strategy yet.
There seems to be some coordinate system confusion.
> 
> https://codereview.chromium.org/1367193003 is the POC post-merge.

If it's the svg:image bit, I suspect that srcRect-construction should be delayed
until after the "wrapping" has happened. (Responding here to not pre-maturely
publish that review... =))

[davve, 2015-10-28 17:07:59]

Closing this one since this is probably not in the direction we want to go. I
currently experiment with other strategies in
https://codereview.chromium.org/1367193003/