Fix memory leak in malformed test
Created attachment 297256 [details] Patch
Comment on attachment 297256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297256&action=review > LayoutTests/ChangeLog:10 > + but we need to look into fixing this code-side. You should file a bug about this. Also, maybe "in WebKitTestRunner" or something instead of "code-side"; JS counts as code no matter how you feel about the language :) > LayoutTests/http/tests/quicklook/resources/tap-at-point-and-notify-done.js:6 > +function tapAtPointAndNotifyDoneGuarded(x, y) Because it calls notifyDone(), it's *never* OK to do this twice, so I think you should just guard the other one instead. (I know you're trying to limit impact, but I almost think we would probably rather find out about other tests that would be broken by this change now rather than leave them silently doing stupid things). > LayoutTests/http/tests/quicklook/resources/tap-at-point-and-notify-done.js:8 > + if ( typeof tapAtPointAndNotifyDoneGuarded.guard == 'undefined' ) { no spaces inside the parens
Created attachment 297259 [details] Patch
Comment on attachment 297259 [details] Patch Most of my comments remain.
Created attachment 297261 [details] Patch
Comment on attachment 297261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297261&action=review > LayoutTests/ChangeLog:8 > + Navigation caused 'onload' to be called twice, causing test harness to have extranious extraneous
Comment on attachment 297261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297261&action=review > LayoutTests/ChangeLog:9 > + HID callback around after script controller was dismantled. Guard should fix test, I think that a cleaner way to fix this would be to move the onload attribute to main frame body. We normally don't want tests to run before the main frame is done loading.
Created attachment 297288 [details] Patch
Comment on attachment 297288 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297288&action=review > LayoutTests/ChangeLog:8 > + Navigation caused 'onload' to be called twice, causing test harness to have extraneous Would you be willing to look into the other approach that I proposed above?
Oh, sorry Alexey, I didn't see your suggestion amid the churn.
Created attachment 297363 [details] Patch
Comment on attachment 297363 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297363&action=review > LayoutTests/http/tests/quicklook/hide-referer-on-navigation.html:14 > +<body onload="tapAtPointAndNotifyDone(document.getElementsByTagName('iframe')[0].offsetLeft > + + 5, document.getElementsByTagName('iframe')[0].offsetTop + 5)"> Would be nicer to say onload="doTest()" and have function doTest()... in the JS above.
Comment on attachment 297363 [details] Patch Attachment 297363 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2739798 New failing tests: http/tests/navigation/keyboard-events-during-provisional-navigation.html
Created attachment 297369 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 297363 [details] Patch Attachment 297363 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2739788 New failing tests: http/tests/navigation/keyboard-events-during-provisional-navigation.html
Created attachment 297370 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 297363 [details] Patch Attachment 297363 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2739838 New failing tests: http/tests/navigation/keyboard-events-during-provisional-navigation.html
Created attachment 297371 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
The http/tests/navigation/keyboard-events-during-provisional-navigation.html failures are unrelated.
so... can this get pushed anyways then? this is an iOS test, it can't be causing Mac failures...
(In reply to comment #20) > so... can this get pushed anyways then? this is an iOS test, it can't be > causing Mac failures... We will try to outsmart the bots.
Created attachment 297466 [details] Patch
Comment on attachment 297466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297466&action=review > LayoutTests/http/tests/quicklook/hide-referer-on-navigation.html:7 > + tapAtPointAndNotifyDone(document.getElementsByTagName('iframe')[0].offsetLeft + 5, document.getElementsByTagName('iframe')[0].offsetTop + 5) It would be slightly nicer to store the result of document.getElementsByTagName('iframe')[0] in a local variable.
Comment on attachment 297466 [details] Patch Clearing flags on attachment: 297466 Committed r210022: <http://trac.webkit.org/changeset/210022>
All reviewed patches have been landed. Closing bug.