Bug 52577 requests more universal beforeload support. The icon and prefetch types are particularly easy to add, and can be done first.
Created attachment 85930 [details] Patch
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.
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?
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.
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.
> > 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 :)
Created attachment 87195 [details] Patch
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.
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.
Created attachment 87210 [details] Patch
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.
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
Created attachment 87301 [details] Patch
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 ?
> 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.
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
Created attachment 87365 [details] Patch
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...
(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.
Comment on attachment 87365 [details] Patch Clearing flags on attachment: 87365 Committed r82342: <http://trac.webkit.org/changeset/82342>
All reviewed patches have been landed. Closing bug.
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