Move pending state from FullscreenController to Fullscreen

Move pending state from FullscreenController to Fullscreen

The precise details of how competing enter+exit requests should work is
currently an ongoing discussion:
https://github.com/whatwg/fullscreen/issues/63
https://github.com/whatwg/fullscreen/pull/64
https://github.com/w3c/web-platform-tests/pull/4250

Previous behavior of FullscreenController:

enterFullscreenForElement() calls WebFrameClient::enterFullscreen()
unless we're already in fullscreen, transitioning in or out of
fullscreen, or if the request was for a cross-process descendant.
(m_fullscreenFrame is non-null also when transitioning out.) This
explains the "request then exit+request" behavior of simply exiting, as
no IPC to enter fullscreen is sent in this situation. However, it's not
obviously intentional, as it's covered by the "We are already in
fullscreen" bits.

exitFullscreen() calls WebFrameClient::exitFullscreen() unless
didExitFullscreen() is on the stack (m_isCancelingFullscreen). This
explains the "request+exit" behavior of "enters+exits quickly", as both
the enter and exit IPCs are sent.

Behavior is preserved as far as possible, with deliberate exception that
the "enters+exits quickly" behavior is no more, due to the added early
return in FullscreenController::exitFullscreen.

The fullyExitFullscreen call surrounded by m_isCancelingFullscreen was a
workaround, and is moved into Fullscreen::didExitFullscreen. Because
FullscreenController::exitFullscreen now does nothing if not in
fullscreen, m_isCancelingFullscreen or similar is no longer needed.

With no Member<>s left in FullscreenController, it can be owned as a
std::unique_ptr by WebViewImpl.

TEST=https://codereview.chromium.org/2541833002
BUG=402376
Committed: https://crrev.com/a2441f293224a5c5c0352f40c5e8fcbbf1a5a051
Cr-Commit-Position: refs/heads/master@{#436585}

[foolip, 2016-12-02 16:57:53]

Description was changed from

==========
Move pending state from FullscreenController to Fullscreen

BUG=
==========

to

==========
Move pending state from FullscreenController to Fullscreen

The precise details of how competing enter+exit requests should work is
currently an ongoing discussion:
https://github.com/whatwg/fullscreen/issues/63
https://github.com/whatwg/fullscreen/pull/64
https://github.com/w3c/web-platform-tests/pull/4250

Previous behavior of FullscreenController:

enterFullscreenForElement() calls WebFrameClient::enterFullscreen()
unless we're already in fullscreen, transitioning in or out of
fullscreen, or if the request was for a cross-process descendant.
(m_fullscreenFrame is non-null also when transitioning out.) This
explains the "request then exit+request" behavior of simply exiting, as
no IPC to enter fullscreen is sent in this situation. However, it's not
obviously intentional, as it's covered by the "We are already in
fullscreen" bits.

exitFullscreen() calls WebFrameClient::exitFullscreen() unless
didExitFullscreen() is on the stack (m_isCancelingFullscreen). This
explains the "request+exit" behavior of "enters+exits quickly", as both
the enter and exit IPCs are sent.

Behavior is preserved as far as possible, with deliberate exception that
the "enters+exits quickly" behavior is no more, due to the added early
return in FullscreenController::exitFullscreen.

The fullyExitFullscreen call surrounded by m_isCancelingFullscreen was a
workaround, and is moved into Fullscreen::didExitFullscreen. Because
FullscreenController::exitFullscreen now does nothing if not in
fullscreen, m_isCancelingFullscreen or similar is no longer needed.

With no Member<>s left in FullscreenController, it can be owned as a
std::unique_ptr by WebViewImpl.

TEST=https://codereview.chromium.org/2541833002
BUG=402376
==========

[foolip, 2016-12-02 16:58:01]

The CQ bit was checked by foolip@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 16:58:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[foolip, 2016-12-02 17:01:00]

foolip@chromium.org changed reviewers:
+ eae@chromium.org, esprehn@chromium.org, mlamouri@chromium.org

[foolip, 2016-12-02 17:01:01]

PTAL. This is an alternative to https://codereview.chromium.org/2495423004/ that
will take us down a different path spec-wise. It's ready for proper review, but
I will probably add a test for the "enters+exits quickly" behavior next week.

[commit-bot: I haz the power, 2016-12-02 17:01:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 17:01:20]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[foolip, 2016-12-02 17:07:20]

The CQ bit was checked by foolip@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 17:07:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 18:26:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 18:26:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[foolip, 2016-12-02 22:28:29]

The CQ bit was checked by foolip@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 22:29:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-03 01:19:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-03 01:19:22]

Dry run: This issue passed the CQ dry run.

[eae, 2016-12-05 13:51:37]

Love the added comments, makes the code a lot easier to understand!

LGTM

[mlamouri (slow - plz ping), 2016-12-05 13:54:54]

sgtm - what would be the spec change?

https://codereview.chromium.org/2550703002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/FullscreenController.h (right):

https://codereview.chromium.org/2550703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FullscreenController.h:78: Initial,
Is `Initial` used after exiting fullscreen scroll and scale has been restored?

https://codereview.chromium.org/2550703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FullscreenController.h:82:
NeedsScrollAndScaleRestore
Add a comment explaining what that means?

[foolip, 2016-12-06 11:31:40]

mlamouri@, I've imported some WIP tests from
https://github.com/w3c/web-platform-tests/pull/4250, two of which illustrate the
kinds of things that would be observably different depending on how one specs
this.

On one side of the spectrum, one can imagine a single tab-wide (browser side)
queue of all requests, where they are processed in order, with a guarantee that
they are processed in order, so that events, state changes and promises always
happen in a predictable order.

On the opposite site, one can have state on all frames and all elements, so that
when doing things like the
element-request-fullscreen-two-{elements,iframes}.html tests, the request order
doesn't matter, because tree order is used.

Ultimately this isn't particularly important, but then spec will have to say
something, since now it's accidentally underdefined and racy.

I have some affinity for the global queue model, and will perhaps revisit
something like https://codereview.chromium.org/2495423004/, but finishing some
other timing-related Fullscreen changes will make it more clear what really
makes sense. And I need to discuss more with the other implementers.

https://codereview.chromium.org/2550703002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/FullscreenController.h (right):

https://codereview.chromium.org/2550703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FullscreenController.h:78: Initial,
On 2016/12/05 13:54:54, mlamouri wrote:
> Is `Initial` used after exiting fullscreen scroll and scale has been restored?

I have added a documentation block, can you take a look?

[foolip, 2016-12-06 11:32:15]

The CQ bit was checked by foolip@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 11:32:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 11:39:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 11:39:02]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mlamouri (slow - plz ping), 2016-12-06 12:15:47]

lgtm

Thanks for adding the tests. I don't think you are solving the problem for
OOPIF. I would encourage you to have the browser process to deal with fullscreen
requests dispatch because it would be more agnostic of Local vs Remote Frames. I
don't have the whole vision of how Fullscreen work contrary to you so I might be
naïve :)

https://codereview.chromium.org/2550703002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fullscreen/api/element-request-fullscreen-and-remove-iframe.html
(right):

https://codereview.chromium.org/2550703002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fullscreen/api/element-request-fullscreen-and-remove-iframe.html:6:
<div id="log"></div>
What's this for?

https://codereview.chromium.org/2550703002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FullscreenController.h (right):

https://codereview.chromium.org/2550703002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FullscreenController.h:84: //    
EnteringFullscreen, in case of a quick exit+enter.
Thanks :)

[foolip, 2016-12-06 14:03:59]

https://codereview.chromium.org/2550703002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fullscreen/api/element-request-fullscreen-and-remove-iframe.html
(right):

https://codereview.chromium.org/2550703002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fullscreen/api/element-request-fullscreen-and-remove-iframe.html:6:
<div id="log"></div>
On 2016/12/06 12:15:47, mlamouri wrote:
> What's this for?

testharness.js automatically adds a <div id="log"> if one isn't present, see
Output.prototype.resolve_log. It's appended to the document, so in some tests
that make sense to run manually (which is true here), I add it explicitly so
it'll be at the top of the page.

[foolip, 2016-12-06 14:08:55]

On 2016/12/06 12:15:47, mlamouri wrote:
> lgtm
> 
> Thanks for adding the tests. I don't think you are solving the problem for
> OOPIF. I would encourage you to have the browser process to deal with
fullscreen
> requests dispatch because it would be more agnostic of Local vs Remote Frames.
I
> don't have the whole vision of how Fullscreen work contrary to you so I might
be
> naïve :)

Absolutely, this isn't solving the OOPIF problems, and in the CL following this
I'm even adding more TODOs for it. It is precisely because of these kinds of
problems that the global queue model is attractive.

As long as I don't make the problem worse, I intent to prioritize shipping
unprefixed fullscreen, as even when OOPIF is enabled, the breakage will be very
subtle. For example, in that future CL we will exit all the way out of
fullscreen if the top frame had two elements on its fullscreen element stack
instead of just popping the first of them. But most usage of fullscreen is
incredibly simple, with just a single element involved.

Thanks for the review!

[foolip, 2016-12-06 14:09:07]

The CQ bit was checked by foolip@chromium.org

[foolip, 2016-12-06 14:09:07]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2550703002/#ps100001
(title: "tests and documentation")

[commit-bot: I haz the power, 2016-12-06 14:09:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 14:13:56]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481033347080980,
"parent_rev": "a150a208fe7d824e61f4496a465e344cdc74e29d", "commit_rev":
"1603e64903156cec6bb5b0da81b0ea83f1566d37"}

[commit-bot: I haz the power, 2016-12-06 14:14:19]

Description was changed from

==========
Move pending state from FullscreenController to Fullscreen

The precise details of how competing enter+exit requests should work is
currently an ongoing discussion:
https://github.com/whatwg/fullscreen/issues/63
https://github.com/whatwg/fullscreen/pull/64
https://github.com/w3c/web-platform-tests/pull/4250

Previous behavior of FullscreenController:

enterFullscreenForElement() calls WebFrameClient::enterFullscreen()
unless we're already in fullscreen, transitioning in or out of
fullscreen, or if the request was for a cross-process descendant.
(m_fullscreenFrame is non-null also when transitioning out.) This
explains the "request then exit+request" behavior of simply exiting, as
no IPC to enter fullscreen is sent in this situation. However, it's not
obviously intentional, as it's covered by the "We are already in
fullscreen" bits.

exitFullscreen() calls WebFrameClient::exitFullscreen() unless
didExitFullscreen() is on the stack (m_isCancelingFullscreen). This
explains the "request+exit" behavior of "enters+exits quickly", as both
the enter and exit IPCs are sent.

Behavior is preserved as far as possible, with deliberate exception that
the "enters+exits quickly" behavior is no more, due to the added early
return in FullscreenController::exitFullscreen.

The fullyExitFullscreen call surrounded by m_isCancelingFullscreen was a
workaround, and is moved into Fullscreen::didExitFullscreen. Because
FullscreenController::exitFullscreen now does nothing if not in
fullscreen, m_isCancelingFullscreen or similar is no longer needed.

With no Member<>s left in FullscreenController, it can be owned as a
std::unique_ptr by WebViewImpl.

TEST=https://codereview.chromium.org/2541833002
BUG=402376
==========

to

==========
Move pending state from FullscreenController to Fullscreen

The precise details of how competing enter+exit requests should work is
currently an ongoing discussion:
https://github.com/whatwg/fullscreen/issues/63
https://github.com/whatwg/fullscreen/pull/64
https://github.com/w3c/web-platform-tests/pull/4250

Previous behavior of FullscreenController:

enterFullscreenForElement() calls WebFrameClient::enterFullscreen()
unless we're already in fullscreen, transitioning in or out of
fullscreen, or if the request was for a cross-process descendant.
(m_fullscreenFrame is non-null also when transitioning out.) This
explains the "request then exit+request" behavior of simply exiting, as
no IPC to enter fullscreen is sent in this situation. However, it's not
obviously intentional, as it's covered by the "We are already in
fullscreen" bits.

exitFullscreen() calls WebFrameClient::exitFullscreen() unless
didExitFullscreen() is on the stack (m_isCancelingFullscreen). This
explains the "request+exit" behavior of "enters+exits quickly", as both
the enter and exit IPCs are sent.

Behavior is preserved as far as possible, with deliberate exception that
the "enters+exits quickly" behavior is no more, due to the added early
return in FullscreenController::exitFullscreen.

The fullyExitFullscreen call surrounded by m_isCancelingFullscreen was a
workaround, and is moved into Fullscreen::didExitFullscreen. Because
FullscreenController::exitFullscreen now does nothing if not in
fullscreen, m_isCancelingFullscreen or similar is no longer needed.

With no Member<>s left in FullscreenController, it can be owned as a
std::unique_ptr by WebViewImpl.

TEST=https://codereview.chromium.org/2541833002
BUG=402376
==========

[commit-bot: I haz the power, 2016-12-06 14:14:20]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-06 14:18:11]

Description was changed from

==========
Move pending state from FullscreenController to Fullscreen

The precise details of how competing enter+exit requests should work is
currently an ongoing discussion:
https://github.com/whatwg/fullscreen/issues/63
https://github.com/whatwg/fullscreen/pull/64
https://github.com/w3c/web-platform-tests/pull/4250

Previous behavior of FullscreenController:

enterFullscreenForElement() calls WebFrameClient::enterFullscreen()
unless we're already in fullscreen, transitioning in or out of
fullscreen, or if the request was for a cross-process descendant.
(m_fullscreenFrame is non-null also when transitioning out.) This
explains the "request then exit+request" behavior of simply exiting, as
no IPC to enter fullscreen is sent in this situation. However, it's not
obviously intentional, as it's covered by the "We are already in
fullscreen" bits.

exitFullscreen() calls WebFrameClient::exitFullscreen() unless
didExitFullscreen() is on the stack (m_isCancelingFullscreen). This
explains the "request+exit" behavior of "enters+exits quickly", as both
the enter and exit IPCs are sent.

Behavior is preserved as far as possible, with deliberate exception that
the "enters+exits quickly" behavior is no more, due to the added early
return in FullscreenController::exitFullscreen.

The fullyExitFullscreen call surrounded by m_isCancelingFullscreen was a
workaround, and is moved into Fullscreen::didExitFullscreen. Because
FullscreenController::exitFullscreen now does nothing if not in
fullscreen, m_isCancelingFullscreen or similar is no longer needed.

With no Member<>s left in FullscreenController, it can be owned as a
std::unique_ptr by WebViewImpl.

TEST=https://codereview.chromium.org/2541833002
BUG=402376
==========

to

==========
Move pending state from FullscreenController to Fullscreen

The precise details of how competing enter+exit requests should work is
currently an ongoing discussion:
https://github.com/whatwg/fullscreen/issues/63
https://github.com/whatwg/fullscreen/pull/64
https://github.com/w3c/web-platform-tests/pull/4250

Previous behavior of FullscreenController:

enterFullscreenForElement() calls WebFrameClient::enterFullscreen()
unless we're already in fullscreen, transitioning in or out of
fullscreen, or if the request was for a cross-process descendant.
(m_fullscreenFrame is non-null also when transitioning out.) This
explains the "request then exit+request" behavior of simply exiting, as
no IPC to enter fullscreen is sent in this situation. However, it's not
obviously intentional, as it's covered by the "We are already in
fullscreen" bits.

exitFullscreen() calls WebFrameClient::exitFullscreen() unless
didExitFullscreen() is on the stack (m_isCancelingFullscreen). This
explains the "request+exit" behavior of "enters+exits quickly", as both
the enter and exit IPCs are sent.

Behavior is preserved as far as possible, with deliberate exception that
the "enters+exits quickly" behavior is no more, due to the added early
return in FullscreenController::exitFullscreen.

The fullyExitFullscreen call surrounded by m_isCancelingFullscreen was a
workaround, and is moved into Fullscreen::didExitFullscreen. Because
FullscreenController::exitFullscreen now does nothing if not in
fullscreen, m_isCancelingFullscreen or similar is no longer needed.

With no Member<>s left in FullscreenController, it can be owned as a
std::unique_ptr by WebViewImpl.

TEST=https://codereview.chromium.org/2541833002
BUG=402376

Committed: https://crrev.com/a2441f293224a5c5c0352f40c5e8fcbbf1a5a051
Cr-Commit-Position: refs/heads/master@{#436585}
==========

[commit-bot: I haz the power, 2016-12-06 14:18:12]

Patchset 6 (id:??) landed as
https://crrev.com/a2441f293224a5c5c0352f40c5e8fcbbf1a5a051
Cr-Commit-Position: refs/heads/master@{#436585}