Bug 157435 – Add injected bundle SPI for getting favicon and touch icon URLs

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Anders Carlsson]

Add injected bundle SPI for getting favicon and touch icon URLs

[Anders Carlsson]

Created attachment 278289 [details]
Patch

[Darin Adler]

Comment on attachment 278289 [details]
Patch

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

> Source/WebCore/html/LinkIconCollector.cpp:51
> +    {

Incorrect indentation.

> Source/WebCore/html/LinkIconCollector.cpp:102
> +            unsigned size = linkElement.sizes().item(0).string().stripWhiteSpace().toUInt(&ok);

It’s almost never good to use stripWhiteSpace, since it’s the wrong set of whitespace characters. We have stripLeadingAndTrailingHTMLSpaces, and we have it on both String and StringView. Here, I think we’d want to use the StringView version—why allocate a string just to convert it to an integer?

> Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInFrame.h:49
> +// FIXME: These should be tagged nonnull.

Why does this have to be a FIXME? We don’t have the correct macro for that?

[Anders Carlsson]

(In reply to comment #2)
> > Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInFrame.h:49
> > +// FIXME: These should be tagged nonnull.
> 
> Why does this have to be a FIXME? We don’t have the correct macro for that?

We do have the correct macro, but adding an annotation to one method will cause the compiler to warn unless all methods have annotations (which they don't).

[Anders Carlsson]

(In reply to comment #2)
> Comment on attachment 278289 [details]
> Patch
> 
> > Source/WebCore/html/LinkIconCollector.cpp:102
> > +            unsigned size = linkElement.sizes().item(0).string().stripWhiteSpace().toUInt(&ok);
> 
> It’s almost never good to use stripWhiteSpace, since it’s the wrong set of
> whitespace characters. We have stripLeadingAndTrailingHTMLSpaces, and we
> have it on both String and StringView. Here, I think we’d want to use the
> StringView version—why allocate a string just to convert it to an integer?
> 

It doesn't look like there's a version of stripLeadingAndTrailingHTMLSpaces that works on a StringView - I'll make sure to fix one and adopt StringView in an upcoming patch.

[Anders Carlsson]

Committed r200591: <http://trac.webkit.org/changeset/200591>