Bug 56424 – add beforeload to icon and prefetch link rel types

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Gavin Peters]

Bug 52577 requests more universal beforeload support.  The icon and prefetch types are particularly easy to add, and can be done first.

[Gavin Peters]

Created attachment 85930 [details]
Patch

[Gavin Peters]

Comment on attachment 85930 [details]
Patch

The biggest question I have is the early exits from HTMLLinkElement::process() that this patch introduces.  In the case of <link rel="prefetch stylesheet" ....> with a failing onbeforeload, the side effect of clearing the m_cachedSheet won't occur, and I'm not sure if that's good or bad or indifferent.

[Tony Gentilcore]

Comment on attachment 85930 [details]
Patch

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.

> 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?

> 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?

[Gavin Peters]

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.

[Gavin Peters]

Tony,

I went ahead this morning and did a little more testing with webarchive and http tests, to see if I could make a more intuitive test of the beforeload handling.  I will shortly upload the HTTP test version, but basically I think the test doesn't work well in the chrome port; the chrome DRT doesn't load favicons, and so favicons are _always_ successfully blocked on that port.

However, an HTTP test works very well for testing the blocking in the mac port, so I think I'll go ahead and upload that version shortly with a chromium exception landed in chrome trunk.

I'll create a bug to track that chromium behaviour.

[Tony Gentilcore]

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

Thanks. Is there a new version of the patch?

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

My main concern is only that it is a pixel test. Would be nice to avoid the platform specific results if possible.

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

Thanks :)

[Gavin Peters]

Created attachment 87195 [details]
Patch

[Gavin Peters]

Comment on attachment 87195 [details]
Patch

Tony: I added a better ChangeLog here, and during all my experiments with testing this I uncovered a limitation of Chromium's DRT (captured in bug 57259 ), and also added an http test for this functionality.

I want to draw your attention to the fact that I still left my own implementation of print in the test in webarchive.  I didn't want to include from all the way over in fast/dom/...., so it seemed the best solution.

[Tony Gentilcore]

Comment on attachment 87195 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        Unfortunately, there's lots of expectations changes here, in a strange

I don't see a lot of expectations changes, are files missing from the CL?

> LayoutTests/platform/chromium/test_expectations.txt:3340
> +BUGWK57259 : http/tests/misc/link-rel-icon-beforeload.html = FAIL PASS

Shouldn't this just be "FAIL" instead of "FAIL PASS". That way it will force removing this line when that bug is fixed.

[Gavin Peters]

Created attachment 87210 [details]
Patch

[Gavin Peters]

Comment on attachment 87210 [details]
Patch

Tony,

I changed my changelog to comment on all the skipped tests & the failure expectation.  Hopefully that's better.  As well, I modified the test_expectations.txt for chrome as you suggested.   Now hopefully it draws attention when/if we fix bug 57259.

[WebKit Commit Bot]

Comment on attachment 87210 [details]
Patch

Rejecting attachment 87210 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-..." exit_code: 2

Last 500 characters of output:
beforeload.html
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/html/HTMLLinkElement.cpp
Hunk #1 succeeded at 195 (offset 2 lines).
Hunk #2 succeeded at 217 (offset 2 lines).
Hunk #3 succeeded at 233 (offset 2 lines).
Hunk #4 succeeded at 256 (offset 2 lines).
patching file Source/WebCore/html/HTMLLinkElement.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Tony Gentilcore', u'--..." exit_code: 1

Full output: http://queues.webkit.org/results/8283211

[Gavin Peters]

Created attachment 87301 [details]
Patch

[Gavin Peters]

Comment on attachment 87301 [details]
Patch

I actually conflicted with myself on this one.  The other question is... can you commit-queue a change to chrome test_expectations.txt ?

[Tony Gentilcore]

> can you commit-queue a change to chrome test_expectations.txt ?

Yeah, that's no problem. The catch is that the cq only runs tests on the mac port, so it doesn't actually verify that the expectations update is accurate.

[WebKit Commit Bot]

Comment on attachment 87301 [details]
Patch

Rejecting attachment 87301 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-..." exit_code: 2

Last 500 characters of output:
utTests/platform/win/Skipped
patching file LayoutTests/webarchive/test-link-rel-icon-beforeload-expected.webarchive
patching file LayoutTests/webarchive/test-link-rel-icon-beforeload.html
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/html/HTMLLinkElement.cpp
patching file Source/WebCore/html/HTMLLinkElement.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Tony Gentilcore', u'--..." exit_code: 1

Full output: http://queues.webkit.org/results/8282405

[Gavin Peters]

Created attachment 87365 [details]
Patch

[Gavin Peters]

Comment on attachment 87365 [details]
Patch

Another merge failure on linux chromium test_expectations.txt.  I wonder if it's possible to land changes to that file...

[Tony Gentilcore]

(In reply to comment #18)
> (From update of attachment 87365 [details])
> Another merge failure on linux chromium test_expectations.txt.  I wonder if it's possible to land changes to that file...

If it doesn't work this time, you could try adding the new line somewhere else in the file.

[WebKit Commit Bot]

Comment on attachment 87365 [details]
Patch

Clearing flags on attachment: 87365

Committed r82342: <http://trac.webkit.org/changeset/82342>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/82342 might have broken GTK Linux 32-bit Release
The following tests are not passing:
css1/basic/comments.html
css1/basic/containment.html
css1/basic/id_as_selector.html
css1/basic/inheritance.html
css1/box_properties/border.html
css1/box_properties/border_bottom.html
css1/box_properties/border_bottom_width.html
css1/box_properties/border_left.html
css1/box_properties/border_left_width.html
css1/box_properties/border_right_inline.html
css1/box_properties/border_right_width.html
css1/box_properties/border_style.html
css1/box_properties/border_top.html
css1/box_properties/border_top_width.html
css1/box_properties/border_width.html
css1/box_properties/clear.html
css1/box_properties/clear_float.html
css1/box_properties/float_elements_in_series.html
css1/box_properties/float_margin.html
css1/box_properties/float_on_text_elements.html
css1/box_properties/height.html
css1/box_properties/margin.html
css1/box_properties/margin_bottom.html
css1/box_properties/margin_inline.html
css1/box_properties/margin_left.html
css1/box_properties/margin_right.html
css1/box_properties/margin_top.html
css1/box_properties/padding.html
css1/box_properties/padding_bottom.html
css1/box_properties/padding_inline.html
css1/box_properties/padding_left.html
css1/box_properties/padding_right.html
css1/box_properties/padding_top.html
css1/box_properties/width.html
css1/cascade/cascade_order.html
css1/classification/display.html
css1/classification/list_style_type.html
css1/classification/white_space.html
css1/color_and_background/background.html
css1/color_and_background/background_attachment.html
css1/color_and_background/background_position.html
css1/color_and_background/background_repeat.html
css1/conformance/forward_compatible_parsing.html
css1/font_properties/font.html
css1/font_properties/font_family.html
css1/font_properties/font_size.html
css1/font_properties/font_weight.html
css1/formatting_model/floating_elements.html
css1/formatting_model/height_of_lines.html
css1/formatting_model/horizontal_formatting.html
css1/formatting_model/inline_elements.html
css1/formatting_model/replaced_elements.html
css1/formatting_model/vertical_formatting.html
css1/pseudo/anchor.html
css1/pseudo/firstletter.html
css1/pseudo/firstline.html
css1/pseudo/multiple_pseudo_elements.html
css1/text_properties/letter_spacing.html
css1/text_properties/line_height.html
css1/text_properties/text_decoration.html
css1/text_properties/text_indent.html
css1/text_properties/text_transform.html
css1/text_properties/vertical_align.html
css1/text_properties/word_spacing.html
css1/units/color_units.html
css1/units/length_units.html
css2.1/t0803-c5502-mrgn-r-02-c.html
css2.1/t0803-c5505-mrgn-02-c.html
css2.1/t080301-c411-vt-mrgn-00-b.html
css2.1/t0905-c5525-fltclr-00-c-ag.html
css2.1/t0905-c5525-fltmrgn-00-c-ag.html
css2.1/t0905-c5525-fltwidth-00-c-g.html
css2.1/t0905-c5526-fltclr-00-c-ag.html
css2.1/t1002-c5523-width-02-b-g.html
css2.1/t1202-counters-08-b.html
css2.1/t1202-counters-09-b.html
css2.1/t140201-c535-bg-fixd-00-b-g.html
css2.1/t140201-c537-bgfxps-00-c-ag.html
css2.1/t1508-c527-font-07-b.html
editing/deleting/delete-after-span-ws-001.html
editing/deleting/delete-after-span-ws-002.html
editing/deleting/delete-after-span-ws-003.html
editing/deleting/delete-line-end-ws-001.html
editing/deleting/delete-line-end-ws-002.html
editing/inserting/insert-div-023.html
editing/selection/focus_editable_html.html
editing/selection/select-all-001.html
editing/selection/select-all-002.html
editing/selection/select-all-003.html
editing/selection/select-all-004.html
editing/selection/unrendered-001.html
editing/selection/unrendered-002.html
editing/selection/unrendered-003.html
editing/selection/unrendered-004.html
editing/selection/unrendered-005.html
fast/text/international/complex-character-based-fallback.html
fast/text/international/thai-line-breaks.html
fonts/cursive.html
fonts/default.html
fonts/fantasy.html
fonts/monospace.html
fonts/sans-serif.html
fonts/serif.html
http/tests/misc/link-rel-icon-beforeload.html
media/video-controls-rendering.html
media/video-zoom.html
tables/mozilla/bugs/bug101674.html
tables/mozilla/bugs/bug10269-2.html
tables/mozilla/bugs/bug10296-1.html
tables/mozilla/bugs/bug1055-1.html
tables/mozilla/bugs/bug113235-1.html
tables/mozilla/bugs/bug113235-3.html
tables/mozilla/bugs/bug11944.html
tables/mozilla/bugs/bug120364.html
tables/mozilla/bugs/bug12384.html
tables/mozilla/bugs/bug1302.html
tables/mozilla/bugs/bug131020.html
tables/mozilla/bugs/bug131020_iframe.html
tables/mozilla/bugs/bug137388-2.html
tables/mozilla/bugs/bug194024.html
tables/mozilla/bugs/bug22019.html
tables/mozilla/bugs/bug23151.html
tables/mozilla/bugs/bug2479-1.html
tables/mozilla/bugs/bug2479-3.html
tables/mozilla/bugs/bug2479-4.html
tables/mozilla/bugs/bug27038-2.html
tables/mozilla/bugs/bug29314.html
tables/mozilla/bugs/bug2947.html
tables/mozilla/bugs/bug32205-2.html
tables/mozilla/bugs/bug38916.html
tables/mozilla/bugs/bug3977.html
tables/mozilla/bugs/bug43039.html
tables/mozilla/bugs/bug43854-1.html
tables/mozilla/bugs/bug44505.html
tables/mozilla/bugs/bug46480-1.html
tables/mozilla/bugs/bug46480-2.html
tables/mozilla/bugs/bug50695-1.html
tables/mozilla/bugs/bug56405.html
tables/mozilla/bugs/bug5797.html
tables/mozilla/bugs/bug5835.html
tables/mozilla/bugs/bug625.html
tables/mozilla/bugs/bug650.html
tables/mozilla/bugs/bug67915-1.html
tables/mozilla/bugs/bug7112-1.html
tables/mozilla/bugs/bug7112-2.html
tables/mozilla/bugs/bug73321.html
tables/mozilla/bugs/bug92143.html
tables/mozilla/bugs/bug96334.html
tables/mozilla/bugs/bug96343.html
tables/mozilla/collapsing_borders/bug41262-3.html
tables/mozilla/core/bloomberg.html
tables/mozilla/core/captions.html
tables/mozilla/core/cell_heights.html
tables/mozilla/core/col_span.html
tables/mozilla/core/col_widths_auto_fix.html
tables/mozilla/core/col_widths_fix_fixPer.html
tables/mozilla/core/nested1.html
tables/mozilla/core/one_row.html
tables/mozilla/core/row_span.html
tables/mozilla/marvin/backgr_index.html
tables/mozilla/marvin/backgr_layers-opacity.html
tables/mozilla/marvin/backgr_position-table.html
tables/mozilla/marvin/backgr_simple-table-cell.html
tables/mozilla/marvin/backgr_simple-table-column-group.html
tables/mozilla/marvin/backgr_simple-table-column.html
tables/mozilla/marvin/backgr_simple-table-row-group.html
tables/mozilla/marvin/backgr_simple-table-row.html
tables/mozilla/marvin/backgr_simple-table.html
tables/mozilla/marvin/x_table_bgcolor_name.xml
tables/mozilla/marvin/x_table_bgcolor_rgb.xml
tables/mozilla/marvin/x_td_bgcolor_name.xml
tables/mozilla/marvin/x_td_bgcolor_rgb.xml
tables/mozilla/marvin/x_td_height.xml
tables/mozilla/marvin/x_td_nowrap.xml
tables/mozilla/marvin/x_th_bgcolor_name.xml
tables/mozilla/marvin/x_th_bgcolor_rgb.xml
tables/mozilla/marvin/x_th_height.xml
tables/mozilla/marvin/x_th_nowrap.xml
tables/mozilla/marvin/x_tr_bgcolor_name.xml
tables/mozilla/marvin/x_tr_bgcolor_rgb.xml
tables/mozilla/other/cell_widths.html
tables/mozilla/other/nestedTables.html
tables/mozilla/other/test3.html
tables/mozilla/other/test6.html
tables/mozilla/other/wa_table_thtd_rowspan.html
tables/mozilla/other/wa_table_tr_align.html
tables/mozilla_expected_failures/bugs/bug10140.html
tables/mozilla_expected_failures/bugs/bug101759.html
tables/mozilla_expected_failures/bugs/bug10216.html
tables/mozilla_expected_failures/bugs/bug1055-2.html
tables/mozilla_expected_failures/bugs/bug106966.html
tables/mozilla_expected_failures/bugs/bug131020-3.html
tables/mozilla_expected_failures/bugs/bug14007-1.html
tables/mozilla_expected_failures/bugs/bug14007-2.html
tables/mozilla_expected_failures/bugs/bug19526.html
tables/mozilla_expected_failures/bugs/bug220653.html
tables/mozilla_expected_failures/bugs/bug22122.html
tables/mozilla_expected_failures/bugs/bug2479-5.html
tables/mozilla_expected_failures/bugs/bug32205-1.html
tables/mozilla_expected_failures/bugs/bug67915-2.html
tables/mozilla_expected_failures/bugs/bug7243.html
tables/mozilla_expected_failures/bugs/bug80762-2.html
tables/mozilla_expected_failures/bugs/bug85016.html
tables/mozilla_expected_failures/bugs/bug89315.html
tables/mozilla_expected_failures/bugs/bug91057.html
tables/mozilla_expected_failures/core/backgrounds.html
tables/mozilla_expected_failures/core/captions1.html
tables/mozilla_expected_failures/core/captions2.html
tables/mozilla_expected_failures/core/captions3.html
tables/mozilla_expected_failures/core/col_span2.html
tables/mozilla_expected_failures/core/columns.html
tables/mozilla_expected_failures/core/conflicts.html
tables/mozilla_expected_failures/core/standards1.html
tables/mozilla_expected_failures/marvin/backgr_border-table-cell.html
tables/mozilla_expected_failures/marvin/backgr_border-table-column-group.html
tables/mozilla_expected_failures/marvin/backgr_border-table-column.html
tables/mozilla_expected_failures/marvin/backgr_border-table-quirks.html
tables/mozilla_expected_failures/marvin/backgr_border-table-row-group.html
tables/mozilla_expected_failures/marvin/backgr_border-table-row.html
tables/mozilla_expected_failures/marvin/backgr_border-table.html
tables/mozilla_expected_failures/marvin/backgr_fixed-bg.html
tables/mozilla_expected_failures/marvin/backgr_layers-hide.html
tables/mozilla_expected_failures/marvin/backgr_layers-show.html
tables/mozilla_expected_failures/marvin/backgr_position-table-cell.html
tables/mozilla_expected_failures/marvin/backgr_position-table-column-group.html
tables/mozilla_expected_failures/marvin/backgr_position-table-column.html
tables/mozilla_expected_failures/marvin/backgr_position-table-row-group.html
tables/mozilla_expected_failures/marvin/backgr_position-table-row.html
tables/mozilla_expected_failures/marvin/table_overflow_dirty_reflow_row.html
tables/mozilla_expected_failures/marvin/table_overflow_style_reflow_row_sibling.html
tables/mozilla_expected_failures/marvin/table_overflow_style_reflow_tbody_sibling.html
tables/mozilla_expected_failures/other/empty_cells.html
tables/mozilla_expected_failures/other/test4.html
transforms/2d/hindi-rotated.html
transforms/2d/transform-fixed-container.html