For consistency we should apply the CSP img-src directive to favicon loads.
Part of <rdar://problem/32082654>
Created attachment 311547 [details] Patch and layout tests
Comment on attachment 311547 [details] Patch and layout tests Attachment 311547 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3844914 New failing tests: http/tests/security/contentSecurityPolicy/allow-favicon.html
Created attachment 311552 [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 311547 [details] Patch and layout tests Attachment 311547 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3844925 New failing tests: http/tests/security/contentSecurityPolicy/allow-favicon.html
Created attachment 311553 [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
Created attachment 311600 [details] Patch and layout tests
Comment on attachment 311600 [details] Patch and layout tests Why do we even have favicon support in WebKit any more? What client is using it? I am pretty sure Safari does not want to use this any more. WebKIt should just parse the filename; icon loading should be done outside WebKit.
(In reply to Darin Adler from comment #8) > Comment on attachment 311600 [details] > Patch and layout tests > > Why do we even have favicon support in WebKit any more? What client is using > it? I am pretty sure Safari does not want to use this any more. WebKIt > should just parse the filename; icon loading should be done outside WebKit. This is not right. Moving to a system where the client app gathers URLs on its own and loads them in its own networking context(s), many of the loads would fail or return incorrect results. While there is tons of legacy "IconDatabase" code we can remove from WebKit soon related to disk storage and load decision making, WebKit still needs to do the actual loading of the icons itself, as directed by the client app. Safari *is* using this new client interface.
Comment on attachment 311600 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=311600&action=review > Source/WebKit2/ChangeLog:9 > + Return 0 seconds for the maximum buffering time for favicons (no change in behavior). This was going to be my comment - Looking at the source changes, it appears there's not actually a behavior change here. What part(s) of the source changes are actually relevant here?
Comment on attachment 311600 [details] Patch and layout tests Removing the comment seems to be the only relevant part of this? Or I'm missing something.
Comment on attachment 311600 [details] Patch and layout tests Dan helped me realize where the behavior change was. Looks fine.
Committed r218015: <http://trac.webkit.org/changeset/218015>
(In reply to Daniel Bates from comment #13) > Committed r218015: <http://trac.webkit.org/changeset/218015> Removed CONSOLE MESSAGE line I inadvertently left in the expected result for test block-favicon.html from an earlier iteration of the test that did not call testRunner.queueReload() and committed this in <http://trac.webkit.org/changeset/218026>.
We have traditionally shied away from using the term favicon, and generally just used icon (or site icon sometimes I think). I don't see a compelling reason to start using this term now. Also, does this new type represent all the different types of icons that may be in this class: - traditional known location site icons (favicon.ico) - <link rel="icon"> icons - <link rel="mask-icon"> icons - <link rel="apple-touch-icon"> icons (Am I forgetting any?)
(In reply to Sam Weinig from comment #15) > Also, does this new type represent all the different types of icons that may > be in this class:...? Yes, IconLoader is used for all of these.
(In reply to Brady Eidson from comment #16) > (In reply to Sam Weinig from comment #15) > > > Also, does this new type represent all the different types of icons that may > > be in this class:...? > > Yes, IconLoader is used for all of these. Cool cool. Can we not use the name 'favicon' then?
(In reply to Sam Weinig from comment #17) > (In reply to Brady Eidson from comment #16) > > (In reply to Sam Weinig from comment #15) > > > > > Also, does this new type represent all the different types of icons that may > > > be in this class:...? > > > > Yes, IconLoader is used for all of these. > > Cool cool. Can we not use the name 'favicon' then? Filed bug #173400 to use "icon" instead of "favicon".