RESOLVED FIXED 109061
<link rel="apple-touch-icon"> tag is not honored on CNN.com, workflowy.com etc
https://bugs.webkit.org/show_bug.cgi?id=109061
Summary <link rel="apple-touch-icon"> tag is not honored on CNN.com, workflowy.com etc
Ruslan Abdikeev
Reported 2013-02-06 09:24:40 PST
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.
Attachments
Patch (1.43 KB, patch)
2013-02-06 09:40 PST, Ruslan Abdikeev
no flags
Patch (4.99 KB, patch)
2013-02-06 11:47 PST, Ruslan Abdikeev
no flags
Patch (7.11 KB, patch)
2013-02-06 14:55 PST, Ruslan Abdikeev
no flags
Patch (14.23 KB, patch)
2013-02-06 18:42 PST, Ruslan Abdikeev
no flags
Patch (16.32 KB, patch)
2013-02-11 17:55 PST, Ruslan Abdikeev
no flags
Patch (16.18 KB, patch)
2013-02-11 18:36 PST, Ruslan Abdikeev
no flags
Patch (18.60 KB, patch)
2013-02-13 10:03 PST, Ruslan Abdikeev
no flags
Patch (18.45 KB, patch)
2013-02-13 20:02 PST, Ruslan Abdikeev
no flags
Patch (18.44 KB, patch)
2013-02-19 18:08 PST, Ruslan Abdikeev
no flags
Patch (18.50 KB, patch)
2013-02-22 14:48 PST, Ruslan Abdikeev
no flags
Patch (22.44 KB, patch)
2013-02-26 12:32 PST, Ruslan Abdikeev
no flags
Patch (22.07 KB, patch)
2013-03-01 08:50 PST, Ruslan Abdikeev
no flags
Patch (22.14 KB, patch)
2013-03-01 18:48 PST, Ruslan Abdikeev
no flags
Patch (22.11 KB, patch)
2013-03-04 10:09 PST, Ruslan Abdikeev
no flags
Ruslan Abdikeev
Comment 1 2013-02-06 09:40:36 PST
Ruslan Abdikeev
Comment 2 2013-02-06 11:47:18 PST
WebKit Review Bot
Comment 3 2013-02-06 12:51:01 PST
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
Build Bot
Comment 4 2013-02-06 12:51:50 PST
Ruslan Abdikeev
Comment 5 2013-02-06 14:55:52 PST
Brady Eidson
Comment 6 2013-02-06 16:31:34 PST
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?
Ruslan Abdikeev
Comment 7 2013-02-06 16:36:31 PST
(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.
Brady Eidson
Comment 8 2013-02-06 17:16:09 PST
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.
Ruslan Abdikeev
Comment 9 2013-02-06 17:56:02 PST
(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?
Brady Eidson
Comment 10 2013-02-06 18:09:53 PST
Sounds reasonable.
Ruslan Abdikeev
Comment 11 2013-02-06 18:42:59 PST
Ruslan Abdikeev
Comment 12 2013-02-06 18:45:26 PST
(In reply to comment #10) > Sounds reasonable. Please take a look, thanks!
Build Bot
Comment 13 2013-02-06 20:06:45 PST
Comment on attachment 186976 [details] Patch Attachment 186976 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16417050
Build Bot
Comment 14 2013-02-06 22:08:29 PST
Build Bot
Comment 15 2013-02-06 23:05:24 PST
Build Bot
Comment 16 2013-02-06 23:10:10 PST
Brady Eidson
Comment 17 2013-02-07 08:57:31 PST
I'll hold off on review until ews is cleared up
Ruslan Abdikeev
Comment 18 2013-02-11 08:14:06 PST
(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?
Ruslan Abdikeev
Comment 19 2013-02-11 09:52:17 PST
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.
Kent Tamura
Comment 20 2013-02-11 15:37:24 PST
(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 .
Ruslan Abdikeev
Comment 21 2013-02-11 17:55:17 PST
Levi Weintraub
Comment 22 2013-02-11 18:04:15 PST
(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 :(
Ruslan Abdikeev
Comment 23 2013-02-11 18:36:25 PST
Ruslan Abdikeev
Comment 24 2013-02-11 18:42:45 PST
(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.
Levi Weintraub
Comment 25 2013-02-11 22:39:42 PST
(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!
Ruslan Abdikeev
Comment 26 2013-02-12 15:15:19 PST
(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.
Build Bot
Comment 27 2013-02-13 04:15:29 PST
Ruslan Abdikeev
Comment 28 2013-02-13 10:03:06 PST
Ruslan Abdikeev
Comment 29 2013-02-13 13:20:47 PST
Why I don't see Win in the EWS? Green is good. Green is healthy.
Ruslan Abdikeev
Comment 30 2013-02-13 20:02:48 PST
Ruslan Abdikeev
Comment 31 2013-02-13 20:04:21 PST
(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.
Ruslan Abdikeev
Comment 32 2013-02-19 18:08:39 PST
Levi Weintraub
Comment 33 2013-02-21 12:10:56 PST
Brady, would you take another look?
Ruslan Abdikeev
Comment 34 2013-02-22 14:48:16 PST
Ruslan Abdikeev
Comment 35 2013-02-22 14:51:26 PST
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.
Brady Eidson
Comment 36 2013-02-25 10:59:57 PST
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
Ruslan Abdikeev
Comment 37 2013-02-25 11:26:45 PST
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).
Ruslan Abdikeev
Comment 38 2013-02-26 09:46:18 PST
(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.
Brady Eidson
Comment 39 2013-02-26 11:15:00 PST
(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.
Brady Eidson
Comment 40 2013-02-26 11:15:17 PST
Comment on attachment 189837 [details] Patch r+ with the question naming change.
Ruslan Abdikeev
Comment 41 2013-02-26 12:32:28 PST
WebKit Review Bot
Comment 42 2013-02-26 12:34:41 PST
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.
Ruslan Abdikeev
Comment 43 2013-02-26 12:36:56 PST
(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.
Adam Barth
Comment 44 2013-02-26 14:50:01 PST
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.
Adam Barth
Comment 45 2013-02-26 15:11:22 PST
Comment on attachment 190340 [details] Patch API change LGTM.
Ruslan Abdikeev
Comment 46 2013-03-01 08:50:14 PST
Ruslan Abdikeev
Comment 47 2013-03-01 14:10:39 PST
(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.
WebKit Review Bot
Comment 48 2013-03-01 15:25:01 PST
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
WebKit Review Bot
Comment 49 2013-03-01 16:17:05 PST
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
Ruslan Abdikeev
Comment 50 2013-03-01 18:48:48 PST
Ruslan Abdikeev
Comment 51 2013-03-01 18:54:28 PST
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.
Adam Barth
Comment 52 2013-03-02 23:32:51 PST
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.
WebKit Review Bot
Comment 53 2013-03-03 01:56:08 PST
Comment on attachment 191088 [details] Patch Clearing flags on attachment: 191088 Committed r144567: <http://trac.webkit.org/changeset/144567>
WebKit Review Bot
Comment 54 2013-03-03 01:56:16 PST
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 55 2013-03-03 02:29:49 PST
Re-opened since this is blocked by bug 111266
Ryosuke Niwa
Comment 56 2013-03-03 04:01:13 PST
(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.
Ruslan Abdikeev
Comment 57 2013-03-04 08:28:45 PST
(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?
Ruslan Abdikeev
Comment 58 2013-03-04 08:58:16 PST
(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.
Ruslan Abdikeev
Comment 59 2013-03-04 10:09:42 PST
Ruslan Abdikeev
Comment 60 2013-03-04 10:37:12 PST
(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.
Adam Barth
Comment 61 2013-03-04 13:06:51 PST
> 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...
Ruslan Abdikeev
Comment 62 2013-03-04 13:08:02 PST
(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!
Adam Barth
Comment 63 2013-03-04 16:18:54 PST
Comment on attachment 191267 [details] Patch Windows is building again. Let's try landing this patch again.
WebKit Review Bot
Comment 64 2013-03-04 16:44:36 PST
Comment on attachment 191267 [details] Patch Clearing flags on attachment: 191267 Committed r144696: <http://trac.webkit.org/changeset/144696>
WebKit Review Bot
Comment 65 2013-03-04 16:44:44 PST
All reviewed patches have been landed. Closing bug.
Adam Barth
Comment 66 2013-03-04 17:17:39 PST
http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/63524 confirms that this patch builds correct for apple-win.
Note You need to log in before you can comment on or make changes to this bug.