Bug 109061

Summary: <link rel="apple-touch-icon"> tag is not honored on CNN.com, workflowy.com etc
Product: WebKit Reporter: Ruslan Abdikeev <aruslan>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andersca, beidson, buildbot, ddkilzer, dglazkov, esprehn+autocc, fishd, jamesr, japhet, leviw, mathias, ojan.autocc, philn, rniwa, syoichi, tkent, tkent+wkapi, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 111266    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Ruslan Abdikeev 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.
Comment 1 Ruslan Abdikeev 2013-02-06 09:40:36 PST
Created attachment 186872 [details]
Patch
Comment 2 Ruslan Abdikeev 2013-02-06 11:47:18 PST
Created attachment 186887 [details]
Patch
Comment 3 WebKit Review Bot 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
Comment 4 Build Bot 2013-02-06 12:51:50 PST
Comment on attachment 186887 [details]
Patch

Attachment 186887 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16393364
Comment 5 Ruslan Abdikeev 2013-02-06 14:55:52 PST
Created attachment 186926 [details]
Patch
Comment 6 Brady Eidson 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?
Comment 7 Ruslan Abdikeev 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.
Comment 8 Brady Eidson 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.
Comment 9 Ruslan Abdikeev 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?
Comment 10 Brady Eidson 2013-02-06 18:09:53 PST
Sounds reasonable.
Comment 11 Ruslan Abdikeev 2013-02-06 18:42:59 PST
Created attachment 186976 [details]
Patch
Comment 12 Ruslan Abdikeev 2013-02-06 18:45:26 PST
(In reply to comment #10)
> Sounds reasonable.

Please take a look, thanks!
Comment 13 Build Bot 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
Comment 14 Build Bot 2013-02-06 22:08:29 PST
Comment on attachment 186976 [details]
Patch

Attachment 186976 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16409082
Comment 15 Build Bot 2013-02-06 23:05:24 PST
Comment on attachment 186976 [details]
Patch

Attachment 186976 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16414112
Comment 16 Build Bot 2013-02-06 23:10:10 PST
Comment on attachment 186976 [details]
Patch

Attachment 186976 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16403099
Comment 17 Brady Eidson 2013-02-07 08:57:31 PST
I'll hold off on review until ews is cleared up
Comment 18 Ruslan Abdikeev 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?
Comment 19 Ruslan Abdikeev 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.
Comment 20 Kent Tamura 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 .
Comment 21 Ruslan Abdikeev 2013-02-11 17:55:17 PST
Created attachment 187737 [details]
Patch
Comment 22 Levi Weintraub 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 :(
Comment 23 Ruslan Abdikeev 2013-02-11 18:36:25 PST
Created attachment 187746 [details]
Patch
Comment 24 Ruslan Abdikeev 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.
Comment 25 Levi Weintraub 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!
Comment 26 Ruslan Abdikeev 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.
Comment 27 Build Bot 2013-02-13 04:15:29 PST
Comment on attachment 187746 [details]
Patch

Attachment 187746 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16454806
Comment 28 Ruslan Abdikeev 2013-02-13 10:03:06 PST
Created attachment 188107 [details]
Patch
Comment 29 Ruslan Abdikeev 2013-02-13 13:20:47 PST
Why I don't see Win in the EWS?
Green is good. Green is healthy.
Comment 30 Ruslan Abdikeev 2013-02-13 20:02:48 PST
Created attachment 188252 [details]
Patch
Comment 31 Ruslan Abdikeev 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.
Comment 32 Ruslan Abdikeev 2013-02-19 18:08:39 PST
Created attachment 189218 [details]
Patch
Comment 33 Levi Weintraub 2013-02-21 12:10:56 PST
Brady, would you take another look?
Comment 34 Ruslan Abdikeev 2013-02-22 14:48:16 PST
Created attachment 189837 [details]
Patch
Comment 35 Ruslan Abdikeev 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.
Comment 36 Brady Eidson 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
Comment 37 Ruslan Abdikeev 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).
Comment 38 Ruslan Abdikeev 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.
Comment 39 Brady Eidson 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.
Comment 40 Brady Eidson 2013-02-26 11:15:17 PST
Comment on attachment 189837 [details]
Patch

r+ with the question naming change.
Comment 41 Ruslan Abdikeev 2013-02-26 12:32:28 PST
Created attachment 190340 [details]
Patch
Comment 42 WebKit Review Bot 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.
Comment 43 Ruslan Abdikeev 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.
Comment 44 Adam Barth 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.
Comment 45 Adam Barth 2013-02-26 15:11:22 PST
Comment on attachment 190340 [details]
Patch

API change LGTM.
Comment 46 Ruslan Abdikeev 2013-03-01 08:50:14 PST
Created attachment 190974 [details]
Patch
Comment 47 Ruslan Abdikeev 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.
Comment 48 WebKit Review Bot 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
Comment 49 WebKit Review Bot 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
Comment 50 Ruslan Abdikeev 2013-03-01 18:48:48 PST
Created attachment 191088 [details]
Patch
Comment 51 Ruslan Abdikeev 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.
Comment 52 Adam Barth 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.
Comment 53 WebKit Review Bot 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>
Comment 54 WebKit Review Bot 2013-03-03 01:56:16 PST
All reviewed patches have been landed.  Closing bug.
Comment 55 WebKit Review Bot 2013-03-03 02:29:49 PST
Re-opened since this is blocked by bug 111266
Comment 56 Ryosuke Niwa 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.
Comment 57 Ruslan Abdikeev 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?
Comment 58 Ruslan Abdikeev 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.
Comment 59 Ruslan Abdikeev 2013-03-04 10:09:42 PST
Created attachment 191267 [details]
Patch
Comment 60 Ruslan Abdikeev 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.
Comment 61 Adam Barth 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...
Comment 62 Ruslan Abdikeev 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!
Comment 63 Adam Barth 2013-03-04 16:18:54 PST
Comment on attachment 191267 [details]
Patch

Windows is building again.  Let's try landing this patch again.
Comment 64 WebKit Review Bot 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>
Comment 65 WebKit Review Bot 2013-03-04 16:44:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 66 Adam Barth 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.