Bug 156727 – Remove remaining bits of dynamic <link> rel='icon' loading

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

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

[Brent Fulgham]

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).

[Brent Fulgham]

Created attachment 276688 [details]
Patch

[Daniel Bates]

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.

[Daniel Bates]

(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">.

[Brent Fulgham]

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.

[Brent Fulgham]

(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.

[Brent Fulgham]

Created attachment 276780 [details]
Patch v2

[Brent Fulgham]

Comment on attachment 276780 [details]
Patch v2

Incorporated Dan's suggestions.

[WebKit Commit Bot]

Comment on attachment 276780 [details]
Patch v2

Clearing flags on attachment: 276780

Committed r199752: <http://trac.webkit.org/changeset/199752>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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'

[Brent Fulgham]

(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.

[Brent Fulgham]

Build fix landed in r199765: <http://trac.webkit.org/changeset/199765>.