Bug 174073 – Switch all WebKit API related to favicons from WebIconDatabase over to new WebCore::IconLoader mechanism

• Component: WebKit API
• Status: RESOLVED FIXED

[Brady Eidson]

Switch all WebKit API related to favicons from WebIconDatabase over to new WebCore::IconLoader mechanism

With the test I'm adding over in https://bugs.webkit.org/show_bug.cgi?id=174069, this patch will demonstrate no regressions and - in fact - will show a *progression* in the KVO observation of the mainFrameIcon.

This will also prep us to cut out massive swaths of icondatabase related code.

[Brady Eidson]

Created attachment 314429 [details]
Patch

[Brady Eidson]

Created attachment 314433 [details]
Patch

[Build Bot]

Comment on attachment 314433 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 314434 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314433 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 314435 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314433 [details]
Patch

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

New failing tests:
workers/bomb.html

[Build Bot]

Created attachment 314436 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Brady Eidson]

*Layout test* failures... nice! Digging in.

[Brady Eidson]

Created attachment 314438 [details]
Patch

[Build Bot]

Comment on attachment 314438 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 314440 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314438 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 314442 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Brady Eidson]

Huh... the first round of ASSERTS I could easily reproduce locally... not this new round.

[Brady Eidson]

And I'm not seeing where EWS stashes the crash logs...

[Brady Eidson]

(In reply to Brady Eidson from comment #15)
> Huh... the first round of ASSERTS I could easily reproduce locally... not
> this new round.

Oh! These aren't crashes. These are failures. Lots... of failures...

[Brady Eidson]

(Suffice it to say, not seeing those locally either)

[Brady Eidson]

(Actually, never mind - can totally reproduce these failures!)

[Brady Eidson]

Created attachment 314446 [details]
Patch

[Brady Eidson]

HTTP tests now all load icons and they shouldn't necessarily. Double-checking what normally keeps them at bay.

[Build Bot]

Comment on attachment 314446 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 314450 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Brady Eidson]

Created attachment 314451 [details]
Patch

[Brady Eidson]

Down to two that are almost certainly actually caused by this change:
  http/tests/misc/favicon-loads-with-images-disabled.html [ Failure ]
  http/tests/security/contentSecurityPolicy/block-favicon.html [ Failure ]

[Brady Eidson]

(In reply to Brady Eidson from comment #25)
> Down to two that are almost certainly actually caused by this change:
>   http/tests/misc/favicon-loads-with-images-disabled.html [ Failure ]
>   http/tests/security/contentSecurityPolicy/block-favicon.html [ Failure ]

The former was a legit bug in the patch (now fixed)

The latter was a legit bug in the test - We actually *should* see the "resource load blocked" console message.

[Build Bot]

Comment on attachment 314451 [details]
Patch

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

New failing tests:
http/tests/security/contentSecurityPolicy/block-favicon.html
http/tests/misc/favicon-loads-with-images-disabled.html

[Build Bot]

Created attachment 314455 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Brady Eidson]

(In reply to Brady Eidson from comment #26)
> (In reply to Brady Eidson from comment #25)
> 
> The latter was a legit bug in the test - We actually *should* see the
> "resource load blocked" console message.

Double checking this - It seems logical to me we should see that the load was actually blocked by our machinery, but maybe it was supposed to be blocked even sooner...?

[Brady Eidson]

(In reply to Brady Eidson from comment #29)
> (In reply to Brady Eidson from comment #26)
> > (In reply to Brady Eidson from comment #25)
> > 
> > The latter was a legit bug in the test - We actually *should* see the
> > "resource load blocked" console message.
> 
> Double checking this - It seems logical to me we should see that the load
> was actually blocked by our machinery, but maybe it was supposed to be
> blocked even sooner...?

Yup, I think this is accurate. The test as-is doesn't actually verify anything.

The icon is not being loaded in WK2, but the test is not actually verifying that!

Will file a followup bug for WK2

[Brady Eidson]

(In reply to Brady Eidson from comment #30)
> (In reply to Brady Eidson from comment #29)
> > (In reply to Brady Eidson from comment #26)
> > > (In reply to Brady Eidson from comment #25)
> > > 
> > > The latter was a legit bug in the test - We actually *should* see the
> > > "resource load blocked" console message.
> > 
> > Double checking this - It seems logical to me we should see that the load
> > was actually blocked by our machinery, but maybe it was supposed to be
> > blocked even sooner...?
> 
> Yup, I think this is accurate. The test as-is doesn't actually verify
> anything.
> 
> The icon is not being loaded in WK2, but the test is not actually verifying
> that!
> 
> Will file a followup bug for WK2

NM - Feel a lot better about the fact that it doesn't run in WK2 anyways.

[Brady Eidson]

Created attachment 314457 [details]
Patch

[Build Bot]

Comment on attachment 314457 [details]
Patch

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

New failing tests:
workers/bomb.html

[Build Bot]

Created attachment 314469 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Brady Eidson]

iOS build error in WebFrameLoaderClient.mm... but that error is not included in the scroll back. 

Nice. Thanks.

[Brady Eidson]

Created attachment 314481 [details]
Patch

[Andy Estes]

Comment on attachment 314481 [details]
Patch

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

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2283
> +    auto* frame = core(m_webFrame.get());
> +    DocumentLoader* documentLoader = frame->loader().documentLoader();
> +    ASSERT(documentLoader);

Can these be references instead of pointers?

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2289
> +    if (!frame->isMainFrame() || !documentLoader->url().protocolIsInHTTPFamily() || ![WebView _isIconLoadingEnabled] || disallowedDueToImageLoadSettings) {

I'd rewrite this to check disallowedDueToImageLoadSettings first.

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2299
> +    for (auto icon = icons.rbegin(); icon != icons.rend(); ++icon) {

It's not clear to me why we have to iterate backwards; I'm sure there's a reason.

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2311
> +    // No WebCore icon loading on iOS
> +    for (auto& icon : icons)
> +        documentLoader->didGetLoadDecisionForIcon(false, icon.second, 0);

Can iOS be handled like the other cases where icon loading is disallowed?

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1794
> +    for (auto& icon : icons)
> +        webPage->send(Messages::WebPageProxy::GetLoadDecisionForIcon(icon.first, CallbackID::fromInteger(icon.second)));

Can this be sent as one message containing all the icons instead of a message for each icon?

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.h:260
> +    void getLoadDecisionForIcons(const Vector<std::pair<WebCore::LinkIcon&, uint64_t>>&) final;

The vector of pairs could use a typedef.

[Brady Eidson]

(In reply to Andy Estes from comment #37)
> Comment on attachment 314481 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314481&action=review
> 
> > Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2283
> > +    auto* frame = core(m_webFrame.get());
> > +    DocumentLoader* documentLoader = frame->loader().documentLoader();
> > +    ASSERT(documentLoader);
> 
> Can these be references instead of pointers?

You mean at the level of rearchitecting WebFrameLoaderClient?
The WebFrame could be, the DocumentLoader probably not.

You mean in this patch? No.

> > Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2289
> > +    if (!frame->isMainFrame() || !documentLoader->url().protocolIsInHTTPFamily() || ![WebView _isIconLoadingEnabled] || disallowedDueToImageLoadSettings) {
> 
> I'd rewrite this to check disallowedDueToImageLoadSettings first.

Yah probably a good call.

> > Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2299
> > +    for (auto icon = icons.rbegin(); icon != icons.rend(); ++icon) {
> 
> It's not clear to me why we have to iterate backwards; I'm sure there's a
> reason.

I'll add a comment. It's preserving previous behavior of choosing the last icon listed instead of the first.

> > Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2311
> > +    // No WebCore icon loading on iOS
> > +    for (auto& icon : icons)
> > +        documentLoader->didGetLoadDecisionForIcon(false, icon.second, 0);
> 
> Can iOS be handled like the other cases where icon loading is disallowed?

I briefly considered a way of combining the runtime "disallowed" check with the iOS build time behavior.

Every iteration I came up with made the code super ugly.

> > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1794
> > +    for (auto& icon : icons)
> > +        webPage->send(Messages::WebPageProxy::GetLoadDecisionForIcon(icon.first, CallbackID::fromInteger(icon.second)));
> 
> Can this be sent as one message containing all the icons instead of a
> message for each icon?

Yes, but that's outside the scope of this patch. (Before this patch the UI process expects one at a time, and the same is true after this patch)

[Andy Estes]

(In reply to Brady Eidson from comment #38)
> (In reply to Andy Estes from comment #37)
> > Comment on attachment 314481 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=314481&action=review
> > 
> > > Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2283
> > > +    auto* frame = core(m_webFrame.get());
> > > +    DocumentLoader* documentLoader = frame->loader().documentLoader();
> > > +    ASSERT(documentLoader);
> > 
> > Can these be references instead of pointers?
> 
> You mean at the level of rearchitecting WebFrameLoaderClient?
> The WebFrame could be, the DocumentLoader probably not.
> 
> You mean in this patch? No.

I simply meant:

    auto& frame = *core(m_webFrame.get());
    DocumentLoader& documentLoader = *frame->loader().documentLoader();

[Brady Eidson]

Created attachment 314500 [details]
Patch

[Brady Eidson]

(In reply to Andy Estes from comment #39)
> (In reply to Brady Eidson from comment #38)
> > (In reply to Andy Estes from comment #37)
> > > Comment on attachment 314481 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=314481&action=review
> > > 
> > > > Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2283
> > > > +    auto* frame = core(m_webFrame.get());
> > > > +    DocumentLoader* documentLoader = frame->loader().documentLoader();
> > > > +    ASSERT(documentLoader);
> > > 
> > > Can these be references instead of pointers?
> > 
> > You mean at the level of rearchitecting WebFrameLoaderClient?
> > The WebFrame could be, the DocumentLoader probably not.
> > 
> > You mean in this patch? No.
> 
> I simply meant:
> 
>     auto& frame = *core(m_webFrame.get());
>     DocumentLoader& documentLoader = *frame->loader().documentLoader();

Oh, no, I much prefer to hold the ASSERT.

[WebKit Commit Bot]

Comment on attachment 314500 [details]
Patch

Clearing flags on attachment: 314500

Committed r219099: <http://trac.webkit.org/changeset/219099>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.