Bug 95979 – Favicon is not loaded when its link element's href attribute is modified in JS

• Component: DOM
• Status: RESOLVED INVALID

[Pierre Rossi]

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

[Pierre Rossi]

Created attachment 162495 [details]
Patch

[Brady Eidson]

Comment on attachment 162495 [details]
Patch

We are *not* going to be adding this to all platforms without having a switch to opt in.

We (Apple) intentionally *do not* want the ability to script favicons.

[Pierre Rossi]

(In reply to comment #2)
> (From update of attachment 162495 [details])
> We are *not* going to be adding this to all platforms without having a switch to opt in.
> 
> We (Apple) intentionally *do not* want the ability to script favicons.

No worries. I see, so the original idea in the previous patches could work for WebKit1, but that doesn't help in the WK2 case. Not sure it's worth adding an ENABLE flag just for that...

[Pierre Rossi]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 162495 [details] [details])
> > We are *not* going to be adding this to all platforms without having a switch to opt in.
> > 
> > We (Apple) intentionally *do not* want the ability to script favicons.
> 
> No worries. I see, so the original idea in the previous patches could work for WebKit1, but that doesn't help in the WK2 case. Not sure it's worth adding an ENABLE flag just for that...

And I meant to post that in the other one... :D

[Brady Eidson]

As for changing WebCore behavior like this, that could easily be protected behind a WebCore::Setting that defaults to disabled.

[Pierre Rossi]

Created attachment 162585 [details]
Patch

[Pierre Rossi]

A bit intrusive maybe ? I suspect the GTK folks might have an interest in this for epiphany though.

[Brady Eidson]

Comment on attachment 162585 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:4964
> +            if (settings()->loadsSiteIconsOnLinkHrefAttributeChanges())

I'm not super thrilled about this name.  There might be other "default" icon URLs that a given platform assumes, or there might be other ways to dynamically change the icon URL, such as through an API layer.

What about settings()->allowsDynamicIconURLs(), or something along those lines?

> Source/WebCore/page/Settings.cpp:159
> +#if PLATFORM(QT)
> +    , m_loadsSiteIconsOnLinkHrefAttributeChanges(true)
> +#else
> +    , m_loadsSiteIconsOnLinkHrefAttributeChanges(false)
> +#endif

In general the WebCore::Settings class is managed by each platform's WebKit and its values should be managed from there.

I think using that more common idiom here would be to have the value be unconditionally false in the Settings initializer list in WebCore then set it to true whenever WebKitQT updates Settings.

> Source/WebCore/page/Settings.h:134
> +        // This settings affects whether to re-load site icons when the link's href attribute is modified.
> +        void setLoadsSiteIconsOnLinkHrefAttributeChanges(bool);
> +        bool loadsSiteIconsOnLinkHrefAttributeChanges() const { return m_loadsSiteIconsOnLinkHrefAttributeChanges; }

With a name change similar to the one I suggested above this comment wouldn't be necessary.

[Pierre Rossi]

Created attachment 162781 [details]
Patch

[Build Bot]

Comment on attachment 162781 [details]
Patch

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

[Pierre Rossi]

Created attachment 162822 [details]
Patch

[WebKit Review Bot]

Comment on attachment 162822 [details]
Patch

Attachment 162822 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13789271

New failing tests:
http/tests/misc/favicon-load-dynamically.html

[Pierre Rossi]

Created attachment 163090 [details]
Patch

[Brady Eidson]

Comment on attachment 163090 [details]
Patch

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

I'm curious how this test actually tests the new code?  It appears to not twiddle the "allowsDynamicIconURIs" preference at all, and there's no test results.

> Source/WebKit2/UIProcess/API/C/WKPreferences.h:66
> +// Defaults to false
> +WK_EXPORT void WKPreferencesSetAllowsDynamicIconURIs(WKPreferencesRef preferencesRef, bool allowsDynamicIconURIs);
> +WK_EXPORT bool WKPreferencesGetAllowsDynamicIconURIs(WKPreferencesRef preferencesRef);
> +

I don't think we'd want to expose this API.  Could we put this in WKPreferencesPrivate.h?

[Pierre Rossi]

(In reply to comment #14)
> (From update of attachment 163090 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163090&action=review
> 

Thanks for reviewing this over and over again, makes me feel sorry I keep botching it up!

> I'm curious how this test actually tests the new code?  It appears to not twiddle the "allowsDynamicIconURIs" preference at all, and there's no test results.
> 

After the discussion on not making this the default on every port, I figured this would mean having port-specific expectations in LayoutTests/platform, was that the wrong assumtion to make in the first place.

> > Source/WebKit2/UIProcess/API/C/WKPreferences.h:66
> > +// Defaults to false
> > +WK_EXPORT void WKPreferencesSetAllowsDynamicIconURIs(WKPreferencesRef preferencesRef, bool allowsDynamicIconURIs);
> > +WK_EXPORT bool WKPreferencesGetAllowsDynamicIconURIs(WKPreferencesRef preferencesRef);
> > +
> 
> I don't think we'd want to expose this API.  Could we put this in WKPreferencesPrivate.h?

That seems fair enough, will do.

[Brady Eidson]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 163090 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=163090&action=review
> > 
> 
> Thanks for reviewing this over and over again, makes me feel sorry I keep botching it up!

You're not botching it up, we just keep finding further refinements  :)

> 
> > I'm curious how this test actually tests the new code?  It appears to not twiddle the "allowsDynamicIconURIs" preference at all, and there's no test results.
> > 
> 
> After the discussion on not making this the default on every port, I figured this would mean having port-specific expectations in LayoutTests/platform, was that the wrong assumtion to make in the first place.

Upon further consideration I realized that if QT's default preferences automatically override the "allowsDynamicIconURIs" preference then the test doesn't have to do that itself.

But there's no code for QT to do that.

The bare minimum addition for a new regression test is the test itself - which your patch includes - and the baseline expected results - which your patch does not include.

If you think it likely that other ports will want to adopt this in the future, you have to add it to the skipped list/test expectations for each port besides qt.

If it's unlikely other ports will want to adopt this, then you could add it directly to LayoutTests/platform/qt so only qt will ever run it.

As this patch is, checking it in would cause a failure on *every* port since there are no baseline expectations included!

[Anders Carlsson]

Comment on attachment 163090 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Shazron Abdullah]

With Safari 12 supporting favicon display in tabs, is it still Apple's intent not to support the ability to script favicons? 

Test: http://lab.ejci.net/favico.js/

For examples where it is useful:
1. Gmail.com pinned - the favicon has a number overlay to show unread messages
2. Slack.com pinned - the favicon has a red dot overlay to show unread messages
3. Travis-CI.com build status page - the favicon itself changes color to show build success / failed (similarly for AppVeyor.com CI)

Tested with Release 64 (Safari 12.1, WebKit 13607.1.3.3) on macOS High Sierra 10.13.6

[Brady Eidson]

This bug - and most bugs about WebKit icon database behavior - should be closed because WebKit no longer makes any losing decisions itself.