[webkit-reviews] review granted: [Bug 163811] The SVG fragment identifier is not respected if it is a part of an HTTP URL : [Attachment 319278] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 163811: The SVG fragment identifier is not respected if it is a part of an
HTTP URL
https://bugs.webkit.org/show_bug.cgi?id=163811

Attachment 319278: Patch

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




--- Comment #51 from Darin Adler <darin at apple.com> ---
Comment on attachment 319278
  --> https://bugs.webkit.org/attachment.cgi?id=319278
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.