Bug 163811 – The SVG fragment identifier is not respected if it is a part of an HTTP URL

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Said Abou-Hallawa]

1. Open the attached test case.

Result: The SVG images are displayed as if the SVG fragments in the source URL.
Expected: All the images should display a single rectangle which is filled with solid color.

2. Copy the test case with the SVG locally.

Result: the SVG are displayed as expected.

[Said Abou-Hallawa]

Created attachment 292398 [details]
rgb-icons-1.svg

[Said Abou-Hallawa]

Created attachment 292399 [details]
rgb-icons-2.svg

[Said Abou-Hallawa]

Created attachment 292400 [details]
rgb-icons-3.svg

[Said Abou-Hallawa]

Created attachment 292402 [details]
test case

[Said Abou-Hallawa]

(In reply to comment #0)
> 1. Open the attached test case.
> 
> Result: The SVG images are displayed as if the SVG fragments in the source
> URL.
> Expected: All the images should display a single rectangle which is filled
> with solid color.
> 
> 2. Copy the test case with the SVG locally.
> 
> Result: the SVG are displayed as expected.

Correcting the reproducing steps:

1. Open the attached test case.
 
Result: The SVG images are displayed as if there were no SVG fragments in the source URL.
Expected: All the images should display a single rectangle which is filled with solid color.
 
2. Copy the test case with the SVG locally.

Result: the SVG are displayed as expected.

[Said Abou-Hallawa]

Created attachment 292450 [details]
Patch

[Build Bot]

Comment on attachment 292450 [details]
Patch

Attachment 292450 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2342231

New failing tests:
svg/css/svg-resource-fragment-identifier-img-src.html

[Build Bot]

Created attachment 292453 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 292450 [details]
Patch

Attachment 292450 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2342259

New failing tests:
svg/css/svg-resource-fragment-identifier-img-src.html

[Build Bot]

Created attachment 292458 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 292450 [details]
Patch

Attachment 292450 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2342258

New failing tests:
svg/css/svg-resource-fragment-identifier-img-src.html

[Build Bot]

Created attachment 292459 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 292450 [details]
Patch

Attachment 292450 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2342267

New failing tests:
svg/css/svg-resource-fragment-identifier-img-src.html

[Build Bot]

Created attachment 292461 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Darin Adler]

Comment on attachment 292450 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=292450&action=review

> Source/WebCore/rendering/RenderImage.cpp:560
> +        downcast<SVGImageForContainer>(*image).setURL(imageElement->src());

I don’t see a guarantee that imageElement won’t be null here.

[youenn fablet]

Probably a naive question but, performance-wise, would it not be better for some intermediate layer between cache and renderer to fix that issue so as to remove the "if" statement from this potentially hot code path?

[Said Abou-Hallawa]

Created attachment 292644 [details]
Patch

[Build Bot]

Comment on attachment 292644 [details]
Patch

Attachment 292644 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2360527

New failing tests:
svg/wicd/test-rightsizing-b.xhtml

[Build Bot]

Created attachment 292651 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 292765 [details]
Patch

[Said Abou-Hallawa]

Created attachment 293167 [details]
Patch

[Build Bot]

Comment on attachment 293167 [details]
Patch

Attachment 293167 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2393702

New failing tests:
svg/wicd/test-rightsizing-b.xhtml

[Build Bot]

Created attachment 293177 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 293683 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 292450 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=292450&action=review

>> Source/WebCore/rendering/RenderImage.cpp:560
>> +        downcast<SVGImageForContainer>(*image).setURL(imageElement->src());
> 
> I don’t see a guarantee that imageElement won’t be null here.

Fixed.

[Said Abou-Hallawa]

(In reply to comment #16)
> Probably a naive question but, performance-wise, would it not be better for
> some intermediate layer between cache and renderer to fix that issue so as
> to remove the "if" statement from this potentially hot code path?

Maybe I can create a new class derived from RenderImageResource to for the SVG RenderImage case say 'RenderSVGImageResource'. Then I can override RenderSVGImageResource::image() to do the URL fix. I will try that.

[Said Abou-Hallawa]

Created attachment 293727 [details]
Patch

[Said Abou-Hallawa]

Created attachment 293774 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #26)
> (In reply to comment #16)
> > Probably a naive question but, performance-wise, would it not be better for
> > some intermediate layer between cache and renderer to fix that issue so as
> > to remove the "if" statement from this potentially hot code path?
> 
> Maybe I can create a new class derived from RenderImageResource to for the
> SVG RenderImage case say 'RenderSVGImageResource'. Then I can override
> RenderSVGImageResource::image() to do the URL fix. I will try that.

No this can't be done because the RenderImageResource is created first and then setCachedImage() is called later. So we can't know in advance the type of the image to create a different class.

[youenn fablet]

(In reply to comment #29)
> (In reply to comment #26)
> > (In reply to comment #16)
> > > Probably a naive question but, performance-wise, would it not be better for
> > > some intermediate layer between cache and renderer to fix that issue so as
> > > to remove the "if" statement from this potentially hot code path?
> > 
> > Maybe I can create a new class derived from RenderImageResource to for the
> > SVG RenderImage case say 'RenderSVGImageResource'. Then I can override
> > RenderSVGImageResource::image() to do the URL fix. I will try that.
> 
> No this can't be done because the RenderImageResource is created first and
> then setCachedImage() is called later. So we can't know in advance the type
> of the image to create a different class.

How about something like:
- Make setCachedImage() take an URL parameter
- In setCachedImage, check if the cached image is SVG. Check also if the passed URL is different from the cached image URL
- If so, clone the cached image, update the clone URL and store the clone RenderImageResource.
- Otherwise store the cached image directly.

[Said Abou-Hallawa]

(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #26)
> > > (In reply to comment #16)
> > > > Probably a naive question but, performance-wise, would it not be better for
> > > > some intermediate layer between cache and renderer to fix that issue so as
> > > > to remove the "if" statement from this potentially hot code path?
> > > 
> > > Maybe I can create a new class derived from RenderImageResource to for the
> > > SVG RenderImage case say 'RenderSVGImageResource'. Then I can override
> > > RenderSVGImageResource::image() to do the URL fix. I will try that.
> > 
> > No this can't be done because the RenderImageResource is created first and
> > then setCachedImage() is called later. So we can't know in advance the type
> > of the image to create a different class.
> 
> How about something like:
> - Make setCachedImage() take an URL parameter
> - In setCachedImage, check if the cached image is SVG. Check also if the
> passed URL is different from the cached image URL
> - If so, clone the cached image, update the clone URL and store the clone
> RenderImageResource.

Did you mean "...and store the clone CachedImage." since you are referring to RenderImageResource::setCachedImage()? right?

But the problem is I do not know how to clone the CachedImage. And if there is an API to clone it, it has to clone the member CachedImage::m_image which is of type SVGImage in our case and this can take too much memory.

I agree that this modification is in a hot code path. But it just adds two conditions to be checked. I think what you are suggesting, even if it is doable, is not worthy the benefit.

> - Otherwise store the cached image directly.

[youenn fablet]

> But the problem is I do not know how to clone the CachedImage. And if there
> is an API to clone it, it has to clone the member CachedImage::m_image which
> is of type SVGImage in our case and this can take too much memory.

What I mean by cloning is creation of a new CachedResource, backed with the same m_image. m_image being a RefPtr, the cloning will just increase m_image counter.
CachedResource owning a ResourceRequest, this will require memory for it, that can be used to set the proper URL.

You can look at CachedImage::setBodyDataFrom for instance that does this kind of cloning.

> I agree that this modification is in a hot code path. But it just adds two
> conditions to be checked. I think what you are suggesting, even if it is
> doable, is not worthy the benefit.

I don't know enough of the constraints here.
Clearly, we would be trading memory (in some specific cases?) vs. speed (in the general case).

[Said Abou-Hallawa]

Created attachment 294513 [details]
Patch

[Said Abou-Hallawa]

*** Bug 163822 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

*** Bug 164386 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

Created attachment 294514 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #32)
> > But the problem is I do not know how to clone the CachedImage. And if there
> > is an API to clone it, it has to clone the member CachedImage::m_image which
> > is of type SVGImage in our case and this can take too much memory.
> 
> What I mean by cloning is creation of a new CachedResource, backed with the
> same m_image. m_image being a RefPtr, the cloning will just increase m_image
> counter.
> CachedResource owning a ResourceRequest, this will require memory for it,
> that can be used to set the proper URL.
> 
> You can look at CachedImage::setBodyDataFrom for instance that does this
> kind of cloning.
> 
> > I agree that this modification is in a hot code path. But it just adds two
> > conditions to be checked. I think what you are suggesting, even if it is
> > doable, is not worthy the benefit.
> 
> I don't know enough of the constraints here.
> Clearly, we would be trading memory (in some specific cases?) vs. speed (in
> the general case).

I think the latest patch fixes the issue in a cleaner way. It stores the full URL in the SVGImageForContainer which is different for every renderer. The previous patches were trying to fix the URL at the drawing time after it was stored without the fragmentIdentifier in the SVGImage.

[Said Abou-Hallawa]

Created attachment 294521 [details]
Patch

[youenn fablet]

(In reply to comment #37)
> (In reply to comment #32)
> > > But the problem is I do not know how to clone the CachedImage. And if there
> > > is an API to clone it, it has to clone the member CachedImage::m_image which
> > > is of type SVGImage in our case and this can take too much memory.
> > 
> > What I mean by cloning is creation of a new CachedResource, backed with the
> > same m_image. m_image being a RefPtr, the cloning will just increase m_image
> > counter.
> > CachedResource owning a ResourceRequest, this will require memory for it,
> > that can be used to set the proper URL.
> > 
> > You can look at CachedImage::setBodyDataFrom for instance that does this
> > kind of cloning.
> > 
> > > I agree that this modification is in a hot code path. But it just adds two
> > > conditions to be checked. I think what you are suggesting, even if it is
> > > doable, is not worthy the benefit.
> > 
> > I don't know enough of the constraints here.
> > Clearly, we would be trading memory (in some specific cases?) vs. speed (in
> > the general case).
> 
> I think the latest patch fixes the issue in a cleaner way. It stores the
> full URL in the SVGImageForContainer which is different for every renderer.
> The previous patches were trying to fix the URL at the drawing time after it
> was stored without the fragmentIdentifier in the SVGImage.

Sounds promising!

[youenn fablet]

Comment on attachment 294521 [details]
Patch

I don't know the rendering code enough, but the approach makes sense to me.
Some nits below.

View in context: https://bugs.webkit.org/attachment.cgi?id=294521&action=review

> Source/WebCore/loader/cache/CachedImage.cpp:151
> +            revalidatedCachedImage.setContainerSizeForRenderer(request.key, std::get<0>(request.value), std::get<1>(request.value), std::get<2>(request.value));

Instead of doing that, can we change SizeAndZoom to a struct instead of a tuple?
This would improve readability.

> Source/WebCore/loader/cache/CachedImage.cpp:221
> +void CachedImage::setContainerSizeForRenderer(const CachedImageClient* renderer, const LayoutSize& containerSize, const URL& url, float containerZoom)

The name does not seem very explicit.
It should be setContainerSizeAndURLandZoomForRenderer which is not really good.
Maybe setRendererContainerInformation?

> Source/WebCore/loader/cache/CachedImage.cpp:228
> +        m_pendingContainerSizeRequests.set(renderer, SizeURLAndZoom(containerSize, url, containerZoom));

Would { containerSize, url, containerZoom } work?

Ditto for m_pendingContainerSizeRequests which is not very explicit.

> Source/WebCore/rendering/RenderImageResourceStyleImage.h:50
> +    void setContainerSizeForRenderer(const IntSize&, const URL&) override;

Use final instead probably.
Ditto for the other methods of the class.

> Source/WebCore/rendering/style/StyleCachedImage.cpp:82
> +    return URL();

Is this case possible? Should it be ASSERT_NOT_REACHED()?

> Source/WebCore/svg/graphics/SVGImage.cpp:65
>      : Image(&observer)

It could be inlined in the header then?

> Source/WebCore/svg/graphics/SVGImage.cpp:190
> +        view->scrollToFragment(url);

Should we write it like:
if (!url.isEmpty()) {
    auto* view = frameView();
   ASSERT...
  ...
}
It would be even better if we would not need to add the ASSERT() and directly having a frameView reference.

> Source/WebCore/svg/graphics/SVGImage.h:47
>      {

Often these methods are put in one line.

> Source/WebCore/svg/graphics/SVGImage.h:95
> +    SVGImage(ImageObserver&);

Do we want to add "explicit" here even if it is a private constructor?

> Source/WebCore/svg/graphics/SVGImageForContainer.h:40
> +    static Ref<SVGImageForContainer> create(SVGImage* image, const FloatSize& containerSize, const URL& url, float zoom)

Might be better to take a URL&& ?

[Said Abou-Hallawa]

Created attachment 294783 [details]
Patch

[Said Abou-Hallawa]

Created attachment 319198 [details]
Patch

[youenn fablet]

Comment on attachment 319198 [details]
Patch

I like the direction of storing the URL in the container.
Some feedback below.

View in context: https://bugs.webkit.org/attachment.cgi?id=319198&action=review

> Source/WebCore/css/CSSImageSetValue.cpp:89
> +URL CSSImageSetValue::url(Document& document)

Can we share this CSSImageSetValue for different documents?
If not, can we try not passing the Document& here, maybe by storing the url or completing all image URLs of the set?
Also can m_deviceScaleFactor and m_imagesInSet change over time so that we load a best fit image URL that will be different from the one returned by this methid?

For your purpose though, are you interested only by the fragment identifier?
If so, maybe you do not even care to complete the URLs?

> Source/WebCore/css/CSSImageSetValue.cpp:107
> +        CachedResourceRequest request(ResourceRequest(url(*document)), options);

Can be written request(url(*document), options);

> Source/WebCore/rendering/RenderImage.cpp:291
> +            url = document().completeURL(imageElement->imageSourceURL());

imageElement probably knows its document and could compute the full completeURL.

> Source/WebCore/rendering/svg/RenderSVGImage.cpp:78
> +    URL url = document().completeURL(imageElement().imageSourceURL());

Is imageSourceURL method already taking care of completing the URL.
Do we need to do it here also? Or maybe it is better to do it here and not within imageSourceURL?

> Source/WebCore/svg/graphics/SVGImageForContainer.h:70
> +        , m_sourceURL(sourceURL)

Should probably pass a URL&&

[Build Bot]

Comment on attachment 319198 [details]
Patch

Attachment 319198 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4399161

New failing tests:
http/tests/svg/svg-fragment-background.html

[Build Bot]

Created attachment 319210 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Darin Adler]

Comment on attachment 319198 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319198&action=review

Did not do a full review, but have a few style comments.

> Source/WebCore/css/CSSCursorImageValue.h:55
> +    URL url() const { return m_originalURL; }

Typically we would use the type const URL& here instead of URL to slightly cut down on reference count churn.

> Source/WebCore/css/CSSImageSetValue.h:61
> +    URL url(Document&);

The function above uses const Document& and this uses Document&. Any good reason for the difference?

> Source/WebCore/loader/cache/CachedImage.cpp:252
> +        ContainerContext containerContext = { containerSize, containerZoom, sourceURL };

Should not need an "=" here.

> Source/WebCore/loader/cache/CachedImage.h:159
> +    typedef HashMap<const CachedImageClient*, ContainerContext> ContainerContextRequests;

We should use "using" in new code rather than "typedef".

> Source/WebCore/rendering/style/StyleCachedImage.cpp:86
> +    if (is<CSSImageValue>(m_cssValue)) {
> +        auto& imageValue = downcast<CSSImageValue>(m_cssValue.get());
> +        return imageValue.url();
> +    }
> +
> +    if (is<CSSImageSetValue>(m_cssValue)) {
> +        auto& imageSetValue = downcast<CSSImageSetValue>(m_cssValue.get());
> +        return imageSetValue.url(document);
> +    }
> +
> +    if (is<CSSCursorImageValue>(m_cssValue.get())) {
> +        auto& cursorValue = downcast<CSSCursorImageValue>(m_cssValue.get());
> +        return cursorValue.url();
> +    }

No need for these local variables. These return statements should all be one liners.

[Said Abou-Hallawa]

Created attachment 319266 [details]
Patch

[Said Abou-Hallawa]

(In reply to Build Bot from comment #44)
> Comment on attachment 319198 [details]
> Patch
> 
> Attachment 319198 [details] did not pass mac-debug-ews (mac):
> Output: http://webkit-queues.webkit.org/results/4399161
> 
> New failing tests:
> http/tests/svg/svg-fragment-background.html

This assertion failure was happening because there was a missing tag closing bracket in "</script" in this test. I fixed the test and I filed https://bugs.webkit.org/show_bug.cgi?id=176061.

[Said Abou-Hallawa]

Created attachment 319278 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 319198 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319198&action=review

>> Source/WebCore/css/CSSCursorImageValue.h:55
>> +    URL url() const { return m_originalURL; }
> 
> Typically we would use the type const URL& here instead of URL to slightly cut down on reference count churn.

Fixed.

>> Source/WebCore/css/CSSImageSetValue.cpp:89
>> +URL CSSImageSetValue::url(Document& document)
> 
> Can we share this CSSImageSetValue for different documents?
> If not, can we try not passing the Document& here, maybe by storing the url or completing all image URLs of the set?
> Also can m_deviceScaleFactor and m_imagesInSet change over time so that we load a best fit image URL that will be different from the one returned by this methid?
> 
> For your purpose though, are you interested only by the fragment identifier?
> If so, maybe you do not even care to complete the URLs?

I realized that all the urls of CSSCursorImageValue, CSSImageSetValue and CSSImageValue are all incomplete. I moved the Document argument out of this function and made StyleCachedImage::setContainerSizeForRenderer() calls document.completeURL() for the returned incomplete url.

>> Source/WebCore/css/CSSImageSetValue.h:61
>> +    URL url(Document&);
> 
> The function above uses const Document& and this uses Document&. Any good reason for the difference?

You are right. This function should retrain an incomplete URL. The caller can complete the URL if it wants to. The Document argument was removed from this function.

> Source/WebCore/loader/cache/CachedImage.cpp:253
> +        ContainerContext containerContext = { containerSize, containerZoom, sourceURL };
> +        m_pendingContainerContextRequests.set(client, containerContext);

I replaced these tow lines by this line:

    m_pendingContainerContextRequests.set(client, ContainerContext { containerSize, containerZoom, imageURL });

>> Source/WebCore/loader/cache/CachedImage.h:159
>> +    typedef HashMap<const CachedImageClient*, ContainerContext> ContainerContextRequests;
> 
> We should use "using" in new code rather than "typedef".

Fixed.

>> Source/WebCore/rendering/RenderImage.cpp:291
>> +            url = document().completeURL(imageElement->imageSourceURL());
> 
> imageElement probably knows its document and could compute the full completeURL.

I do not think we should that. The imageElement should return the incomplete URL. The caller can compete the URL if it wants to. All the code I checked does that.

>> Source/WebCore/rendering/style/StyleCachedImage.cpp:86
>> +    }
> 
> No need for these local variables. These return statements should all be one liners.

Fixed.

[Darin Adler]

Comment on attachment 319278 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319278&action=review

> Source/WebCore/rendering/RenderImage.cpp:291
> +            imageSourceURL = document().completeURL(imageElement->imageSourceURL());

A couple questions:

1) Why is it valuable to call completeURL here? If we are going to simply extract the fragment, then completeURL is a wasted operation; it has no effect on the fragment. I suggest naming the function in a way that makes it clear that it’s a URL “for the fragment only”. Or possibly even change the scrolling code so it works with a fragment string, not just with a URL.

2) Does this handle the empty string correctly? Not important if we don’t need completeURL at all.

3) Does this handle trailing HTML spaces correctly? We should make sure we have test cases that check that behavior. Is a trailing space part of the fragment identifier or not? Some call sites strip leading and trailing HTML spaces before calling completeURL. So what is appropriate here? Has this already been done?

> Source/WebCore/rendering/style/StyleCachedImage.cpp:80
> +        return downcast<CSSCursorImageValue>(m_cssValue.get()).imageURL();

This converts a URL to a String implicitly; surprised it compiles without explicitly calling string() on the URL. Also seems unfortunate to throw away the parsing of the already parsed URL and then re-parse it later. Not typically a good design pattern. Unclear why this class uses URL and the others use String for these similar CSS concepts.

> Source/WebCore/rendering/style/StyleCachedImage.cpp:187
> +    ASSERT(renderer);

This assertion seems to indicate that this function should take a reference to a renderer rather than a pointer. And since you renamed it you are touching every call site so could change it.

> Source/WebCore/rendering/style/StyleCachedImage.cpp:188
> +    m_cachedImage->setContainerContextForClient(renderer, LayoutSize(containerSize), containerZoom, renderer->document().completeURL(imageURL()));

Same questions here about completeURL.

> Source/WebCore/rendering/svg/RenderSVGImage.cpp:78
> +    URL imageSourceURL = document().completeURL(imageElement().imageSourceURL());

Same questions here about completeURL.

> Source/WebCore/svg/graphics/SVGImage.cpp:195
> +    if (!imageURL.isEmpty()) {
> +        FrameView* view = frameView();
> +        ASSERT(view);
> +        view->scrollToFragment(imageURL);
> +    }

I am not sure of the value of the check for the empty string. Is it important that we handle the empty string differently from any other URL that just doesn’t happen to have a fragment identifier?

The assertion that checks frameView() for null doesn’t seem quite right. I would suggest asserting this earlier in this function or not at all; especially unpleasant to have it change the if statement body into multiple lines of code. We normally don’t have to assert that pointers are non-null, since null pointer crashes are typically easy to understand even without assertions, so we do need to think about why we are writing the assertion. Is it to make the requirement clear? If so, then the assertion earlier in the function may be better.

I’d like to see this just be a single line:

    frameView()->scrollToFragment(initialFragmentURL);

My different name “initialFragmentURL” is intended to convey that this needs to be a URL with the fragment identifier telling us what to scroll to initially, but does not otherwise need to be the correct URL for the image. Which I think is a change we should consider. Maybe there is an even better name.

> Source/WebCore/svg/graphics/SVGImage.h:95
> +    ImageDrawResult drawForContainer(GraphicsContext&, const FloatSize containerSize, float containerZoom, const URL&, const FloatRect& dstRect, const FloatRect& srcRect, CompositeOperator, BlendMode);
> +    void drawPatternForContainer(GraphicsContext&, const FloatSize& containerSize, float containerZoom, const URL&, const FloatRect& srcRect, const AffineTransform&, const FloatPoint& phase, const FloatSize& spacing,

Not entirely sure that the meaning of the URL argument is clear without an argument name.

> Source/WebCore/svg/graphics/SVGImageCache.cpp:31
> +#include "URL.h"

This include should not be needed. Typically you don’t need to include a class just to pass a const X& to an argument of type const X&, so I am surprised that is needed here.

> Source/WebCore/svg/graphics/SVGImageCache.cpp:55
>      ASSERT(client);

Seems to indicate that the client argument should be a reference, not a pointer.

> Source/WebCore/svg/graphics/SVGImageForContainer.h:79
> +    const URL m_imageURL;

If this is only used for the fragment identifier we could consider a different name.

[Said Abou-Hallawa]

Created attachment 319325 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 319278 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319278&action=review

>> Source/WebCore/rendering/RenderImage.cpp:291
>> +            imageSourceURL = document().completeURL(imageElement->imageSourceURL());
> 
> A couple questions:
> 
> 1) Why is it valuable to call completeURL here? If we are going to simply extract the fragment, then completeURL is a wasted operation; it has no effect on the fragment. I suggest naming the function in a way that makes it clear that it’s a URL “for the fragment only”. Or possibly even change the scrolling code so it works with a fragment string, not just with a URL.
> 
> 2) Does this handle the empty string correctly? Not important if we don’t need completeURL at all.
> 
> 3) Does this handle trailing HTML spaces correctly? We should make sure we have test cases that check that behavior. Is a trailing space part of the fragment identifier or not? Some call sites strip leading and trailing HTML spaces before calling completeURL. So what is appropriate here? Has this already been done?

1) We do not need to have a completeURL to extract the fragmentIdentifier. The only reason I used completeURL() is to get a URL from a String so we can eventually pass it to FrameView::scrollToFragment().
2) Yes it does because the case we care about is where the URL hasFragmentIdentifier(). In the case of an empty URL or empty imageSourceURL(), hasFragmentIdentifier() will return false.
3) The URLParser strips the trailing spaces from the URL. For example "http://someDomain.com/image.svg#identifier         " is equivalent to "http://someDomain.com/image.svg#identifier". But the HTML/SVG parser does not strip the trailing spaces from the element's identifier. For example <img id="someId    "> is different from  <img id="someId">.

This test case

    <img src="http://someDomain.com/image.svg#identifier         " id="someId    ">
    <script>
        var element = document.querySelector("img");
        console.log("'" + element.id + "'");
        console.log("'" + element.src + "'");
    </script>

Will log the following output:
    'someId    '
    'http://someDomain.com/image.svg#identifier'


I think this difference exists because we use two different parsers whose rules are different.

>> Source/WebCore/rendering/style/StyleCachedImage.cpp:80
>> +        return downcast<CSSCursorImageValue>(m_cssValue.get()).imageURL();
> 
> This converts a URL to a String implicitly; surprised it compiles without explicitly calling string() on the URL. Also seems unfortunate to throw away the parsing of the already parsed URL and then re-parse it later. Not typically a good design pattern. Unclear why this class uses URL and the others use String for these similar CSS concepts.

I made this function return a URL instead of returning a String. This eliminates the need to re-parse the String to a URL later.

>> Source/WebCore/rendering/style/StyleCachedImage.cpp:187
>> +    ASSERT(renderer);
> 
> This assertion seems to indicate that this function should take a reference to a renderer rather than a pointer. And since you renamed it you are touching every call site so could change it.

Done.

>> Source/WebCore/rendering/style/StyleCachedImage.cpp:188
>> +    m_cachedImage->setContainerContextForClient(renderer, LayoutSize(containerSize), containerZoom, renderer->document().completeURL(imageURL()));
> 
> Same questions here about completeURL.

The call to completeURL() was removed since imageURL() returns a URL now.

>> Source/WebCore/rendering/svg/RenderSVGImage.cpp:78
>> +    URL imageSourceURL = document().completeURL(imageElement().imageSourceURL());
> 
> Same questions here about completeURL.

I could not remove the call to this completeURL() because imageSourceURL() returns an AtomicString.

>> Source/WebCore/svg/graphics/SVGImage.cpp:195
>> +    }
> 
> I am not sure of the value of the check for the empty string. Is it important that we handle the empty string differently from any other URL that just doesn’t happen to have a fragment identifier?
> 
> The assertion that checks frameView() for null doesn’t seem quite right. I would suggest asserting this earlier in this function or not at all; especially unpleasant to have it change the if statement body into multiple lines of code. We normally don’t have to assert that pointers are non-null, since null pointer crashes are typically easy to understand even without assertions, so we do need to think about why we are writing the assertion. Is it to make the requirement clear? If so, then the assertion earlier in the function may be better.
> 
> I’d like to see this just be a single line:
> 
>     frameView()->scrollToFragment(initialFragmentURL);
> 
> My different name “initialFragmentURL” is intended to convey that this needs to be a URL with the fragment identifier telling us what to scroll to initially, but does not otherwise need to be the correct URL for the image. Which I think is a change we should consider. Maybe there is an even better name.

Done.

>> Source/WebCore/svg/graphics/SVGImage.h:95
>> +    void drawPatternForContainer(GraphicsContext&, const FloatSize& containerSize, float containerZoom, const URL&, const FloatRect& srcRect, const AffineTransform&, const FloatPoint& phase, const FloatSize& spacing,
> 
> Not entirely sure that the meaning of the URL argument is clear without an argument name.

The arguments' names were added.

>> Source/WebCore/svg/graphics/SVGImageCache.cpp:31
>> +#include "URL.h"
> 
> This include should not be needed. Typically you don’t need to include a class just to pass a const X& to an argument of type const X&, so I am surprised that is needed here.

The include statement was removed.

>> Source/WebCore/svg/graphics/SVGImageCache.cpp:55
>>      ASSERT(client);
> 
> Seems to indicate that the client argument should be a reference, not a pointer.

Done.

>> Source/WebCore/svg/graphics/SVGImageForContainer.h:79
>> +    const URL m_imageURL;
> 
> If this is only used for the fragment identifier we could consider a different name.

The names was changed to m_initialFragmentURL.

[WebKit Commit Bot]

Comment on attachment 319325 [details]
Patch

Clearing flags on attachment: 319325

Committed r221377: <http://trac.webkit.org/changeset/221377>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34165100>

[Darin Adler]

I think it's worth revisiting this code to tighten this up a bit further, but no rush to do so immediately, except I think we landed this with insufficient test coverage, not testing handling of no fragment identifier vs. fragment identifier of empty string, and trailing spaces after the fragment identifier.

(In reply to Said Abou-Hallawa from comment #53)
> The only reason I used completeURL() is to get a URL from a String so we can
> eventually pass it to FrameView::scrollToFragment().

We should *not* use completeURL for this, then. Just "URL { URL { }, string }" will do; no need to involve a Document and resolve against a base URL just to get a fragment identifier. I think we need to change FrameView::scrollToFragment to take a fragment identifier, not an entire URL. Unless that makes the call sites really hard to read.

> 2) Yes it does because the case we care about is where the URL
> hasFragmentIdentifier(). In the case of an empty URL or empty
> imageSourceURL(), hasFragmentIdentifier() will return false.

Representing a fragment identifier that is optional can be accomplished with WTF::String (using either null string or empty string to represent lack of a fragment identifier), or with std::optional<WTF::String>. We don’t need to pass a URL just to have an optional fragment identifier.

> 3) The URLParser strips the trailing spaces from the URL. For example
> "http://someDomain.com/image.svg#identifier         " is equivalent to
> "http://someDomain.com/image.svg#identifier". But the HTML/SVG parser does
> not strip the trailing spaces from the element's identifier. For example
> <img id="someId    "> is different from  <img id="someId">.

That's fine analysis of what our code does, but that's really not the point. We need to figure out what correct behavior is and add test cases checking for that behavior. Doesn't matter exactly how our code is structured, but it does matter what the web-visible behavior is. This is a regrettable loose end and we should add these tests.

> >> Source/WebCore/rendering/svg/RenderSVGImage.cpp:78
> >> +    URL imageSourceURL = document().completeURL(imageElement().imageSourceURL());
> > 
> > Same questions here about completeURL.
> 
> I could not remove the call to this completeURL() because imageSourceURL()
> returns an AtomicString.

We should not call completeURL on a string if our eventual goal is simply to get the fragment identifier. Just as I mentioned above.

The completeURL function should be reserved for cases where we need to resolve a URL against the base URL for a document. It should not be used just to convert a string to a URL so we can parse out its fragment identifier.

[Said Abou-Hallawa]

(In reply to Darin Adler from comment #57)
> I think it's worth revisiting this code to tighten this up a bit further,
> but no rush to do so immediately, except I think we landed this with
> insufficient test coverage, not testing handling of no fragment identifier
> vs. fragment identifier of empty string, and trailing spaces after the
> fragment identifier.
> 

You are right. I found out we do not reset the scrolling anchor if the fragmentIdentifier is not provided in the URL or it does not exist in the SVGImage. I filed https://bugs.webkit.org/show_bug.cgi?id=176577 as a follow up for your comments and to fix this issue.

> (In reply to Said Abou-Hallawa from comment #53)
> > The only reason I used completeURL() is to get a URL from a String so we can
> > eventually pass it to FrameView::scrollToFragment().
> 
> We should *not* use completeURL for this, then. Just "URL { URL { }, string
> }" will do; no need to involve a Document and resolve against a base URL
> just to get a fragment identifier. I think we need to change
> FrameView::scrollToFragment to take a fragment identifier, not an entire
> URL. Unless that makes the call sites really hard to read.
> 

I could not use URL { URL { }, string }.fragmentIdentifier() to get the fragmentIdentifier of a URL like this "resources/rgb-icons-3.svg#green-icon". The reason is the following. URLParser::parse() starts its state with State::SchemeStart. The first character 'r' changes the state to State::Scheme. The first '/' changes the state to State::NoScheme. The next character after '/' hits the following code:

        case State::NoScheme:
            LOG_STATE("NoScheme");
            if (!base.isValid() || (base.m_cannotBeABaseURL && *c != '#')) {
                failure();
                return;
            }

Since (base.m_cannotBeABaseURL && *c != '#') is true, the parsing fails and no fragmentIdentifier can returned from URL { URL { }, "resources/rgb-icons-3.svg#green-icon" }.

> > 2) Yes it does because the case we care about is where the URL
> > hasFragmentIdentifier(). In the case of an empty URL or empty
> > imageSourceURL(), hasFragmentIdentifier() will return false.
> 
> Representing a fragment identifier that is optional can be accomplished with
> WTF::String (using either null string or empty string to represent lack of a
> fragment identifier), or with std::optional<WTF::String>. We don’t need to
> pass a URL just to have an optional fragment identifier.
> 

Done in the patch which is attached to https://bugs.webkit.org/show_bug.cgi?id=176577.

> > 3) The URLParser strips the trailing spaces from the URL. For example
> > "http://someDomain.com/image.svg#identifier         " is equivalent to
> > "http://someDomain.com/image.svg#identifier". But the HTML/SVG parser does
> > not strip the trailing spaces from the element's identifier. For example
> > <img id="someId    "> is different from  <img id="someId">.
> 
> That's fine analysis of what our code does, but that's really not the point.
> We need to figure out what correct behavior is and add test cases checking
> for that behavior. Doesn't matter exactly how our code is structured, but it
> does matter what the web-visible behavior is. This is a regrettable loose
> end and we should add these tests.
> 

Currently the behavior is incorrect when the fragmentIdentifier is missing or does not exist in the SVGImage. A test case is added to https://bugs.webkit.org/show_bug.cgi?id=176577 and the code is fixed to make it work properly when the same SVGImage is referenced multiple times with valid and invalid fragmentIdentifiers.

> > >> Source/WebCore/rendering/svg/RenderSVGImage.cpp:78
> > >> +    URL imageSourceURL = document().completeURL(imageElement().imageSourceURL());
> > > 
> > > Same questions here about completeURL.
> > 
> > I could not remove the call to this completeURL() because imageSourceURL()
> > returns an AtomicString.
> 
> We should not call completeURL on a string if our eventual goal is simply to
> get the fragment identifier. Just as I mentioned above.
> 
> The completeURL function should be reserved for cases where we need to
> resolve a URL against the base URL for a document. It should not be used
> just to convert a string to a URL so we can parse out its fragment
> identifier.

I still can't remove the call to completeURL() for the reason I explained above. We can either move parsing the fragmentIdentifier outside the URLParser so it can be used by the URLParser if the base is valid but we can make calls like this outside URLParser:

String fragmentIdentifier = URLFragmentIdentifier(imageElement().imageSourceURL());

Or we can change the URLParser such that, it continues parsing the string without having to check whether the base.m_cannotBeABaseURL or not. I really do not understand why parsing the URL stops after the first '/' if the base URL is invalid. Why can't we continue even if the base URL is empty? Or why didn't we stop from the beginning? We could have checked whether the base URL is invalid before looping through the input string.

[Darin Adler]

Alex Christensen says I am wrong and says we can’t extract the fragment identifier without completing the URL, given how the URL specifications are written. So unless we decide to change that, and I guess we should not, I take back my suggestion to undo all that part. We probably should *not* change these functions to just take the fragment identifier. We can pass the entire URL like today.

[Said Abou-Hallawa]

(In reply to Darin Adler from comment #59)
> Alex Christensen says I am wrong and says we can’t extract the fragment
> identifier without completing the URL, given how the URL specifications are
> written. So unless we decide to change that, and I guess we should not, I
> take back my suggestion to undo all that part. We probably should *not*
> change these functions to just take the fragment identifier. We can pass the
> entire URL like today.

I reverted the URL -> fragmentIdentifier replacement from https://bugs.webkit.org/show_bug.cgi?id=176577.