Changes to full screen mode caused two test cases to start generating extra test. The tests technically pass, but the javascript source code contained is being dumped by DRT.
Failing tests: fullscreen/full-screen-remove-ancestor.html fullscreen/full-screen-remove.html
Created attachment 78295 [details] Patch
Comment on attachment 78295 [details] Patch Rejecting attachment 78295 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2 Last 500 characters of output: ...... fast/xmlhttprequest ................ fast/xpath ..................................... fast/xpath/4XPath/Borrowed ...... fast/xpath/4XPath/Core .......... fast/xpath/py-dom-xpath ........ fast/xsl ....................................... fonts ...... fullscreen ... fullscreen/full-screen-remove-ancestor.html -> crashed Exiting early after 1 failures. 16612 tests run. 325.49s total testing time 16611 test cases (99%) succeeded 1 test case (<1%) crashed 8 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/7396070
Created attachment 78344 [details] Patch
Thanks for fixing. Unfortunately I don't really understand the patch, so best to leave this for someone else to review. :)
Comment on attachment 78344 [details] Patch Updated patch to fix the crash while running the full-screen-remove-ancestor test.
Comment on attachment 78344 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78344&action=review r=me, bonus points if you explain why the patch fixes <script> being dumped into the output, in the changelog. > Source/WebCore/ChangeLog:13 > + In nodeWillBeRemoved(), match the code in webkitDidExitFullScreen(). > + Don't detach the documentElement, but rather simply call recalcStyle(Force). Don't you mean "don't detach the fullscreen element"?
Comment on attachment 78344 [details] Patch Rejecting attachment 78344 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 78344]" exit_code: 1 Last 500 characters of output: st/html/script-tests/disable-style-element.js A LayoutTests/fast/html/disable-style-element-expected.txt M LayoutTests/ChangeLog M Source/WebCore/dom/StyleElement.h M Source/WebCore/ChangeLog M Source/WebCore/html/HTMLStyleElement.h M Source/WebCore/html/HTMLStyleElement.idl M Source/WebCore/html/HTMLStyleElement.cpp r75352 = 5b0a15fef122c3d163eb2e34b13253e38202ffea (refs/remotes/trunk) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Full output: http://queues.webkit.org/results/7400083
Ah. :) Yes. Setting cq+ will land the patch regardless. We should probably teach it to not try to land patches which are still marked r?: From the output: NOBODY (OOPS!) found in /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py.
Bug 36638 is about this strange behavior. I'll look at making a patch. Sorry for the trouble.
Comment on attachment 78344 [details] Patch I don't think this fix is right. I think the right change is to make RenderFullScreen properly anonymous. That will remove the risk of the document's renderer being destroyed, which is ultimately what goes wrong here. Like so: Index: rendering/RenderFullScreen.h =================================================================== --- rendering/RenderFullScreen.h (revision 75328) +++ rendering/RenderFullScreen.h (working copy) @@ -33,7 +33,7 @@ class RenderFullScreen : public RenderFlexibleBox { public: - RenderFullScreen(Node* node) : RenderFlexibleBox(node) { setReplaced(false); setIsAnonymous(false); } + RenderFullScreen(Node* node) : RenderFlexibleBox(node) { setReplaced(false); } virtual bool isRenderFullScreen() const { return true; } virtual const char* renderName() const { return "RenderFullScreen"; } Is there a reason RenderFullScreen is non-anonymous?
(In reply to comment #11) > (From update of attachment 78344 [details]) > > Is there a reason RenderFullScreen is non-anonymous? Yes, I found that, if RenderFullScreen was anonymous and the parent element of the full screen element also had an anonymous renderer, the layout system would collapse the two anonymous blocks and remove the RenderFullScreen renderer from the render tree.
(continuing...) I believe the function which pruned out the anonymous RenderFullScreen was void RenderBlock::removeLeftoverAnonymousBlock(RenderBlock* child).
(continuing...) However, it looks like that was before RenderFullScreen became a flex-box; the code which calls removeLeftoverAnonymousBlock will first check that the anonymous block's display style is BLOCK, and the RenderFullScreen will have a display style of BOX; the collapsing behavior described above may no longer be an issue.
(In reply to comment #14) > (continuing...) > > However, it looks like that was before RenderFullScreen became a flex-box; the code which calls removeLeftoverAnonymousBlock will first check that the anonymous block's display style is BLOCK, and the RenderFullScreen will have a display style of BOX; the collapsing behavior described above may no longer be an issue. After making the change Maciej suggested, it appears the RenderFullScreen render is still being pruned in the majority of my test cases, which breaks the animation aspect of the full screen feature.
(In reply to comment #7) > (From update of attachment 78344 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78344&action=review > > r=me, bonus points if you explain why the patch fixes <script> being dumped into the output, in the changelog. I haven't debugged into DRT yet, but I believe the render tree was being left in an inconsistent state when an ancestor was removed. > > Source/WebCore/ChangeLog:13 > > + In nodeWillBeRemoved(), match the code in webkitDidExitFullScreen(). > > + Don't detach the documentElement, but rather simply call recalcStyle(Force). > > Don't you mean "don't detach the fullscreen element"? In this case, the full screen element is the documentElement. When an ancestor of the current full screen element is removed, the documentElement becomes the new full screen element.
(In reply to comment #15) > After making the change Maciej suggested, it appears the RenderFullScreen render is still being pruned in the majority of my test cases, which breaks the animation aspect of the full screen feature. Upon further testing, it appears the renderer isn't being pruned, but rather the RenderLayer tree will not use hardware-compositing for an anonymous renderer.
Created attachment 78446 [details] Patch
(In reply to comment #18) > Created an attachment (id=78446) [details] > Patch Thanks for the r+ Simon; but I wanted to address Maciej's comment about anonymity. This new patch no longer marks RenderFullScreen as a non-anonymous renderer. The lack of accelerated compositing on the RenderFullScreen object has been fixed in this patch as well.
Comment on attachment 78446 [details] Patch Clearing flags on attachment: 78446 Committed r75450: <http://trac.webkit.org/changeset/75450>
All reviewed patches have been landed. Closing bug.