[webkit-reviews] review denied: [Bug 95979] Favicon is not loaded when its link element's href attribute is modified in JS : [Attachment 162585] Patch

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Pierre Rossi
<pierre.rossi at gmail.com>'s request for review:
Bug 95979: Favicon is not loaded when its link element's href attribute is
modified in JS
https://bugs.webkit.org/show_bug.cgi?id=95979

Attachment 162585: Patch
https://bugs.webkit.org/attachment.cgi?id=162585&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
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.