[webkit-reviews] review denied: [Bug 229917] Correctly support fragment-only URLs in CSS images : [Attachment 444204] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Matt Woodrow <m_woodrow at apple.com>'s
request for review:
Bug 229917: Correctly support fragment-only URLs in CSS images
https://bugs.webkit.org/show_bug.cgi?id=229917

Attachment 444204: Patch

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




--- Comment #14 from Darin Adler <darin at apple.com> ---
Comment on attachment 444204
  --> https://bugs.webkit.org/attachment.cgi?id=444204
Patch

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

Thanks for taking this on! We really want to fix this. I don’t think this patch
is quite right yet.

review- because the use of String::find here is incorrect.

> Source/WebCore/css/CSSImageValue.cpp:92
> +    if (!m_location.resolvedURL.string().find('#') ||
m_location.resolvedURL.string().isEmpty())
> +	   return m_location.resolvedURL;

Calling find is an inefficient way to to this check. One efficient way to check
if something starts with a '#' is to call the startsWith function passing the
character '#'. But given that the URL class already parsed the URL there is an
even more efficient way to check using URL class functions. However, this call
site has a URL but the other call site just has a String.

Also, we don’t want to write this special case explicitly in two different
places. We probably want to make a named function to do the check.

In addition, we should encapsulate this as a smarter version of completeURL, I
think.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:3620
> +	       // Image URLs that start with an explicit #, or are empty should
be preserved, otherwise
> +	       // resolve the complete URL.
> +	       if (!string.find('#') || string.isEmpty()) {
> +		   return CSSImageValue::create(URL(URL(),
string.toAtomString().string()),
> +		       context.isContentOpaque ? LoadedFromOpaqueSource::Yes :
LoadedFromOpaqueSource::No);
> +	       } else {
> +		   return
CSSImageValue::create(context.completeURL(string.toAtomString().string()),
> +		       context.isContentOpaque ? LoadedFromOpaqueSource::Yes :
LoadedFromOpaqueSource::No);
> +	       }

Is this only for image URLs? What about other CSS URLs? I think this logic
should probably go in the CSSParserContext::completeURL function.