No apple-touch-icons are fetched (and, hence, used) for CNN.con, workflowy.com etc. The same holds for apple-touch-icon-precomposed. Test steps: Load http://aruslan.me/t1237621jsa/t.html <html><head> <link href="http://www.cnn.com/favicon.ie9.ico" rel="Shortcut Icon" type="image/x-icon" /> <link href="http://i.cdn.turner.com/cnn/.e/img/3.0/global/misc/apple-touch-icon.png" rel="apple-touch-icon" type="image/png" /> <title>Title</title> </head><body>Body.</body></html> Observed results: 1. favicon.ie9.ico is fetched and shown correctly. 2. apple-touch-icon is NOT fetched; instead, the request is made to the website to fetch "default" fallback apple-touch-icon.png. Expected results: 1. favicon.ie9.ico is fetched and shown (no changes). 2. apple-touch-icon from CDN is fetched and shown.
Created attachment 186872 [details] Patch
Created attachment 186887 [details] Patch
Comment on attachment 186887 [details] Patch Attachment 186887 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16388449 New failing tests: fast/dom/icon-url-list-apple-touch.html
Comment on attachment 186887 [details] Patch Attachment 186887 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16393364
Created attachment 186926 [details] Patch
Comment on attachment 186926 [details] Patch Why are we making a change that causes all desktop webkit clients to download a non-standard icon that was meant for mobile devices?
(In reply to comment #6) > (From update of attachment 186926 [details]) > Why are we making a change that causes all desktop webkit clients to download a non-standard icon that was meant for mobile devices? Desktop clients shouldn't define ENABLE(TOUCH_ICON_LOADING). When TOUCH_ICON_LOADING is not enabled, LinkRelAttribute ignores <link rel> for non-standard icons. As the result, Document.cpp doesn't have them anyway. This patch is to fix the error that causes all TOUCH_ICON_LOADING-enabled clients to never get touch icons.
Good point, but... There is a comment inside Document::ironURLs() that says: // Include any icons where type = link, rel = "shortcut icon". Admittedly this is an example of why comments aren't good enforcement over more obvious naming in code. But that point aside, this patch changes that behavior. This method is about "shortcut icons" that should be considered in the course of normal favicon loading whereas apple-touch-icons were always meant to be loaded out of band from normal page loading. We should rename iconURLs to shortcutIconURLs and leave it's behavior as-is. If you need to get at the apple-touch-icons for whatever reason, they can be in a different accessor. You could also refactor the current accessor to take an IconType parameter and leverage that for code reuse between the current behavior and the new behavior you hope to add.
(In reply to comment #8) > We should rename iconURLs to shortcutIconURLs and leave it's behavior as-is. If you need to get at the apple-touch-icons for whatever reason, they can be in a different accessor. > You could also refactor the current accessor to take an IconType parameter and leverage that for code reuse between the current behavior and the new behavior you hope to add. Agreed; I'm going to rename existing iconURLs() to shortcutIconURLs() keeping its semantics and add iconURLs(int iconTypes) that accepts a bitmask of IconTypes. Additionally, I'll extend Internals.idl, fix tests and surrounding call-sites. Are these names and parameters fine by you?
Sounds reasonable.
Created attachment 186976 [details] Patch
(In reply to comment #10) > Sounds reasonable. Please take a look, thanks!
Comment on attachment 186976 [details] Patch Attachment 186976 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16417050
Comment on attachment 186976 [details] Patch Attachment 186976 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16409082
Comment on attachment 186976 [details] Patch Attachment 186976 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16414112
Comment on attachment 186976 [details] Patch Attachment 186976 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16403099
I'll hold off on review until ews is cleared up
(In reply to comment #17) > I'll hold off on review until ews is cleared up It fails to build on gtk/mac/mac-wk2/win with Undefined symbols for architecture x86_64: "__ZN7WebCore8Document8iconURLsEv" Strangely, this means that something refers to (now gone) iconURLs(void), but only at the link time. Otherwise it would have failed to compile the call to iconURLs(). Do we use precompiled libraries on gtk/mac/win? How could I trigger a clobber build or something?
gtk-ews fails with the reverse: ./.libs/libWebCoreInternals.a(./.libs/../Source/WebCore/testing/.libs/libWebCoreInternals_la-Internals.o):Internals.cpp:function WebCore::Internals::iconURLs(WebCore::Document*, int) const: error: undefined reference to 'WebCore::Document::iconURLs(int)' Looks like it uses correct headers, but links against a stale library.
(In reply to comment #18) > (In reply to comment #17) > > I'll hold off on review until ews is cleared up > > It fails to build on gtk/mac/mac-wk2/win with > > Undefined symbols for architecture x86_64: > "__ZN7WebCore8Document8iconURLsEv" WebCore in these ports export only specific symbols. See http://trac.webkit.org/wiki/ExportingSymbols .
Created attachment 187737 [details] Patch
(In reply to comment #21) > Created an attachment (id=187737) [details] > Patch Heads up in case you didn't notice, but this patch doesn't end up applying, so EWS gave up :(
Created attachment 187746 [details] Patch
(In reply to comment #22) > (In reply to comment #21) > > Created an attachment (id=187737) [details] [details] > > Patch > > Heads up in case you didn't notice, but this patch doesn't end up applying, so EWS gave up :( Thanks, Levi, rebase FTW.
(In reply to comment #24) > (In reply to comment #22) > > (In reply to comment #21) > > > Created an attachment (id=187737) [details] [details] [details] > > > Patch > > > > Heads up in case you didn't notice, but this patch doesn't end up applying, so EWS gave up :( > > Thanks, Levi, rebase FTW. Woo, green!
(In reply to comment #25) > (In reply to comment #24) > > (In reply to comment #22) > > > (In reply to comment #21) > > > > Created an attachment (id=187737) [details] [details] [details] [details] > > > > Patch > > > > > > Heads up in case you didn't notice, but this patch doesn't end up applying, so EWS gave up :( > > > > Thanks, Levi, rebase FTW. > > Woo, green! What are my next steps? On Mac it says "pass" and then it fails (?), on Mac-WK2 it fails as well.
Comment on attachment 187746 [details] Patch Attachment 187746 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16454806
Created attachment 188107 [details] Patch
Why I don't see Win in the EWS? Green is good. Green is healthy.
Created attachment 188252 [details] Patch
(In reply to comment #25) > (In reply to comment #24) > > (In reply to comment #22) > > > (In reply to comment #21) > > > > Created an attachment (id=187737) [details] [details] [details] [details] > > > > Patch > > > > > > Heads up in case you didn't notice, but this patch doesn't end up applying, so EWS gave up :( > > > > Thanks, Levi, rebase FTW. > > Woo, green! Rebased and re-submitted (the previous patch is green on all platforms, but for some reason EWS never put it to Win). Assuming Win is green this time, this should be a final version.
Created attachment 189218 [details] Patch
Brady, would you take another look?
Created attachment 189837 [details] Patch
Brady -- would you take another look please? Rebase. The last 4 patches are exactly the same, but first the "win" EWS bot had unrelated problems (eventually became "green"), and now "cr-android" EWS bot is failing and "mac" build had some problems apparently.
Comment on attachment 189837 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189837&action=review > Source/WebCore/dom/Document.cpp:4466 > +const Vector<IconURL>& Document::shortcutIconURLs() > +{ > + // Include any icons where type = link, rel = "shortcut icon". > + return iconURLs(Favicon); > +} Where does the "Favicon" symbol come from and... > Source/WebCore/dom/Document.cpp:4468 > +const Vector<IconURL>& Document::iconURLs(int iconTypes) ...why is this not an enum? This should be an enum
Comment on attachment 189837 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189837&action=review >> Source/WebCore/dom/Document.cpp:4466 >> +} > > Where does the "Favicon" symbol come from and... from WebCore::IconType enum (defined in Source/WebCore/dom/IconURL.h). >> Source/WebCore/dom/Document.cpp:4468 >> +const Vector<IconURL>& Document::iconURLs(int iconTypes) > > ...why is this not an enum? > > This should be an enum It's a bitmask of requested icon types (this matches IconController::urlsForTypes(int iconTypes) API).
(In reply to comment #36) > (From update of attachment 189837 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189837&action=review > > > Source/WebCore/dom/Document.cpp:4466 > > +const Vector<IconURL>& Document::shortcutIconURLs() > > +{ > > + // Include any icons where type = link, rel = "shortcut icon". > > + return iconURLs(Favicon); > > +} > > Where does the "Favicon" symbol come from and... > > > Source/WebCore/dom/Document.cpp:4468 > > +const Vector<IconURL>& Document::iconURLs(int iconTypes) > > ...why is this not an enum? > > This should be an enum Hi Brady, "Favicon" symbol comes from WebCore::IconType enum (defined in Source/WebCore/dom/IconURL.h), and >> +const Vector<IconURL>& Document::iconURLs(int iconTypes) isn't enum because it's a bitmask of requested icon types. This matches IconController::urlsForTypes(int iconTypes) API. Please let me know if anything should be adjusted here. Otherwise, I'd appreciate if you could approve this patch. Yours, -- Ruslan.
(In reply to comment #38) > (In reply to comment #36 > > Hi Brady, > "Favicon" symbol comes from WebCore::IconType enum (defined in Source/WebCore/dom/IconURL.h), and > >> +const Vector<IconURL>& Document::iconURLs(int iconTypes) > isn't enum because it's a bitmask of requested icon types. > This matches IconController::urlsForTypes(int iconTypes) API. Then please call it iconTypesMask, and no harm in updating the old one also. r+ with that change.
Comment on attachment 189837 [details] Patch r+ with the question naming change.
Created attachment 190340 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
(In reply to comment #40) > (From update of attachment 189837 [details]) > r+ with the question naming change. Renamed iconTypes to iconTypesMask in Document, Internals, IconController and Chromium WebFrame.
Comment on attachment 190340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190340&action=review > LayoutTests/fast/dom/icon-url-list-apple-touch.html:17 > + var supportsTouchIcons = window.internals.isTouchIconsLoadingSupported(); This is sort of unusual. Typically we just have different expected.txt files when compile-time options change rather than having the test automagically adapt.
Comment on attachment 190340 [details] Patch API change LGTM.
Created attachment 190974 [details] Patch
(In reply to comment #44) > (From update of attachment 190340 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190340&action=review > > > LayoutTests/fast/dom/icon-url-list-apple-touch.html:17 > > + var supportsTouchIcons = window.internals.isTouchIconsLoadingSupported(); > > This is sort of unusual. Typically we just have different expected.txt files when compile-time options change rather than having the test automagically adapt. Hi Adam and Brady, PTAL: I removed test auto-adaptation to apple-touch-icon loading and created two -expected -- one in the cross-platform with favicon only, and one in chromium with favicon and apple-touch-icon. Yours, -- Ruslan.
Comment on attachment 190974 [details] Patch Attachment 190974 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16877059 New failing tests: fast/dom/icon-url-list-apple-touch.html
Comment on attachment 190974 [details] Patch Attachment 190974 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16796239 New failing tests: fast/dom/icon-url-list-apple-touch.html
Created attachment 191088 [details] Patch
Moved the expectations from chromium/ to chromium-android/ due to chromium-linux failing because it doesn't ENABLE TOUCH_ICON_LOADING. I believe some of the chromium-mac targets could be enabling touch icons loading, in which case the expectations should be copied there from chromium-android. To summarize, I believe the auto-adapting test was a simpler design and it didn't require heavy maintenance.
Comment on attachment 191088 [details] Patch This patch looks good. Thank you for taking the expected.txt route. I can understand why it seems like the adapting test might be easier when writing the test, but this approach is consistent with how we manage other platform differences and is easier to maintain in the long run.
Comment on attachment 191088 [details] Patch Clearing flags on attachment: 191088 Committed r144567: <http://trac.webkit.org/changeset/144567>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 111266
(In reply to comment #55) > Re-opened since this is blocked by bug 111266 The patch caused Apple Windows build failures: e.g. 19>LINK : C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\bin\DumpRenderTree.dll not found or not built by the last incremental link; performing full link 19> Creating library C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\lib\DumpRenderTree.lib and object C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\lib\DumpRenderTree.exp 19>WebCoreTestSupport.lib(JSInternals.obj) : error LNK2019: unresolved external symbol "class JSC::JSValue __cdecl WebCore::toJS(class JSC::ExecState *,class WebCore::JSDOMGlobalObject *,class WebCore::CSSStyleDeclaration *)" (?toJS@WebCore@@YA?AVJSValue@JSC@@PAVExecState@3@PAVJSDOMGlobalObject@1@PAVCSSStyleDeclaration@1@@Z) referenced in function "__int64 __fastcall WebCore::jsInternalsPrototypeFunctionComputedStyleIncludingVisitedInfo(class JSC::ExecState *)" (?jsInternalsPrototypeFunctionComputedStyleIncludingVisitedInfo@WebCore@@YI_JPAVExecState@JSC@@@Z) http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/63456/steps/compile-webkit/logs/stdio It wasn’t a simple missing symbol error. It looked like something wasn’t compliling.
(In reply to comment #56) > The patch caused Apple Windows build failures: http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/63456/steps/compile-webkit/logs/stdio > > It wasn’t a simple missing symbol error. It looked like something wasn’t compliling. Thank you for jumping in on the issue! Given that the breakage wasn't caused by this patch (reverting this patch didn't help, and the build was fixed subsequently with http://trac.webkit.org/changeset/144573), I'm going to re-land it Objections?
(In reply to comment #57) > (In reply to comment #56) > > The patch caused Apple Windows build failures: > http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/63456/steps/compile-webkit/logs/stdio > > > > It wasn’t a simple missing symbol error. It looked like something wasn’t compliling. > > Thank you for jumping in on the issue! > > Given that the breakage wasn't caused by this patch (reverting this patch didn't help, and the build was fixed subsequently with http://trac.webkit.org/changeset/144573), I'm going to re-land it > > Objections? P.S. The revert (http://trac.webkit.org/changeset/144572) failed with the same link errors as this patch: http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/63457 The real fix was submitted in a subsequent patch (http://trac.webkit.org/changeset/144573) and made the tree green: http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/63458 That is, unless I'm reading it wrong.
Created attachment 191267 [details] Patch
(In reply to comment #59) > Created an attachment (id=191267) [details] > Patch This is a rebased patch. Previously the patch failed to compile on apple-win, because WebKitBuild\Debug\obj\WebCore\DerivedSources\JSInternals.cpp had to be regenerated, but there was an unrelated issue preventing this from happening. The build just before the revert successfully compiled everything related to this patch, but was blocked on the other issue: http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/63456/steps/compile-webkit/logs/stdio Reverting the patch didn't help, as the unrelated issue remained. Asking for permission to re-land the patch.
> Asking for permission to re-land the patch. The http://build.webkit.org/waterfall?show=Apple%20Win%20Debug%20(Build) bot is current not compiling. I'd prefer to re-land your patch when the bot is compiling so that we can verify that it does compile...
(In reply to comment #61) > > Asking for permission to re-land the patch. > > The http://build.webkit.org/waterfall?show=Apple%20Win%20Debug%20(Build) bot is current not compiling. I'd prefer to re-land your patch when the bot is compiling so that we can verify that it does compile... Sure thing. Thanks, Adam!
Comment on attachment 191267 [details] Patch Windows is building again. Let's try landing this patch again.
Comment on attachment 191267 [details] Patch Clearing flags on attachment: 191267 Committed r144696: <http://trac.webkit.org/changeset/144696>
http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/63524 confirms that this patch builds correct for apple-win.