[Webkit-unassigned] [Bug 56424] add beforeload to icon and prefetch link rel types

Sun Mar 27 07:02:53 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=56424

Gavin Peters <gavinp at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|57180                       |

--- Comment #4 from Gavin Peters <gavinp at chromium.org>  2011-03-27 07:02:53 PST ---
Thank you for your review, Tony!  I've made most of these changes locally, and on Monday I'll do the testing on a Mac etc... and get an upload of a new patch.  Until then, make do with my comments below...

(In reply to comment #3)
> (From update of attachment 85930 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85930&action=review
> 
> > LayoutTests/fast/dom/HTMLLinkElement/prefetch-beforeload.html:12
> > +}
> 
> It would be best to skip implementing a print() method and instead include <script src="../js/resources/js-test-pre.js"></script> and use testPassed()/testFailed(). Ditto in the other tests.

Done.

> > LayoutTests/platform/chromium-mac/webarchive/test-link-rel-icon-beforeload-expected.txt:1
> > +CONSOLE MESSAGE: line 8: Uncaught TypeError: Object [object Object] has no method 'dumpDOMAsWebArchive'
> 
> It is kind of unfortunate to need a pixel test for this behavior. Is there any particular reason why a webarchive test is needed to test a <link rel=icon> w/ a beforeload?

I believe so.  The existing link rel=icon test was a webarchive, and that's the only way I could find to confirm the get or not of the icon itself: on the mac platform the webarchive includes the icon, if any, downloaded, so the webarchive provides proof positive that the favicon didn't come down in the beforeload test.  I spent some time today trying to see if I could do it with the DRT.dumpResoruceResponseMIMETYpes() as in the prefetch tests, and that did not work for me.

> 
> > Source/WebCore/html/HTMLLinkElement.cpp:196
> > +bool HTMLLinkElement::checkBeforeLoadEvent()
> 
> Thanks for extracting this logic. This dance is non-obvious and failure to perform it properly has caused several bugs. The same code is in ScriptElement.cpp and might need to be in other places where these extra checks aren't performed. Is it possible to move it up to dispatchBeforeLoadEvent() or have another shared helper method to avoid the duplication? Or perhaps that should be a separate patch?

I think this belongs in another CL.  I've gone ahead and created Bug 57180 for that.  I'll tackle it soon.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.