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
Brent Fulgham
2016-04-18 20:44:33 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). Created attachment 276688 [details]
Patch
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. (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 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. (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. Created attachment 276780 [details]
Patch v2
Comment on attachment 276780 [details]
Patch v2
Incorporated Dan's suggestions.
Comment on attachment 276780 [details] Patch v2 Clearing flags on attachment: 276780 Committed r199752: <http://trac.webkit.org/changeset/199752> All reviewed patches have been landed. Closing bug. 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' (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. Build fix landed in r199765: <http://trac.webkit.org/changeset/199765>. |