Convert FullscreenController to use WebCallbacks

Convert FullscreenController to use WebCallbacks

https://github.com/whatwg/fullscreen/issues/63

Current 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.

The new behavior for entering is similar, except that while exiting, the
error callback is invoked. TODO: test other browsers.

The new behavior for exiting is to do nothing unless in fullscreen. This
fixes the "enters+exits quickly" behavior.

There is an asymetry between requesting and exiting fullscreen; only
Fullscreen::requestFullscreen can be used get into fullscreen in the
Fullscreen API sense, with a "fullscreen element" and so on. However,
the browser can exit at any time. This is why there are no callbacks for
WebFrameClient::exitFullscreen(), FullscreenController needs to know
which frames/document to notify anyway.

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.

BUG=402376

[foolip, 2016-11-21 20:41:06]

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

[commit-bot: I haz the power, 2016-11-21 20:41:52]

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-11-21 21:47:26]

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

[commit-bot: I haz the power, 2016-11-21 21:47:27]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[foolip, 2016-11-22 14:28:42]

Description was changed from

==========
Convert FullscreenController to use WebCallbacks

TODO: Ponder whether interleaves enter/exit calls matter of if this can
all be simplified. The original problem was that the prefixed/unprefixed
type had to be propagated to fire the correct events async after
requestFullscreen/exitFullscreen. bind() solves that problem.

Also, for exiting, is any callback really needed, or could frame
traversal solve the whole problem? Test if interleaved enter/exit are
short-circuited in Firefox and Edge.

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

to

==========
Convert FullscreenController to use WebCallbacks

https://github.com/whatwg/fullscreen/issues/63

Current 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" of simply exiting, as no IPC to
enter fullscreen is sent in this situation. However, it's no 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.

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

[foolip, 2016-11-22 15:44:00]

Description was changed from

==========
Convert FullscreenController to use WebCallbacks

https://github.com/whatwg/fullscreen/issues/63

Current 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" of simply exiting, as no IPC to
enter fullscreen is sent in this situation. However, it's no 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.

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

to

==========
Convert FullscreenController to use WebCallbacks

https://github.com/whatwg/fullscreen/issues/63

Current 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" 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.

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

[foolip, 2016-11-23 16:03:39]

Description was changed from

==========
Convert FullscreenController to use WebCallbacks

https://github.com/whatwg/fullscreen/issues/63

Current 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" 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.

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

to

==========
Convert FullscreenController to use WebCallbacks

https://github.com/whatwg/fullscreen/issues/63

Current 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" 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.

BUG=402376
==========

[foolip, 2016-11-23 16:10:23]

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

[commit-bot: I haz the power, 2016-11-23 16:10:54]

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-11-23 16:52:55]

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

[commit-bot: I haz the power, 2016-11-23 16:52:56]

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

[foolip, 2016-11-23 16:59:34]

Description was changed from

==========
Convert FullscreenController to use WebCallbacks

https://github.com/whatwg/fullscreen/issues/63

Current 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" 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.

BUG=402376
==========

to

==========
Convert FullscreenController to use WebCallbacks

https://github.com/whatwg/fullscreen/issues/63

Current 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.

The new behavior for entering is similar, except that while exiting, the
error callback is invoked. TODO: test other browsers.

The new behavior for exiting is to do nothing unless in fullscreen. This
fixes the "enters+exits quickly" behavior.

There is an assymetry between requesting and exiting fullscreen; only
Fullscreen::requestFullscreen can be used get into fullscreen in the
Fullscreen API sense, with a "fullscreen element" and so on. However,
the browser can exit at any time. This is why there are no callbacks
WebFrameClient::exitFullscreen(), FullscreenController needs to know
which frames/document to notify anyway.

Previously, FullscreenController::didExitFullscreen() would call
fullyExitFullscreen and didExitFullscreen for the innermost document.
Now, FullscreenController will notify only the topmost local frames. The
fullyExitFullscreen call is a workaround, and moved into
Fullscreen::didExitFullscreen. Because
FullscreenController::exitFullscreen now does nothing if not in
fullscreen, m_isCancelingFullscreen is no longer needed.

When entering fullscreen in an iframe, m_currentFullScreenElement is
only set for the innermost document. This has not changed, but combined
with the aforementioned changes, it required some changes to the logic
for starting event timers in Fullscreen::didExitFullscreen.

BUG=402376
==========

[foolip, 2016-11-23 21:22:31]

Description was changed from

==========
Convert FullscreenController to use WebCallbacks

https://github.com/whatwg/fullscreen/issues/63

Current 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.

The new behavior for entering is similar, except that while exiting, the
error callback is invoked. TODO: test other browsers.

The new behavior for exiting is to do nothing unless in fullscreen. This
fixes the "enters+exits quickly" behavior.

There is an assymetry between requesting and exiting fullscreen; only
Fullscreen::requestFullscreen can be used get into fullscreen in the
Fullscreen API sense, with a "fullscreen element" and so on. However,
the browser can exit at any time. This is why there are no callbacks
WebFrameClient::exitFullscreen(), FullscreenController needs to know
which frames/document to notify anyway.

Previously, FullscreenController::didExitFullscreen() would call
fullyExitFullscreen and didExitFullscreen for the innermost document.
Now, FullscreenController will notify only the topmost local frames. The
fullyExitFullscreen call is a workaround, and moved into
Fullscreen::didExitFullscreen. Because
FullscreenController::exitFullscreen now does nothing if not in
fullscreen, m_isCancelingFullscreen is no longer needed.

When entering fullscreen in an iframe, m_currentFullScreenElement is
only set for the innermost document. This has not changed, but combined
with the aforementioned changes, it required some changes to the logic
for starting event timers in Fullscreen::didExitFullscreen.

BUG=402376
==========

to

==========
Convert FullscreenController to use WebCallbacks

https://github.com/whatwg/fullscreen/issues/63

Current 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.

The new behavior for entering is similar, except that while exiting, the
error callback is invoked. TODO: test other browsers.

The new behavior for exiting is to do nothing unless in fullscreen. This
fixes the "enters+exits quickly" behavior.

There is an assymetry between requesting and exiting fullscreen; only
Fullscreen::requestFullscreen can be used get into fullscreen in the
Fullscreen API sense, with a "fullscreen element" and so on. However,
the browser can exit at any time. This is why there are no callbacks
WebFrameClient::exitFullscreen(), FullscreenController needs to know
which frames/document to notify anyway.

Previously, FullscreenController::didExitFullscreen() would call
fullyExitFullscreen and didExitFullscreen for the innermost document.
Now, FullscreenController will notify only the topmost local frames. The
fullyExitFullscreen call is a workaround, and moved into
Fullscreen::didExitFullscreen. Because
FullscreenController::exitFullscreen now does nothing if not in
fullscreen, m_isCancelingFullscreen is no longer needed.

BUG=402376
==========

[foolip, 2016-11-23 21:25:19]

Description was changed from

==========
Convert FullscreenController to use WebCallbacks

https://github.com/whatwg/fullscreen/issues/63

Current 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.

The new behavior for entering is similar, except that while exiting, the
error callback is invoked. TODO: test other browsers.

The new behavior for exiting is to do nothing unless in fullscreen. This
fixes the "enters+exits quickly" behavior.

There is an assymetry between requesting and exiting fullscreen; only
Fullscreen::requestFullscreen can be used get into fullscreen in the
Fullscreen API sense, with a "fullscreen element" and so on. However,
the browser can exit at any time. This is why there are no callbacks
WebFrameClient::exitFullscreen(), FullscreenController needs to know
which frames/document to notify anyway.

Previously, FullscreenController::didExitFullscreen() would call
fullyExitFullscreen and didExitFullscreen for the innermost document.
Now, FullscreenController will notify only the topmost local frames. The
fullyExitFullscreen call is a workaround, and moved into
Fullscreen::didExitFullscreen. Because
FullscreenController::exitFullscreen now does nothing if not in
fullscreen, m_isCancelingFullscreen is no longer needed.

BUG=402376
==========

to

==========
Convert FullscreenController to use WebCallbacks

https://github.com/whatwg/fullscreen/issues/63

Current 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.

The new behavior for entering is similar, except that while exiting, the
error callback is invoked. TODO: test other browsers.

The new behavior for exiting is to do nothing unless in fullscreen. This
fixes the "enters+exits quickly" behavior.

There is an assymetry between requesting and exiting fullscreen; only
Fullscreen::requestFullscreen can be used get into fullscreen in the
Fullscreen API sense, with a "fullscreen element" and so on. However,
the browser can exit at any time. This is why there are no callbacks
for WebFrameClient::exitFullscreen(), FullscreenController needs to know
which frames/document to notify anyway.

Previously, FullscreenController::didExitFullscreen() would call
fullyExitFullscreen and didExitFullscreen for the innermost document.
Now, FullscreenController will notify only the topmost local frames. The
fullyExitFullscreen call is a workaround, and moved into
Fullscreen::didExitFullscreen. Because
FullscreenController::exitFullscreen now does nothing if not in
fullscreen, m_isCancelingFullscreen is no longer needed.

BUG=402376
==========

[foolip, 2016-11-23 21:27:22]

Description was changed from

==========
Convert FullscreenController to use WebCallbacks

https://github.com/whatwg/fullscreen/issues/63

Current 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.

The new behavior for entering is similar, except that while exiting, the
error callback is invoked. TODO: test other browsers.

The new behavior for exiting is to do nothing unless in fullscreen. This
fixes the "enters+exits quickly" behavior.

There is an assymetry between requesting and exiting fullscreen; only
Fullscreen::requestFullscreen can be used get into fullscreen in the
Fullscreen API sense, with a "fullscreen element" and so on. However,
the browser can exit at any time. This is why there are no callbacks
for WebFrameClient::exitFullscreen(), FullscreenController needs to know
which frames/document to notify anyway.

Previously, FullscreenController::didExitFullscreen() would call
fullyExitFullscreen and didExitFullscreen for the innermost document.
Now, FullscreenController will notify only the topmost local frames. The
fullyExitFullscreen call is a workaround, and moved into
Fullscreen::didExitFullscreen. Because
FullscreenController::exitFullscreen now does nothing if not in
fullscreen, m_isCancelingFullscreen is no longer needed.

BUG=402376
==========

to

==========
Convert FullscreenController to use WebCallbacks

https://github.com/whatwg/fullscreen/issues/63

Current 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.

The new behavior for entering is similar, except that while exiting, the
error callback is invoked. TODO: test other browsers.

The new behavior for exiting is to do nothing unless in fullscreen. This
fixes the "enters+exits quickly" behavior.

There is an asymetry between requesting and exiting fullscreen; only
Fullscreen::requestFullscreen can be used get into fullscreen in the
Fullscreen API sense, with a "fullscreen element" and so on. However,
the browser can exit at any time. This is why there are no callbacks for
WebFrameClient::exitFullscreen(), FullscreenController needs to know
which frames/document to notify anyway.

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.

BUG=402376
==========

[foolip, 2016-11-23 21:27:49]

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

[commit-bot: I haz the power, 2016-11-23 21:28:38]

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

[foolip, 2016-11-23 22:17:35]

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

[commit-bot: I haz the power, 2016-11-23 22:18:42]

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-11-23 23:21:31]

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

[commit-bot: I haz the power, 2016-11-23 23:21:33]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[foolip, 2016-11-24 17:09:23]

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

[commit-bot: I haz the power, 2016-11-24 17:09:46]

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-11-24 18:16:02]

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

[commit-bot: I haz the power, 2016-11-24 18:16:03]

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

[foolip, 2016-11-24 21:30:55]

foolip@chromium.org changed reviewers:
+ esprehn@chromium.org

[foolip, 2016-11-24 21:30:56]

esprehn@, this isn't ready for review yet (needs spec changes and tests) but
could you advise on the layering side? Is using WebCallbacks in this fashion
acceptable, or is there something that would be more idiomatic?

The fundamental reason for keeping a queue of callbacks on FullscreenController
is that it's the only way to maintain the order of requests across documents.
This is actually racy in the spec now, but the order is observable and handling
in the same order as requested is the most obvious choice. Again, spec changes
required.

[foolip, 2016-11-29 14:15:17]

On 2016/11/24 21:30:56, foolip wrote:
> esprehn@, this isn't ready for review yet (needs spec changes and tests) but
> could you advise on the layering side? Is using WebCallbacks in this fashion
> acceptable, or is there something that would be more idiomatic?
> 
> The fundamental reason for keeping a queue of callbacks on
FullscreenController
> is that it's the only way to maintain the order of requests across documents.
> This is actually racy in the spec now, but the order is observable and
handling
> in the same order as requested is the most obvious choice. Again, spec changes
> required.

Friendly ping :)

[mlamouri (slow - plz ping), 2016-11-30 09:24:55]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-11-30 09:24:57]

drive-by review because it caught my attention :)

https://codereview.chromium.org/2495423004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2495423004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:424: wrapUnique(new
FullscreenCallbacks(
Any particular reason why not keepinp track of `document` and `element` in
`FullscreenCallbacks` and call the right method instead of creating two bind?

https://codereview.chromium.org/2495423004/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebFullscreenCallbacks.h (right):

https://codereview.chromium.org/2495423004/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebFullscreenCallbacks.h:12: using
WebFullscreenCallbacks = WebCallbacks<void, void>;
You are adding this to public/platform/ but you don't seem to use it outside of
Blink. What about exposing FullscreenCallbacks in core/ instead? It sounds that
the current users will be able to access the header (from core/ and web/) and it
will be slightly more straightforward.

[foolip, 2016-11-30 10:23:53]

Thanks for the drive-by mlamouri@! I'll poke you again when this is ready for
final-ish review. (Still blocked on figuring out exactly what the spec should
say.)

https://codereview.chromium.org/2495423004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2495423004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:424: wrapUnique(new
FullscreenCallbacks(
On 2016/11/30 09:24:57, mlamouri (OOO until 29-11) wrote:
> Any particular reason why not keepinp track of `document` and `element` in
> `FullscreenCallbacks` and call the right method instead of creating two bind?

This is a wart from before when I was using callbacks for exit as well, and I
didn't simplify. I'll see about having FullscreenCallbacks have
Persistent<Document>, Persistent<Element> and a RequestType instead. (One
original reason for this refactoring was that I didn't want to make
FullscreenController keep track of three things like that.)

https://codereview.chromium.org/2495423004/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebFullscreenCallbacks.h (right):

https://codereview.chromium.org/2495423004/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebFullscreenCallbacks.h:12: using
WebFullscreenCallbacks = WebCallbacks<void, void>;
On 2016/11/30 09:24:57, mlamouri (OOO until 29-11) wrote:
> You are adding this to public/platform/ but you don't seem to use it outside
of
> Blink. What about exposing FullscreenCallbacks in core/ instead? It sounds
that
> the current users will be able to access the header (from core/ and web/) and
it
> will be slightly more straightforward.

Thanks, that's exactly the kind of feedback I was looking for. Maybe a
Fullscreen::PendingRequest or something, I'll experiment a bit.

[foolip, 2016-11-30 18:37:58]

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

[commit-bot: I haz the power, 2016-11-30 18:38:37]

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-11-30 19:18:17]

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

[commit-bot: I haz the power, 2016-11-30 19:18:18]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mlamouri (slow - plz ping), 2016-12-01 10:31:03]

https://codereview.chromium.org/2495423004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/FullscreenCallbacks.h (right):

https://codereview.chromium.org/2495423004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/FullscreenCallbacks.h:10: class
FullscreenCallbacks {
Any reason why you don't make this RequestFullscreenCallbacks instead of having
this interface that is used only once?

[foolip, 2016-12-01 10:52:59]

https://codereview.chromium.org/2495423004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/FullscreenCallbacks.h (right):

https://codereview.chromium.org/2495423004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/FullscreenCallbacks.h:10: class
FullscreenCallbacks {
On 2016/12/01 10:31:03, mlamouri wrote:
> Any reason why you don't make this RequestFullscreenCallbacks instead of
having
> this interface that is used only once?

A remnant from when I used callbacks also for exiting. I may have to put that
back, but if not I'll simplify as suggested.

[esprehn, 2016-12-01 20:26:12]

Is there a reason to not use WebCallbacks<void, void> here? Your interface is
just a duplicate of it.

https://codereview.chromium.org/2495423004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2495423004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:293: return wrapUnique(new
RequestFullscreenCallbacks(Fullscreen::from(document),
The element and the document can disagree, is there a reason to not do
element.document() here?

https://codereview.chromium.org/2495423004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/2495423004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FullscreenController.cpp:93: for (auto& callbacks
: callbacksList)
Can we just move all of this logic down into core instead? We want to make web/
smaller overall, so adding the callbacks is worse than just moving this entire
function body into core. We can do this if needed though.

[foolip, 2016-12-01 23:05:01]

Thanks for taking a look, esprehn@! It's still a bit open ended what the right
path is here and the spec still malleable, so any ideas much appreciated.

> Is there a reason to not use WebCallbacks<void, void> here? Your interface is
just a duplicate of it.

It started out as WebCallbacks<void, void>, changed after mlamouri@'s comments.

https://codereview.chromium.org/2495423004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2495423004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:293: return wrapUnique(new
RequestFullscreenCallbacks(Fullscreen::from(document),
On 2016/12/01 20:26:12, esprehn wrote:
> The element and the document can disagree, is there a reason to not do
> element.document() here?

I was working on this today, just uploaded "handle (poorly) the element being
moved after request" which changed this to use the document. (Fullscreen is a
Supplement<Document> so holding onto either is fine, but I want to make it not a
supplement eventually.)

element.document() is at the top of Fullscreen::requestFullscreen, so I could
take just an element here or assert that they're the same. Will see.

https://codereview.chromium.org/2495423004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/2495423004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FullscreenController.cpp:93: for (auto& callbacks
: callbacksList)
On 2016/12/01 20:26:12, esprehn wrote:
> Can we just move all of this logic down into core instead? We want to make
web/
> smaller overall, so adding the callbacks is worse than just moving this entire
> function body into core. We can do this if needed though.

All of FullscreenController? In order to handle fullscreen requests in different
iframes in the same order as they were made, there needs to be a queue
elsewhere. It's easy to write tests for it, by a click event handler calling
iframe2.contentDocument.body.requestFullscreen() and
iframe1...requestFullscreen(), and expecting that iframe1 will be the visible
one. (This would have to be after fixing https://627792) 

However, it's incredibly unlikely to be constrained by web compat, and I'm
currently trying to pick a sane model for
https://github.com/whatwg/fullscreen/issues/63 and could do something where the
implementation would be very different.

My top candidate so far was a global queue of requests and exits, where
everything is handled in the order the requests were made, but with some
short-circuiting when requests and exits are both on the queue to avoid the
"enters+exits quickly" behavior we currently have.

Another model would be for each document to have its own queue, and for
FullscreenController to instead just notify all documents in frame tree order.
(The same order as animation frame callbacks are invoked.) This would be
observably different, but probably just fine. We'd still need to keep some of
the m_state stuff in FullscreenController to avoid the "enters+exits quickly"
behavior.

[foolip, 2016-12-01 23:15:19]

Also note that much of this machinery is pretty silly until the timing is fixed
properly. I have a long series of patches leading up to the top layer rewrite.
Lots of TODOs, but if you'd like to see the direction I could upload them all so
you can see where this is going.

[mlamouri (slow - plz ping), 2016-12-02 10:45:09]

On 2016/12/01 at 23:05:01, foolip wrote:
> Thanks for taking a look, esprehn@! It's still a bit open ended what the right
path is here and the spec still malleable, so any ideas much appreciated.
> 
> > Is there a reason to not use WebCallbacks<void, void> here? Your interface
is just a duplicate of it.
> 
> It started out as WebCallbacks<void, void>, changed after mlamouri@'s
comments.

To clarify, I mostly suggested not to have a FullscreenCallbacks exposed to
public/. In the current state of affairs, the FullscreenCallbacks interface
isn't needed but as you pointed, it sounds that you need to finalize the rest of
the work.

[foolip, 2016-12-02 17:04:51]

Closing this in favor of https://codereview.chromium.org/2495423004/, which
shares large parts of this CL but has per-Fullscreen supplement queues instead
of one queue in FullscreenController. Observably different, but I think I can
spec it.

[foolip, 2016-12-02 17:05:50]

On 2016/12/02 17:04:51, foolip wrote:
> Closing this in favor of https://codereview.chromium.org/2495423004/, which
> shares large parts of this CL but has per-Fullscreen supplement queues instead
> of one queue in FullscreenController. Observably different, but I think I can
> spec it.

Oops: https://codereview.chromium.org/2550703002/

[foolip, 2016-12-06 21:26:41]

Just in case it's needed, PS10 is rebased on top of
https://codereview.chromium.org/2495423004/ to use callbacks instead. It affects
one test, in a way that matches Firefox, but is still not great.