Bug 156727

Summary: Remove remaining bits of dynamic <link> rel='icon' loading
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, bfulgham, cdumez, commit-queue, dbates, japhet, yoav
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=112705
https://bugs.webkit.org/show_bug.cgi?id=153151
Bug Depends on: 56424    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch v2 none

Description Brent Fulgham 2016-04-18 20:44:33 PDT
WebKit emits an "onbeforeload" event when various link operations are conducted. In Bug 153151 I stopped the old (unused) behavior of notifying clients of new favicon updates. However, I retained the code that emitted an 'onbeforeload' event.

We should not emit this non-standard event for these dynamic favicon loads, since we do not support this feature, and should not advertise that we are performing a load that we are actually ignoring.

This breaks one test:

    webarchive/test-link-rel-icon-beforeload.html

To keep things working properly, I intend to switch the link to a 'subresource' (which still emits the event) to keep the test functioning properly.

I will rename it to

    webarchive/test-link-rel-subresource-beforeload.html
Comment 1 Brent Fulgham 2016-04-18 20:48:04 PDT
Note: This test was added 5 year ago:

http://trac.webkit.org/browser/trunk/LayoutTests/webarchive/test-link-rel-icon-beforeload.html?rev=82342

Nothing in the related bug or comments indicate that "<link rel='icon'>" was the critical part of this test. Rather, it was made to ensure that 'onbeforeload' was being emitted for various rel types (specifically 'icon' and 'prefetch' links).
Comment 2 Brent Fulgham 2016-04-18 21:30:44 PDT
Created attachment 276688 [details]
Patch
Comment 3 Daniel Bates 2016-04-19 10:28:31 PDT
Comment on attachment 276688 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276688&action=review

We should also remove TestRunner.setDumpIconChanges(), TestRunner.dumpIconChanges() and the only test that made use of this functionality LayoutTests/fast/dom/icon-url-property.html. Take care to remove all references to and expected test results for LayoutTests/fast/dom/icon-url-property.html and "dumpIconChanges". From briefly using find and grep, this test is referenced in file LayoutTests/platform/{gtk, ios-simulator}/TestExpectations and has expected results LayoutTests/platform/{efl, mac, win}/fast/dom/icon-url-property-expected.txt. We should also close bug #112705 as invalid since we are removing the test LayoutTests/fast/dom/icon-url-property.html.

> Source/WebCore/ChangeLog:10
> +        Don't call 'shouldLoadLink' for 'icon' link types. It performs no
> +        useful checks for 'icon' types, and emits the non-standard
> +        'onbeforeload' event.

For your consideration, I suggest that we add a remark to this description that mentions that we removed FrameLoaderClient::dispatchDidChangeIcons(), the mechanism for notifying an embedding client of dynamic icon changes, in r199673 (bug #153151).

> LayoutTests/platform/wk2/TestExpectations:72
> +webkit.org/b/115809 webarchive/test-link-rel-subresource-beforeload.html [ Skip ]

Notice that you removed the call to setIconDatabaseEnabled() from the test. And <https://webkit.org/b/115809> is about implementing testRunner.setIconDatabaseEnabled() in WebKitTestRunner. So, this test should now run in WebKitTestRunner assuming the port is built with ENABLE(LINK_PREFETCH). We should remove this entry from this test expectation and add an entry for this test to the TestExpectations files for ports that do not enable ENABLE(LINK_PREFETCH).

> LayoutTests/webarchive/test-link-rel-subresource-beforeload.html:27
> +<link rel="subresource" onbeforeload="print('PASS','green');return false" href="resources/favicon.ico" type="image/x-icon">

OK, this test is acceptable. Notice that link type subresource is a non-standard just as event handler onbeforeload. Should we decide to support link prefetching/resource hints then we will update this test as appropriate.
Comment 4 Daniel Bates 2016-04-19 10:35:48 PDT
(In reply to comment #3)
> > LayoutTests/webarchive/test-link-rel-subresource-beforeload.html:27
> > +<link rel="subresource" onbeforeload="print('PASS','green');return false" href="resources/favicon.ico" type="image/x-icon">
> 
> OK, this test is acceptable. Notice that link type subresource is a
> non-standard just as event handler onbeforeload. Should we decide to support
> link prefetching/resource hints then we will update this test as appropriate.

Following the landing of the patch of bug #156334, we may want to consider updating the test to use <link rel="preload">.
Comment 5 Brent Fulgham 2016-04-19 15:17:50 PDT
Comment on attachment 276688 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276688&action=review

>> Source/WebCore/ChangeLog:10
>> +        'onbeforeload' event.
> 
> For your consideration, I suggest that we add a remark to this description that mentions that we removed FrameLoaderClient::dispatchDidChangeIcons(), the mechanism for notifying an embedding client of dynamic icon changes, in r199673 (bug #153151).

Good idea! I will do so.

>> LayoutTests/platform/wk2/TestExpectations:72
>> +webkit.org/b/115809 webarchive/test-link-rel-subresource-beforeload.html [ Skip ]
> 
> Notice that you removed the call to setIconDatabaseEnabled() from the test. And <https://webkit.org/b/115809> is about implementing testRunner.setIconDatabaseEnabled() in WebKitTestRunner. So, this test should now run in WebKitTestRunner assuming the port is built with ENABLE(LINK_PREFETCH). We should remove this entry from this test expectation and add an entry for this test to the TestExpectations files for ports that do not enable ENABLE(LINK_PREFETCH).

An excellent point. I'll make these changes.

>> LayoutTests/webarchive/test-link-rel-subresource-beforeload.html:27
>> +<link rel="subresource" onbeforeload="print('PASS','green');return false" href="resources/favicon.ico" type="image/x-icon">
> 
> OK, this test is acceptable. Notice that link type subresource is a non-standard just as event handler onbeforeload. Should we decide to support link prefetching/resource hints then we will update this test as appropriate.

Sounds good.
Comment 6 Brent Fulgham 2016-04-19 15:19:50 PDT
(In reply to comment #3)
> Comment on attachment 276688 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276688&action=review
> 
> We should also remove TestRunner.setDumpIconChanges(),
> TestRunner.dumpIconChanges() and the only test that made use of this
> functionality LayoutTests/fast/dom/icon-url-property.html. Take care to
> remove all references to and expected test results for
> LayoutTests/fast/dom/icon-url-property.html and "dumpIconChanges".

Will do.
Comment 7 Brent Fulgham 2016-04-19 17:19:12 PDT
Created attachment 276780 [details]
Patch v2
Comment 8 Brent Fulgham 2016-04-19 17:19:58 PDT
Comment on attachment 276780 [details]
Patch v2

Incorporated Dan's suggestions.
Comment 9 WebKit Commit Bot 2016-04-19 18:33:17 PDT
Comment on attachment 276780 [details]
Patch v2

Clearing flags on attachment: 276780

Committed r199752: <http://trac.webkit.org/changeset/199752>
Comment 10 WebKit Commit Bot 2016-04-19 18:33:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Alexey Proskuryakov 2016-04-19 23:10:17 PDT
This broke Windows build:

  PixelDumpSupportWin.cpp
C:\cygwin\home\buildbot\slave\win-release\build\Tools\DumpRenderTree\win\FrameLoadDelegate.cpp(187): error C2039: 'dumpIconChanges': is not a member of 'TestRunner' [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Tools\DumpRenderTree\DumpRenderTreeLib.vcxproj]
  C:\cygwin\home\buildbot\slave\win-release\build\Tools\DumpRenderTree\TestRunner.h(40): note: see declaration of 'TestRunner'
Comment 12 Brent Fulgham 2016-04-19 23:20:36 PDT
(In reply to comment #11)
> This broke Windows build:
> 
>   PixelDumpSupportWin.cpp
> C:\cygwin\home\buildbot\slave\win-
> release\build\Tools\DumpRenderTree\win\FrameLoadDelegate.cpp(187): error
> C2039: 'dumpIconChanges': is not a member of 'TestRunner'
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Tools\DumpRenderTree\DumpRenderTreeLib.
> vcxproj]
>  
> C:\cygwin\home\buildbot\slave\win-
> release\build\Tools\DumpRenderTree\TestRunner.h(40): note: see declaration
> of 'TestRunner'

Bah! I'll fix right now.
Comment 13 Brent Fulgham 2016-04-19 23:26:36 PDT
Build fix landed in r199765: <http://trac.webkit.org/changeset/199765>.