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.
Created attachment 314429 [details] Patch
Created attachment 314433 [details] Patch
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.
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
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.
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
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
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
*Layout test* failures... nice! Digging in.
Created attachment 314438 [details] Patch
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.
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
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.
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
Huh... the first round of ASSERTS I could easily reproduce locally... not this new round.
And I'm not seeing where EWS stashes the crash logs...
(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...
(Suffice it to say, not seeing those locally either)
(Actually, never mind - can totally reproduce these failures!)
Created attachment 314446 [details] Patch
HTTP tests now all load icons and they shouldn't necessarily. Double-checking what normally keeps them at bay.
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.
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
Created attachment 314451 [details] Patch
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 ]
(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.
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
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
(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...?
(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
(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.
Created attachment 314457 [details] Patch
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
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
iOS build error in WebFrameLoaderClient.mm... but that error is not included in the scroll back. Nice. Thanks.
Created attachment 314481 [details] Patch
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.
(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)
(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();
Created attachment 314500 [details] Patch
(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.
Comment on attachment 314500 [details] Patch Clearing flags on attachment: 314500 Committed r219099: <http://trac.webkit.org/changeset/219099>
All reviewed patches have been landed. Closing bug.