[Webkit-unassigned] [Bug 229917] Correctly support fragment-only URLs in CSS images

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=229917

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #444204|review?                     |review-
              Flags|                            |

--- 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211115/d0697b72/attachment.htm>