Sync fullscreen element removal with the spec

Sync fullscreen element removal with the spec

http://fullscreen.spec.whatwg.org/#model
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26326

TEST=LayoutTests/fullscreen/model/remove-*.html (new)
TEST=LayoutTests/fullscreen/full-screen-remove-*.html (no regression)

The distinction between fullyExitFullscreen() and exitFullscreen() was
not tested, but the new remove-last.html test fails with the old code.

BUG=383813
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180259

[philipj_slow, 2014-08-13 21:04:26]

PTAL. Hooking everything into Element::removedFrom seems to simplify matters,
and the old tests still pass, so that's nice.

[falken, 2014-08-14 04:12:47]

lgtm

https://codereview.chromium.org/476433002/diff/1/LayoutTests/fullscreen/model...
File LayoutTests/fullscreen/model/remove-first.html (right):

https://codereview.chromium.org/476433002/diff/1/LayoutTests/fullscreen/model...
LayoutTests/fullscreen/model/remove-first.html:31: });
Totally just an idea, but you might consider using Promises to avoid this
indentation marching. The Service Worker team has been doing so, you can see
recent tests in http/tests/serviceworker for examples.

It would look something like this:

function wait_for_fullscreenchange() {
    return new Promise(function(resolve) {
        document.onfullscreenchange = function(e) {
            resolve();
        }
    });
}

async_test(function(t)
{
    var first = document.getElementById("first");
    trusted_request(first);
    return wait_for_fullscreenchange();
    }).then(function() {
        assert_equals(document.fullscreenElement, first);
        var last = document.getElementById("last");
        trusted_request(last);
        return wait_for_fullscreenchange();
    }).then(function() {
        assert_equals(document.fullscreenElement, last);
        first.remove();
        return wait_for_fullscreenchange();
    }).then(function() {
        assert_equals(document.fullscreenElement, null);
        t.done();
    }).catch(function(error) {
        assert_unreached();
    });
});

https://codereview.chromium.org/476433002/diff/1/Source/core/dom/FullscreenEl...
File Source/core/dom/FullscreenElementStack.cpp (right):

https://codereview.chromium.org/476433002/diff/1/Source/core/dom/FullscreenEl...
Source/core/dom/FullscreenElementStack.cpp:565: // If an element element in a
fullscreen element stack is removed from a document document, run
I find these hard to read. How about putting italicized words in pipes like
|element| and |document|.

https://codereview.chromium.org/476433002/diff/1/Source/core/dom/FullscreenEl...
Source/core/dom/FullscreenElementStack.cpp:581: }
Is it worth doing NOTREACHED() here?

[philipj_slow, 2014-08-14 08:10:31]

nits

[philipj_slow, 2014-08-14 08:10:41]

https://codereview.chromium.org/476433002/diff/1/Source/core/dom/FullscreenEl...
File Source/core/dom/FullscreenElementStack.cpp (right):

https://codereview.chromium.org/476433002/diff/1/Source/core/dom/FullscreenEl...
Source/core/dom/FullscreenElementStack.cpp:565: // If an element element in a
fullscreen element stack is removed from a document document, run
On 2014/08/14 04:12:47, falken wrote:
> I find these hard to read. How about putting italicized words in pipes like
> |element| and |document|.

I had done this, but then I started worrying about consistency. Let's do it
everywhere:
https://codereview.chromium.org/473803002/

https://codereview.chromium.org/476433002/diff/1/Source/core/dom/FullscreenEl...
Source/core/dom/FullscreenElementStack.cpp:581: }
On 2014/08/14 04:12:47, falken wrote:
> Is it worth doing NOTREACHED() here?

It's actually reachable, because elementRemoved will be called without knowing
if the element is in fact in the fullscreen element stack. I could try to use
containsFullScreenElement() to optimize, but I want to remove that flag
eventually because it's only really needed for the :-webkit-full-screen-ancestor
pseudo-class which can hopefully be removed after the top-layer rewrite.

I'll add a comment here, but if you prefer I could determine "in fullscreen
element stack" first and then do the steps per spec.

[philipj_slow, 2014-08-14 08:16:29]

https://codereview.chromium.org/476433002/diff/1/LayoutTests/fullscreen/model...
File LayoutTests/fullscreen/model/remove-first.html (right):

https://codereview.chromium.org/476433002/diff/1/LayoutTests/fullscreen/model...
LayoutTests/fullscreen/model/remove-first.html:31: });
On 2014/08/14 04:12:47, falken wrote:
> Totally just an idea, but you might consider using Promises to avoid this
> indentation marching. The Service Worker team has been doing so, you can see
> recent tests in http/tests/serviceworker for examples.
> 
> It would look something like this:
> 
> function wait_for_fullscreenchange() {
>     return new Promise(function(resolve) {
>         document.onfullscreenchange = function(e) {
>             resolve();
>         }
>     });
> }
> 
> async_test(function(t)
> {
>     var first = document.getElementById("first");
>     trusted_request(first);
>     return wait_for_fullscreenchange();
>     }).then(function() {
>         assert_equals(document.fullscreenElement, first);
>         var last = document.getElementById("last");
>         trusted_request(last);
>         return wait_for_fullscreenchange();
>     }).then(function() {
>         assert_equals(document.fullscreenElement, last);
>         first.remove();
>         return wait_for_fullscreenchange();
>     }).then(function() {
>         assert_equals(document.fullscreenElement, null);
>         t.done();
>     }).catch(function(error) {
>         assert_unreached();
>     });
> });

That looks really nice! Unfortunately, I've been running the tests in Opera
12.16, which had the unprefixed API, and depending on Promises would make the
tests always fail...

I'm really on the fence, so what I think I'm going to do is go with the
indentation marching for this CL, see if future tests become equally or more
convoluted, and if so try to convert all tests in a single go.

[philipj_slow, 2014-08-14 08:16:30]

https://codereview.chromium.org/476433002/diff/1/LayoutTests/fullscreen/model...
File LayoutTests/fullscreen/model/remove-first.html (right):

https://codereview.chromium.org/476433002/diff/1/LayoutTests/fullscreen/model...
LayoutTests/fullscreen/model/remove-first.html:31: });
On 2014/08/14 04:12:47, falken wrote:
> Totally just an idea, but you might consider using Promises to avoid this
> indentation marching. The Service Worker team has been doing so, you can see
> recent tests in http/tests/serviceworker for examples.
> 
> It would look something like this:
> 
> function wait_for_fullscreenchange() {
>     return new Promise(function(resolve) {
>         document.onfullscreenchange = function(e) {
>             resolve();
>         }
>     });
> }
> 
> async_test(function(t)
> {
>     var first = document.getElementById("first");
>     trusted_request(first);
>     return wait_for_fullscreenchange();
>     }).then(function() {
>         assert_equals(document.fullscreenElement, first);
>         var last = document.getElementById("last");
>         trusted_request(last);
>         return wait_for_fullscreenchange();
>     }).then(function() {
>         assert_equals(document.fullscreenElement, last);
>         first.remove();
>         return wait_for_fullscreenchange();
>     }).then(function() {
>         assert_equals(document.fullscreenElement, null);
>         t.done();
>     }).catch(function(error) {
>         assert_unreached();
>     });
> });

That looks really nice! Unfortunately, I've been running the tests in Opera
12.16, which had the unprefixed API, and depending on Promises would make the
tests always fail...

I'm really on the fence, so what I think I'm going to do is go with the
indentation marching for this CL, see if future tests become equally or more
convoluted, and if so try to convert all tests in a single go.

[falken, 2014-08-14 08:27:41]

still lgtm

[philipj_slow, 2014-08-14 08:29:01]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-08-14 08:30:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/philipj@opera.com/476433002/20001

[commit-bot: I haz the power, 2014-08-14 11:35:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)

[commit-bot: I haz the power, 2014-08-14 14:32:13]

Committed patchset #2 (20001) as 180259