Issue 1914643005: Add support for entering/exiting HTML fullscreen from OOPIFs.

alexmos

Description was changed from ========== [DO NOT LAND] Fullscreen experiments BUG= ========== to ========== [DO ...

4 years, 8 months ago (2016-04-23 01:56:18 UTC) #1

alexmos

Description was changed from ========== [DO NOT LAND] Fullscreen experiments BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== ...

4 years, 7 months ago (2016-05-10 00:48:46 UTC) #2

alexmos

Description was changed from ========== Add support for entering/exiting HTML fullscreen from OOPIFs. BUG=550497 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ...

4 years, 7 months ago (2016-05-10 00:49:23 UTC) #3

alexmos

Description was changed from ========== Add support for entering/exiting HTML fullscreen from OOPIFs. BUG=550497 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ...

4 years, 7 months ago (2016-05-10 20:42:37 UTC) #4

Description was changed from

==========
Add support for entering/exiting HTML fullscreen from OOPIFs.

BUG=550497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add support for entering/exiting HTML fullscreen from OOPIFs.

The existing workflow for entering HTML fullscreen goes like this:
1. JS on the page calls element.webkitRequestFullscreen()
2. Fullscreen::requestFullscreen():
   a. Checks if fullscreen is allowed by ancestors (allowFullscreen)
   b. Creates fullscreenchange events for |element| and its ancestor
      <iframes> and saves them in its m_eventQueue.
   c. Puts |element| on its fullscreen element stack.
3. FullscreenController::enterFullScreenForElement(element)
   a. Saves |element| as provisional fullscreen element.
   b. Saves some page sizing info (to be restored after fullscreen)
4. RenderFrameImpl::enterFullscreen sends a FrameHostMsg_ToggleFullscreen
   to browser process.
5. RenderFrameHostImpl::OnToggleFullscreen() asks the current tab to
   enter fullscreen.
6. After the tab is resized for fullscreen, browser process generates a
   ViewMsg_Resize to the main frame renderer, with
   is_fullscreen_granted=1 in ResizeParams.
7. RenderWidget::Resize realizes there's a fullscreen change and calls
   into FullscreenController::didEnterFullScreen().
8. FullscreenController::didEnterFullScreen picks up the stored
   provisional fullscreen element and calls
   Fullscreen::didEnterFullScreenForElement on it.
9. Fullscreen::didEnterFullScreenForElement(element):
   a. Adjusts layout on |element| for fullscreen
   b. Sets fullscreen CSS styles on the fullscreen element and all
      ancestor elements.
   c. Fires fullscreenchange events from step 2b.

This CL takes the first step to make this flow OOPIF-friendly.  The main
changes are:

- in step 5, before entering fullscreen for tab, we will send an IPC
  to ancestor OOPIFs so that they do steps 2 and 3 for their
  respective ancestor <iframes> (i.e., set up provisional fullscreen
  elements and prepare their share of the fullscreenchange events).
- ViewMsg_Resize will trigger fullscreen in subframe widgets in
  addition to main frame ones.  When fullscreen is entered,
  ViewMsg_Resize is sent to all widgets on the page.  WebFrameWidgets
  now also support picking up the fullscreen change and calling into
  the page's FullscreenController.
- blink::Fullscreen and FullscreenController now support entering
  fullscreen for <iframe> elements which contain out-of-process
  fullscreen elements.

There are still various issues to be dealt with in followup CLs:
- further refactoring ancestor walks in Fullscreen::requestFullscreen
  and exitFullscreen to be compatible with hierarchies with a
  RemoteFrame between LocalFrames (such as A-B-A).  This is needed to
  fire all ancestor fullscreenchange events properly.
- dealing with nested fullscreen elements.
- optimizing fullscreen layout in ancestor processes.
- correcting background color when fullscreening an OOP <iframe>
  element.

BUG=550497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

alexmos

Description was changed from ========== Add support for entering/exiting HTML fullscreen from OOPIFs. The existing ...

4 years, 7 months ago (2016-05-10 20:43:27 UTC) #5

Description was changed from

==========
Add support for entering/exiting HTML fullscreen from OOPIFs.

The existing workflow for entering HTML fullscreen goes like this:
1. JS on the page calls element.webkitRequestFullscreen()
2. Fullscreen::requestFullscreen():
   a. Checks if fullscreen is allowed by ancestors (allowFullscreen)
   b. Creates fullscreenchange events for |element| and its ancestor
      <iframes> and saves them in its m_eventQueue.
   c. Puts |element| on its fullscreen element stack.
3. FullscreenController::enterFullScreenForElement(element)
   a. Saves |element| as provisional fullscreen element.
   b. Saves some page sizing info (to be restored after fullscreen)
4. RenderFrameImpl::enterFullscreen sends a FrameHostMsg_ToggleFullscreen
   to browser process.
5. RenderFrameHostImpl::OnToggleFullscreen() asks the current tab to
   enter fullscreen.
6. After the tab is resized for fullscreen, browser process generates a
   ViewMsg_Resize to the main frame renderer, with
   is_fullscreen_granted=1 in ResizeParams.
7. RenderWidget::Resize realizes there's a fullscreen change and calls
   into FullscreenController::didEnterFullScreen().
8. FullscreenController::didEnterFullScreen picks up the stored
   provisional fullscreen element and calls
   Fullscreen::didEnterFullScreenForElement on it.
9. Fullscreen::didEnterFullScreenForElement(element):
   a. Adjusts layout on |element| for fullscreen
   b. Sets fullscreen CSS styles on the fullscreen element and all
      ancestor elements.
   c. Fires fullscreenchange events from step 2b.

This CL takes the first step to make this flow OOPIF-friendly.  The main
changes are:

- in step 5, before entering fullscreen for tab, we will send an IPC
  to ancestor OOPIFs so that they do steps 2 and 3 for their
  respective ancestor <iframes> (i.e., set up provisional fullscreen
  elements and prepare their share of the fullscreenchange events).
- ViewMsg_Resize will trigger fullscreen in subframe widgets in
  addition to main frame ones.  When fullscreen is entered,
  ViewMsg_Resize is sent to all widgets on the page.  WebFrameWidgets
  now also support picking up the fullscreen change and calling into
  the page's FullscreenController.
- blink::Fullscreen and FullscreenController now support entering
  fullscreen for <iframe> elements which contain out-of-process
  fullscreen elements.

There are still various issues to be dealt with in followup CLs:
- further refactoring ancestor walks in Fullscreen::requestFullscreen
  and exitFullscreen to be compatible with hierarchies with a
  RemoteFrame between LocalFrames (such as A-B-A).  This is needed to
  fire all ancestor fullscreenchange events properly.
- dealing with nested fullscreen elements.
- optimizing fullscreen layout in ancestor processes.
- correcting background color when fullscreening an OOP <iframe>
  element.

BUG=550497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add support for entering/exiting HTML fullscreen from OOPIFs.

The existing flow for entering HTML fullscreen goes like this:
1. JS on the page calls element.webkitRequestFullscreen()
2. Fullscreen::requestFullscreen():
   a. Checks if fullscreen is allowed by ancestors (allowFullscreen)
   b. Creates fullscreenchange events for |element| and its ancestor
      <iframes> and saves them in its m_eventQueue.
   c. Puts |element| on its fullscreen element stack.
3. FullscreenController::enterFullScreenForElement(element)
   a. Saves |element| as provisional fullscreen element.
   b. Saves some page sizing info (to be restored after fullscreen)
4. RenderFrameImpl::enterFullscreen sends a FrameHostMsg_ToggleFullscreen
   to browser process.
5. RenderFrameHostImpl::OnToggleFullscreen() asks the current tab to
   enter fullscreen.
6. After the tab is resized for fullscreen, browser process generates a
   ViewMsg_Resize to the main frame renderer, with
   is_fullscreen_granted=1 in ResizeParams.
7. RenderWidget::Resize realizes there's a fullscreen change and calls
   into FullscreenController::didEnterFullScreen().
8. FullscreenController::didEnterFullScreen picks up the stored
   provisional fullscreen element and calls
   Fullscreen::didEnterFullScreenForElement on it.
9. Fullscreen::didEnterFullScreenForElement(element):
   a. Adjusts layout on |element| for fullscreen
   b. Sets fullscreen CSS styles on the fullscreen element and all
      ancestor elements.
   c. Fires fullscreenchange events from step 2b.

This CL takes the first step to make this flow OOPIF-friendly.  The main
changes are:

- in step 5, before entering fullscreen for tab, we will send an IPC
  to ancestor OOPIFs so that they do steps 2 and 3 for their
  respective ancestor <iframes> (i.e., set up provisional fullscreen
  elements and prepare their share of the fullscreenchange events).
- ViewMsg_Resize will trigger fullscreen in subframe widgets in
  addition to main frame ones.  When fullscreen is entered,
  ViewMsg_Resize is sent to all widgets on the page.  WebFrameWidgets
  now also support picking up the fullscreen change and calling into
  the page's FullscreenController.
- blink::Fullscreen and FullscreenController now support entering
  fullscreen for <iframe> elements which contain out-of-process
  fullscreen elements.

There are still various issues to be dealt with in followup CLs:
- further refactoring ancestor walks in Fullscreen::requestFullscreen
  and exitFullscreen to be compatible with hierarchies with a
  RemoteFrame between LocalFrames (such as A-B-A).  This is needed to
  fire all ancestor fullscreenchange events properly.
- dealing with nested fullscreen elements.
- optimizing fullscreen layout in ancestor processes.
- correcting background color when fullscreening an OOP <iframe>
  element.

BUG=550497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

alexmos

Description was changed from ========== Add support for entering/exiting HTML fullscreen from OOPIFs. The existing ...

4 years, 7 months ago (2016-05-10 21:05:06 UTC) #6

Description was changed from

==========
Add support for entering/exiting HTML fullscreen from OOPIFs.

The existing flow for entering HTML fullscreen goes like this:
1. JS on the page calls element.webkitRequestFullscreen()
2. Fullscreen::requestFullscreen():
   a. Checks if fullscreen is allowed by ancestors (allowFullscreen)
   b. Creates fullscreenchange events for |element| and its ancestor
      <iframes> and saves them in its m_eventQueue.
   c. Puts |element| on its fullscreen element stack.
3. FullscreenController::enterFullScreenForElement(element)
   a. Saves |element| as provisional fullscreen element.
   b. Saves some page sizing info (to be restored after fullscreen)
4. RenderFrameImpl::enterFullscreen sends a FrameHostMsg_ToggleFullscreen
   to browser process.
5. RenderFrameHostImpl::OnToggleFullscreen() asks the current tab to
   enter fullscreen.
6. After the tab is resized for fullscreen, browser process generates a
   ViewMsg_Resize to the main frame renderer, with
   is_fullscreen_granted=1 in ResizeParams.
7. RenderWidget::Resize realizes there's a fullscreen change and calls
   into FullscreenController::didEnterFullScreen().
8. FullscreenController::didEnterFullScreen picks up the stored
   provisional fullscreen element and calls
   Fullscreen::didEnterFullScreenForElement on it.
9. Fullscreen::didEnterFullScreenForElement(element):
   a. Adjusts layout on |element| for fullscreen
   b. Sets fullscreen CSS styles on the fullscreen element and all
      ancestor elements.
   c. Fires fullscreenchange events from step 2b.

This CL takes the first step to make this flow OOPIF-friendly.  The main
changes are:

- in step 5, before entering fullscreen for tab, we will send an IPC
  to ancestor OOPIFs so that they do steps 2 and 3 for their
  respective ancestor <iframes> (i.e., set up provisional fullscreen
  elements and prepare their share of the fullscreenchange events).
- ViewMsg_Resize will trigger fullscreen in subframe widgets in
  addition to main frame ones.  When fullscreen is entered,
  ViewMsg_Resize is sent to all widgets on the page.  WebFrameWidgets
  now also support picking up the fullscreen change and calling into
  the page's FullscreenController.
- blink::Fullscreen and FullscreenController now support entering
  fullscreen for <iframe> elements which contain out-of-process
  fullscreen elements.

There are still various issues to be dealt with in followup CLs:
- further refactoring ancestor walks in Fullscreen::requestFullscreen
  and exitFullscreen to be compatible with hierarchies with a
  RemoteFrame between LocalFrames (such as A-B-A).  This is needed to
  fire all ancestor fullscreenchange events properly.
- dealing with nested fullscreen elements.
- optimizing fullscreen layout in ancestor processes.
- correcting background color when fullscreening an OOP <iframe>
  element.

BUG=550497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add support for entering/exiting HTML fullscreen from OOPIFs.

The existing flow for entering HTML fullscreen goes like this:
1. JS on the page calls element.webkitRequestFullscreen()
2. Fullscreen::requestFullscreen():
   a. Checks if fullscreen is allowed by ancestors (allowFullscreen)
   b. Creates fullscreenchange events for |element| and its ancestor
      <iframes> and saves them in its m_eventQueue.
   c. Puts |element| on its fullscreen element stack.
3. FullscreenController::enterFullScreenForElement(element)
   a. Saves |element| as provisional fullscreen element.
   b. Saves some page sizing info (to be restored after fullscreen)
4. RenderFrameImpl::enterFullscreen sends a FrameHostMsg_ToggleFullscreen
   to browser process.
5. RenderFrameHostImpl::OnToggleFullscreen() asks the current tab to
   enter fullscreen.
6. After the tab is resized for fullscreen, browser process generates a
   ViewMsg_Resize to the main frame renderer, with
   is_fullscreen_granted=1 in ResizeParams.
7. RenderWidget::Resize realizes there's a fullscreen change and calls
   into FullscreenController::didEnterFullScreen().
8. FullscreenController::didEnterFullScreen picks up the stored
   provisional fullscreen element and calls
   Fullscreen::didEnterFullScreenForElement on it.
9. Fullscreen::didEnterFullScreenForElement(element):
   a. Adjusts layout on |element| for fullscreen
   b. Sets fullscreen CSS styles on the fullscreen element and all
      ancestor elements.
   c. Fires fullscreenchange events from step 2b.

This CL takes the first step to make this flow OOPIF-friendly.  The main
changes are:

- in step 5, before entering fullscreen for tab, we will send an IPC
  to ancestor OOPIFs so that they do steps 2 and 3 for their
  respective ancestor <iframes> (i.e., set up provisional fullscreen
  elements and prepare their share of the fullscreenchange events).
- ViewMsg_Resize will trigger fullscreen in subframe widgets in
  addition to main frame ones.  When fullscreen is entered,
  ViewMsg_Resize is sent to all widgets on the page.  WebFrameWidgets
  now also support picking up the fullscreen change and calling into
  the page's FullscreenController.
- blink::Fullscreen and FullscreenController now support entering
  fullscreen for <iframe> elements which contain out-of-process
  fullscreen elements.

There are still various issues to be dealt with in followup CLs:
- further refactoring ancestor walks in Fullscreen::requestFullscreen
  and exitFullscreen to be compatible with hierarchies with a
  RemoteFrame between LocalFrames (such as A-B-A).  This is needed to
  fire all ancestor fullscreenchange events properly.
- dealing with nested fullscreen elements.
- optimizing fullscreen layout in ancestor processes.
- correcting background color when fullscreening an OOP <iframe>
  element.

More info in design doc: https://goo.gl/Y5wdqi

BUG=550497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

alexmos

Description was changed from ========== Add support for entering/exiting HTML fullscreen from OOPIFs. The existing ...

4 years, 7 months ago (2016-05-10 21:32:28 UTC) #7

Description was changed from

==========
Add support for entering/exiting HTML fullscreen from OOPIFs.

The existing flow for entering HTML fullscreen goes like this:
1. JS on the page calls element.webkitRequestFullscreen()
2. Fullscreen::requestFullscreen():
   a. Checks if fullscreen is allowed by ancestors (allowFullscreen)
   b. Creates fullscreenchange events for |element| and its ancestor
      <iframes> and saves them in its m_eventQueue.
   c. Puts |element| on its fullscreen element stack.
3. FullscreenController::enterFullScreenForElement(element)
   a. Saves |element| as provisional fullscreen element.
   b. Saves some page sizing info (to be restored after fullscreen)
4. RenderFrameImpl::enterFullscreen sends a FrameHostMsg_ToggleFullscreen
   to browser process.
5. RenderFrameHostImpl::OnToggleFullscreen() asks the current tab to
   enter fullscreen.
6. After the tab is resized for fullscreen, browser process generates a
   ViewMsg_Resize to the main frame renderer, with
   is_fullscreen_granted=1 in ResizeParams.
7. RenderWidget::Resize realizes there's a fullscreen change and calls
   into FullscreenController::didEnterFullScreen().
8. FullscreenController::didEnterFullScreen picks up the stored
   provisional fullscreen element and calls
   Fullscreen::didEnterFullScreenForElement on it.
9. Fullscreen::didEnterFullScreenForElement(element):
   a. Adjusts layout on |element| for fullscreen
   b. Sets fullscreen CSS styles on the fullscreen element and all
      ancestor elements.
   c. Fires fullscreenchange events from step 2b.

This CL takes the first step to make this flow OOPIF-friendly.  The main
changes are:

- in step 5, before entering fullscreen for tab, we will send an IPC
  to ancestor OOPIFs so that they do steps 2 and 3 for their
  respective ancestor <iframes> (i.e., set up provisional fullscreen
  elements and prepare their share of the fullscreenchange events).
- ViewMsg_Resize will trigger fullscreen in subframe widgets in
  addition to main frame ones.  When fullscreen is entered,
  ViewMsg_Resize is sent to all widgets on the page.  WebFrameWidgets
  now also support picking up the fullscreen change and calling into
  the page's FullscreenController.
- blink::Fullscreen and FullscreenController now support entering
  fullscreen for <iframe> elements which contain out-of-process
  fullscreen elements.

There are still various issues to be dealt with in followup CLs:
- further refactoring ancestor walks in Fullscreen::requestFullscreen
  and exitFullscreen to be compatible with hierarchies with a
  RemoteFrame between LocalFrames (such as A-B-A).  This is needed to
  fire all ancestor fullscreenchange events properly.
- dealing with nested fullscreen elements.
- optimizing fullscreen layout in ancestor processes.
- correcting background color when fullscreening an OOP <iframe>
  element.

More info in design doc: https://goo.gl/Y5wdqi

BUG=550497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add support for entering/exiting HTML fullscreen from OOPIFs.

Currently, entering HTML fullscreen utilizes a number of ancestor
frame walks.  This is because entering fullscreen for an element (1)
alters layout for all its ancestor <iframes> (by applying
:-webkit-full-screen styles to them, which applies some UA CSS rules),
(2) fires fullscreenchange in all ancestor documents on <iframe>
elements along the ancestor chain, and (3) makes
document.webkitFullscreenElement return the <iframe> element
containing the fullscreened element in ancestor frames.

Currently, the logic behind this assumes that all ancestor frames are
local and does nothing when they encounter a remote frame.  This CL
takes the first step to fix this.

The existing flow for entering HTML fullscreen goes like this:
1. JS on the page calls element.webkitRequestFullscreen()
2. Fullscreen::requestFullscreen():
   a. Checks if fullscreen is allowed by ancestors (allowFullscreen)
   b. Creates fullscreenchange events for |element| and its ancestor
      <iframes> and saves them in its m_eventQueue.
   c. Puts |element| on its fullscreen element stack.
3. FullscreenController::enterFullScreenForElement(element)
   a. Saves |element| as provisional fullscreen element.
   b. Saves some page sizing info (to be restored after fullscreen)
4. RenderFrameImpl::enterFullscreen sends a FrameHostMsg_ToggleFullscreen
   to browser process.
5. RenderFrameHostImpl::OnToggleFullscreen() asks the current tab to
   enter fullscreen.
6. After the tab is resized for fullscreen, browser process generates a
   ViewMsg_Resize to the main frame renderer, with
   is_fullscreen_granted=1 in ResizeParams.
7. RenderWidget::Resize realizes there's a fullscreen change and calls
   into FullscreenController::didEnterFullScreen().
8. FullscreenController::didEnterFullScreen picks up the stored
   provisional fullscreen element and calls
   Fullscreen::didEnterFullScreenForElement on it.
9. Fullscreen::didEnterFullScreenForElement(element):
   a. Adjusts layout on |element| for fullscreen
   b. Sets fullscreen CSS styles on the fullscreen element and all
      ancestor elements.
   c. Fires fullscreenchange events from step 2b.

The main changes in this CL are:

- in step 5, before entering fullscreen for tab, we will send an IPC
  to ancestor OOPIFs so that they do steps 2 and 3 for their
  respective ancestor <iframe> elements (i.e., set up provisional
  fullscreen elements and prepare their share of the fullscreenchange
  events).
- ViewMsg_Resize will trigger fullscreen in subframe widgets in
  addition to main frame ones.  When fullscreen is entered,
  ViewMsg_Resize is sent to all widgets on the page.  WebFrameWidgets
  now also support picking up the fullscreen change and calling into
  the page's FullscreenController.
- blink::Fullscreen and FullscreenController now support entering
  fullscreen for <iframe> elements which contain out-of-process
  fullscreen elements.

There are still various issues to be dealt with in followup CLs:
- further refactoring ancestor walks in Fullscreen::requestFullscreen
  and exitFullscreen to be compatible with hierarchies with a
  RemoteFrame between LocalFrames (such as A-B-A).  This is needed to
  fire all ancestor fullscreenchange events properly in such cases.
- dealing with nested fullscreen elements.
- optimizing fullscreen layout in ancestor processes.
- correcting background color when fullscreening an OOP <iframe>
  element.

More info in design doc: https://goo.gl/Y5wdqi

BUG=550497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

alexmos

Description was changed from ========== Add support for entering/exiting HTML fullscreen from OOPIFs. Currently, entering ...

4 years, 7 months ago (2016-05-10 21:35:19 UTC) #8

Description was changed from

==========
Add support for entering/exiting HTML fullscreen from OOPIFs.

Currently, entering HTML fullscreen utilizes a number of ancestor
frame walks.  This is because entering fullscreen for an element (1)
alters layout for all its ancestor <iframes> (by applying
:-webkit-full-screen styles to them, which applies some UA CSS rules),
(2) fires fullscreenchange in all ancestor documents on <iframe>
elements along the ancestor chain, and (3) makes
document.webkitFullscreenElement return the <iframe> element
containing the fullscreened element in ancestor frames.

Currently, the logic behind this assumes that all ancestor frames are
local and does nothing when they encounter a remote frame.  This CL
takes the first step to fix this.

The existing flow for entering HTML fullscreen goes like this:
1. JS on the page calls element.webkitRequestFullscreen()
2. Fullscreen::requestFullscreen():
   a. Checks if fullscreen is allowed by ancestors (allowFullscreen)
   b. Creates fullscreenchange events for |element| and its ancestor
      <iframes> and saves them in its m_eventQueue.
   c. Puts |element| on its fullscreen element stack.
3. FullscreenController::enterFullScreenForElement(element)
   a. Saves |element| as provisional fullscreen element.
   b. Saves some page sizing info (to be restored after fullscreen)
4. RenderFrameImpl::enterFullscreen sends a FrameHostMsg_ToggleFullscreen
   to browser process.
5. RenderFrameHostImpl::OnToggleFullscreen() asks the current tab to
   enter fullscreen.
6. After the tab is resized for fullscreen, browser process generates a
   ViewMsg_Resize to the main frame renderer, with
   is_fullscreen_granted=1 in ResizeParams.
7. RenderWidget::Resize realizes there's a fullscreen change and calls
   into FullscreenController::didEnterFullScreen().
8. FullscreenController::didEnterFullScreen picks up the stored
   provisional fullscreen element and calls
   Fullscreen::didEnterFullScreenForElement on it.
9. Fullscreen::didEnterFullScreenForElement(element):
   a. Adjusts layout on |element| for fullscreen
   b. Sets fullscreen CSS styles on the fullscreen element and all
      ancestor elements.
   c. Fires fullscreenchange events from step 2b.

The main changes in this CL are:

- in step 5, before entering fullscreen for tab, we will send an IPC
  to ancestor OOPIFs so that they do steps 2 and 3 for their
  respective ancestor <iframe> elements (i.e., set up provisional
  fullscreen elements and prepare their share of the fullscreenchange
  events).
- ViewMsg_Resize will trigger fullscreen in subframe widgets in
  addition to main frame ones.  When fullscreen is entered,
  ViewMsg_Resize is sent to all widgets on the page.  WebFrameWidgets
  now also support picking up the fullscreen change and calling into
  the page's FullscreenController.
- blink::Fullscreen and FullscreenController now support entering
  fullscreen for <iframe> elements which contain out-of-process
  fullscreen elements.

There are still various issues to be dealt with in followup CLs:
- further refactoring ancestor walks in Fullscreen::requestFullscreen
  and exitFullscreen to be compatible with hierarchies with a
  RemoteFrame between LocalFrames (such as A-B-A).  This is needed to
  fire all ancestor fullscreenchange events properly in such cases.
- dealing with nested fullscreen elements.
- optimizing fullscreen layout in ancestor processes.
- correcting background color when fullscreening an OOP <iframe>
  element.

More info in design doc: https://goo.gl/Y5wdqi

BUG=550497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add support for entering/exiting HTML fullscreen from OOPIFs.

Currently, entering HTML fullscreen utilizes a number of ancestor
frame walks.  This is because entering fullscreen for an element (1)
alters layout for all its ancestor <iframes> (by applying
:-webkit-full-screen styles to them, which applies some UA CSS rules),
(2) fires fullscreenchange in all ancestor documents on <iframe>
elements along the ancestor chain, and (3) makes
document.webkitFullscreenElement return the <iframe> element
containing the fullscreened element in ancestor frames.

Currently, the logic behind this assumes that all ancestor frames are
local and does nothing when it encounters a remote frame.  This CL
takes the first step to fix this.

The existing flow for entering HTML fullscreen goes like this:
1. JS on the page calls element.webkitRequestFullscreen()
2. Fullscreen::requestFullscreen():
   a. Checks if fullscreen is allowed by ancestors (allowFullscreen)
   b. Creates fullscreenchange events for |element| and its ancestor
      <iframes> and saves them in its m_eventQueue.
   c. Puts |element| on its fullscreen element stack.
3. FullscreenController::enterFullScreenForElement(element)
   a. Saves |element| as provisional fullscreen element.
   b. Saves some page sizing info (to be restored after fullscreen)
4. RenderFrameImpl::enterFullscreen sends a FrameHostMsg_ToggleFullscreen
   to browser process.
5. RenderFrameHostImpl::OnToggleFullscreen() asks the current tab to
   enter fullscreen.
6. After the tab is resized for fullscreen, browser process generates a
   ViewMsg_Resize to the main frame renderer, with
   is_fullscreen_granted=1 in ResizeParams.
7. RenderWidget::Resize realizes there's a fullscreen change and calls
   into FullscreenController::didEnterFullScreen().
8. FullscreenController::didEnterFullScreen picks up the stored
   provisional fullscreen element and calls
   Fullscreen::didEnterFullScreenForElement on it.
9. Fullscreen::didEnterFullScreenForElement(element):
   a. Adjusts layout on |element| for fullscreen
   b. Sets fullscreen CSS styles on the fullscreen element and all
      ancestor elements.
   c. Fires fullscreenchange events from step 2b.

The main changes in this CL are:

- in step 5, before entering fullscreen for tab, we will send an IPC
  to ancestor OOPIFs so that they do steps 2 and 3 for their
  respective ancestor <iframe> elements (i.e., set up provisional
  fullscreen elements and prepare their share of the fullscreenchange
  events).
- ViewMsg_Resize will trigger fullscreen in subframe widgets in
  addition to main frame ones.  When fullscreen is entered,
  ViewMsg_Resize is sent to all widgets on the page.  WebFrameWidgets
  now also support picking up the fullscreen change and calling into
  the page's FullscreenController.
- blink::Fullscreen and FullscreenController now support entering
  fullscreen for <iframe> elements which contain out-of-process
  fullscreen elements.

There are still various issues to be dealt with in followup CLs:
- further refactoring ancestor walks in Fullscreen::requestFullscreen
  and exitFullscreen to be compatible with hierarchies with a
  RemoteFrame between LocalFrames (such as A-B-A).  This is needed to
  fire all ancestor fullscreenchange events properly in such cases.
- dealing with nested fullscreen elements.
- optimizing fullscreen layout in ancestor processes.
- correcting background color when fullscreening an OOP <iframe>
  element.

More info in design doc: https://goo.gl/Y5wdqi

BUG=550497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

alexmos

alexmos@chromium.org changed reviewers: + creis@chromium.org, dcheng@chromium.org

4 years, 7 months ago (2016-05-10 21:36:23 UTC) #9

alexmos

Charlie/Daniel, can you please take a look? (Charlie for content, Daniel for blink.) I've tried ...

4 years, 7 months ago (2016-05-10 21:36:25 UTC) #10

Charlie/Daniel, can you please take a look?  (Charlie for content, Daniel for
blink.)  I've tried to explain some of the hairy bits below.  Please let me know
if this overall approach makes sense.

https://codereview.chromium.org/1914643005/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1914643005/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1803: // necessary changes
to the other two ancestors.
I considered whether this should be per-widget or per-frame, rather than
per-ancestor-SiteInstance.  One reason for doing it this way is that it
simplifies changes in Blink's FullscreenController, which tracks fullscreen
state per page.  Another reason is that currently, JS can synchronously observe
results of document.requestFullscreen() by checking
document.webkitCurrentFullScreenElement.  So, with A-B-A, the bottom frame could
do 
  element.requestFullscreen();
  use parent.parent.webkitCurrentFullScreenElement
And at that point, parent.parent.webkitCurrentFullScreenElement returns the
<iframe> element for B (even though fullscreenchange hasn't yet fired, and
fullscreen hasn't been entered).

So, to keep this sane, I've decided to let Blink code walk through all local
ancestors in each SiteInstance.

https://codereview.chromium.org/1914643005/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1832: // widgets, this will
also trigger them to resize via frameRectsChanged.
My doc (https://goo.gl/Y5wdqi) has some more info about why I ended up keeping
fullscreen triggering in ViewMsg_Resize, but basically this avoids firing the
fullscreenchange event too early, before the fullscreen resizing is complete,
and allows the widget to process all the fullscreen resizing and restyling as
part of a single IPC.

https://codereview.chromium.org/1914643005/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/1914643005/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1868: if
(!RenderViewHostImpl::From(render_widget_host))
Removing this allows subframe widgets to pick up the is_fullscreen_granted bit
when sending ViewMsg_Resize. (This is used in RWHI::GetResizeParams.)  

I've checked all other uses of this, and they all pass in an RVH's widget, so
this should be safe.  In fact, the |render_widget_host| param is now
unnecessary, but I'd like to remove it in a follow-up CL, since it affects a
bunch of unrelated files.

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

https://codereview.chromium.org/1914643005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:64: return true;
This will go away once I rebase on the allowFullscreen replication CL
(https://codereview.chromium.org/1938753002/)

https://codereview.chromium.org/1914643005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:445: // not be necessary to
repeat it here.
Namely, the wrapLayoutObject call and stuff in LayoutFullScreen::updateStyle()
is only necessary in the actual fullscreen element's frame and process (this is
the stuff that creates an anonymous flexbox with a large z-index and solid black
background).   It isn't really necessary for the parent <iframes> since those
adjust for fullscreen via UA CSS instead (see Source/core/css/fullscreen.css),
which is triggered by the setContainsFullScreenElement flag below.  I've gotten
this to work, but I'm deferring this to a followup CL to avoid overcomplicating
this one.

https://codereview.chromium.org/1914643005/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/1914643005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:198:
view()->didUpdateFullScreenSize();
This is needed for things like opening/resizing a devtools page on the side
while in fullscreen mode.

Charlie Reis

This all seems reasonable to me, and thanks for the thorough tests. I'm happy to ...

4 years, 7 months ago (2016-05-12 22:59:24 UTC) #11

This all seems reasonable to me, and thanks for the thorough tests.  I'm happy
to approve content/, though I wonder if there's anyone who knows fullscreen in a
bit more depth that might be able to look over the approach.  Are you able to
find any past owners in the source history?

https://codereview.chromium.org/1914643005/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1914643005/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1798: // remote child frame
that contains the fullscreened element.  The renderer
nit: renderer process

https://codereview.chromium.org/1914643005/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1803: // necessary changes
to the other two ancestors.
On 2016/05/10 21:36:24, alexmos wrote:
> I considered whether this should be per-widget or per-frame, rather than
> per-ancestor-SiteInstance.  One reason for doing it this way is that it
> simplifies changes in Blink's FullscreenController, which tracks fullscreen
> state per page.  Another reason is that currently, JS can synchronously
observe
> results of document.requestFullscreen() by checking
> document.webkitCurrentFullScreenElement.  So, with A-B-A, the bottom frame
could
> do 
>   element.requestFullscreen();
>   use parent.parent.webkitCurrentFullScreenElement
> And at that point, parent.parent.webkitCurrentFullScreenElement returns the
> <iframe> element for B (even though fullscreenchange hasn't yet fired, and
> fullscreen hasn't been entered).
> 
> So, to keep this sane, I've decided to let Blink code walk through all local
> ancestors in each SiteInstance.  

Acknowledged.

https://codereview.chromium.org/1914643005/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1818:
parent_rfh->GetRoutingID(), child_routing_id));
Nick was wondering if this should just go to child_routing_id instead, and let
the proxy handle it?  (I left a comment in RenderFrameImpl as well.)

https://codereview.chromium.org/1914643005/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1832: // widgets, this will
also trigger them to resize via frameRectsChanged.
On 2016/05/10 21:36:24, alexmos wrote:
> My doc (https://goo.gl/Y5wdqi) has some more info about why I ended up keeping
> fullscreen triggering in ViewMsg_Resize, but basically this avoids firing the
> fullscreenchange event too early, before the fullscreen resizing is complete,
> and allows the widget to process all the fullscreen resizing and restyling as
> part of a single IPC.

Acknowledged.

https://codereview.chromium.org/1914643005/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/1914643005/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1868: if
(!RenderViewHostImpl::From(render_widget_host))
On 2016/05/10 21:36:24, alexmos wrote:
> Removing this allows subframe widgets to pick up the is_fullscreen_granted bit
> when sending ViewMsg_Resize. (This is used in RWHI::GetResizeParams.)  
> 
> I've checked all other uses of this, and they all pass in an RVH's widget, so
> this should be safe.  In fact, the |render_widget_host| param is now
> unnecessary, but I'd like to remove it in a follow-up CL, since it affects a
> bunch of unrelated files.

Ok.  Let's put a TODO to remove it.

https://codereview.chromium.org/1914643005/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1914643005/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2048:
render_view_->webview()->willEnterFullScreen(fullscreen_frame->web_frame());
This method doesn't seem to act on |this| at all.  Would it be simpler to just
send the IPC to the child RenderFrameProxy and have it call willEnterFullScreen
on its RenderView?

https://codereview.chromium.org/1914643005/diff/160001/content/test/data/fram...
File content/test/data/frame_tree/page_with_one_frame.html (right):

https://codereview.chromium.org/1914643005/diff/160001/content/test/data/fram...
content/test/data/frame_tree/page_with_one_frame.html:16: <iframe id="child0"
src="/cross-site/baz.com/title1.html" allowfullscreen=true></iframe>
This is kind of unrelated to most of the tests that use this file.  Is it
possible to do this programmatically from the test where you need it?

alexmos

alexmos@chromium.org changed reviewers: + falken@chromium.org

4 years, 7 months ago (2016-05-13 07:21:02 UTC) #12

alexmos

On 2016/05/12 22:59:24, Charlie Reis (slow to reply) wrote: > This all seems reasonable to ...

4 years, 7 months ago (2016-05-13 07:21:03 UTC) #13

On 2016/05/12 22:59:24, Charlie Reis (slow to reply) wrote:
> This all seems reasonable to me, and thanks for the thorough tests.  I'm happy
> to approve content/, though I wonder if there's anyone who knows fullscreen in
a
> bit more depth that might be able to look over the approach.  Are you able to
> find any past owners in the source history?

Hmm, it seems that philipj@opera.com has been doing the most recent fullscreen
work in Blink, but it look like he left recently. :(

falken@: I see that you've reviewed several fullscreen CLs in the past.  Would
you be comfortable reviewing the approach here?

(note: the trybot failures in PS10 are due to ui/gfx/display.h moving to
ui/display/display.h, I'll rebase and update this tomorrow.  All tests were
green in PS9.)

https://codereview.chromium.org/1914643005/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1914643005/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1798: // remote child frame
that contains the fullscreened element.  The renderer
On 2016/05/12 22:59:23, Charlie Reis (slow to reply) wrote:
> nit: renderer process

Done.

https://codereview.chromium.org/1914643005/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1818:
parent_rfh->GetRoutingID(), child_routing_id));
On 2016/05/12 22:59:23, Charlie Reis (slow to reply) wrote:
> Nick was wondering if this should just go to child_routing_id instead, and let
> the proxy handle it?  (I left a comment in RenderFrameImpl as well.)

This is a great idea!  Done.

https://codereview.chromium.org/1914643005/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1914643005/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2048:
render_view_->webview()->willEnterFullScreen(fullscreen_frame->web_frame());
On 2016/05/12 22:59:23, Charlie Reis (slow to reply) wrote:
> This method doesn't seem to act on |this| at all.  Would it be simpler to just
> send the IPC to the child RenderFrameProxy and have it call
willEnterFullScreen
> on its RenderView?

Done.

https://codereview.chromium.org/1914643005/diff/160001/content/test/data/fram...
File content/test/data/frame_tree/page_with_one_frame.html (right):

https://codereview.chromium.org/1914643005/diff/160001/content/test/data/fram...
content/test/data/frame_tree/page_with_one_frame.html:16: <iframe id="child0"
src="/cross-site/baz.com/title1.html" allowfullscreen=true></iframe>
On 2016/05/12 22:59:23, Charlie Reis (slow to reply) wrote:
> This is kind of unrelated to most of the tests that use this file.  Is it
> possible to do this programmatically from the test where you need it?

Ah, this is actually first introduced in my test in the allowFullscreen
replication CL, so that we have coverage for the static <iframe allowfullscreen>
case in addition to the dynamic one:
https://codereview.chromium.org/1938753002/diff/100001/content/test/data/fram...

so this was going to go away once I rebase on that CL, but your point still
stands.  I can add a new html test file with just the static flag, but I'll
probably do that in the replication CL, and then I can reuse the file here. 
Would that be ok?

falken

On 2016/05/13 07:21:03, alexmos wrote: > On 2016/05/12 22:59:24, Charlie Reis (slow to reply) wrote: ...

4 years, 7 months ago (2016-05-13 07:43:49 UTC) #14

Charlie Reis

Looks like you're the new owner, Alex! :) content/ LGTM. https://codereview.chromium.org/1914643005/diff/160001/content/test/data/frame_tree/page_with_one_frame.html File content/test/data/frame_tree/page_with_one_frame.html (right): https://codereview.chromium.org/1914643005/diff/160001/content/test/data/frame_tree/page_with_one_frame.html#newcode16 ...

4 years, 7 months ago (2016-05-13 20:12:14 UTC) #15

dcheng

Overall approach looks reasonable. Just some high-level questions. https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/Source/core/dom/Element.cpp File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/Source/core/dom/Element.cpp#newcode3059 third_party/WebKit/Source/core/dom/Element.cpp:3059: static ...

4 years, 7 months ago (2016-05-18 01:04:35 UTC) #16

alexmos

Thanks for reviewing! https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/Source/core/dom/Element.cpp File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/Source/core/dom/Element.cpp#newcode3059 third_party/WebKit/Source/core/dom/Element.cpp:3059: static Element* parentCrossingFrameBoundaries(Element* element) On 2016/05/18 ...

4 years, 7 months ago (2016-05-18 15:08:33 UTC) #17

Thanks for reviewing!

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:3059: static Element*
parentCrossingFrameBoundaries(Element* element)
On 2016/05/18 01:04:34, dcheng wrote:
> This function should probably be renamed, it's not just a parent anymore
> necessarily.

I agree the name isn't great, but it was hard to come up with something better.
:)  How does nextLocalAncestorElement sound?

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

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:463: void
Fullscreen::didExitFullScreenForElement(bool isAncestorOfFullscreenElement)
On 2016/05/18 01:04:34, dcheng wrote:
> See below comment. Wondering if we need this parameter at all: can it be
> inferred by the fact that m_fullScreenElement is an HTMLFrameOwnerElement with
a
> RemoteFrame for its content frame?

I actually started out with that approach, but turns out it can't distinguish
between fullscreening the actual HTMLIFrameElement (with a RemoteFrame) in the
parent frame and fullscreening some element in the child frame.  In  the parent
process, both will end up setting the same element as m_fullScreenElement, but
they need to behave slightly differently: the latter needs the
:-webkit-full-screen-ancestor CSS but the former doesn't (hence the check
below).

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:1957:
m_fullscreenController->setFullscreenIsForCrossProcessAncestor();
On 2016/05/18 01:04:35, dcheng wrote:
> So I'm curious if we need to actually do this: can FullscreenController infer
> this by the fact that the FrameOwner's contentFrame is a RemoteFrame?

See my comment above - the reason is that you can also fullscreen iframe
elements themselves (which may contain RemoteFrames).

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebView.h (right):

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebView.h:343: virtual void
willEnterFullScreen(WebRemoteFrame*) = 0;
On 2016/05/18 01:04:35, dcheng wrote:
> Out of scope for this patch, but is there any hope of unifying this with
> WebWidget::didEnterFullScreen in the future?

It could probably be done, though it might be a bit tricky.  Basically, I think
you're suggesting to just call didEnterFullScreen, optionally passing in this
WebRemoteFrame, and inside its implementation, if a remote frame is passed in,
do the additional work to set up the fullscreen events, etc (which
willEnterFullScreen currently does) before proceeding with actual fullscreen
activation - right?

The tricky part is that didEnterFullScreen is triggered by the resize message,
which would now need to carry a routing ID for the remote fullscreen frame, if
any, which means that browser process will have to maintain some state about
which frame has a fullscreened element, so it can tag its routing ID along on
ViewMsg_Resize, and also this routing ID must be computable from
RenderWidgetHostImpl, which doesn't know about proxies, etc.  I actually started
going that route at one point, but abandoned that approach as it grew too
complex.

dcheng

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/Source/core/dom/Element.cpp File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/Source/core/dom/Element.cpp#newcode3059 third_party/WebKit/Source/core/dom/Element.cpp:3059: static Element* parentCrossingFrameBoundaries(Element* element) On 2016/05/18 at 15:08:33, alexmos ...

4 years, 7 months ago (2016-05-19 00:21:34 UTC) #18

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:3059: static Element*
parentCrossingFrameBoundaries(Element* element)
On 2016/05/18 at 15:08:33, alexmos wrote:
> On 2016/05/18 01:04:34, dcheng wrote:
> > This function should probably be renamed, it's not just a parent anymore
> > necessarily.
> 
> I agree the name isn't great, but it was hard to come up with something
better. :)  How does nextLocalAncestorElement sound?

Ehh.... maybe? I'm not too thrilled with either name, but I think this is
slightly better?

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebView.h (right):

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebView.h:343: virtual void
willEnterFullScreen(WebRemoteFrame*) = 0;
On 2016/05/18 at 15:08:33, alexmos wrote:
> On 2016/05/18 01:04:35, dcheng wrote:
> > Out of scope for this patch, but is there any hope of unifying this with
> > WebWidget::didEnterFullScreen in the future?
> 
> It could probably be done, though it might be a bit tricky.  Basically, I
think you're suggesting to just call didEnterFullScreen, optionally passing in
this WebRemoteFrame, and inside its implementation, if a remote frame is passed
in, do the additional work to set up the fullscreen events, etc (which
willEnterFullScreen currently does) before proceeding with actual fullscreen
activation - right?
> 
> The tricky part is that didEnterFullScreen is triggered by the resize message,
which would now need to carry a routing ID for the remote fullscreen frame, if
any, which means that browser process will have to maintain some state about
which frame has a fullscreened element, so it can tag its routing ID along on
ViewMsg_Resize, and also this routing ID must be computable from
RenderWidgetHostImpl, which doesn't know about proxies, etc.  I actually started
going that route at one point, but abandoned that approach as it grew too
complex.

I looked around the code at this: it looks like we explicitly call WasResized()
when entering/leaving fullscreen. Instead of using WasResized(), maybe it'd make
sense to have an explicit RWH::EnterFullscreen() / ExitFullscreen() with a
corresponding IPC? And maybe we won't need the fullscreen member on
ResizeMsg_Params then... WDYT?

alexmos

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/Source/core/dom/Element.cpp File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/Source/core/dom/Element.cpp#newcode3059 third_party/WebKit/Source/core/dom/Element.cpp:3059: static Element* parentCrossingFrameBoundaries(Element* element) On 2016/05/19 00:21:34, dcheng wrote: ...

4 years, 7 months ago (2016-05-19 01:08:25 UTC) #19

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:3059: static Element*
parentCrossingFrameBoundaries(Element* element)
On 2016/05/19 00:21:34, dcheng wrote:
> On 2016/05/18 at 15:08:33, alexmos wrote:
> > On 2016/05/18 01:04:34, dcheng wrote:
> > > This function should probably be renamed, it's not just a parent anymore
> > > necessarily.
> > 
> > I agree the name isn't great, but it was hard to come up with something
> better. :)  How does nextLocalAncestorElement sound?
> 
> Ehh.... maybe? I'm not too thrilled with either name, but I think this is
> slightly better?

OK, done.

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebView.h (right):

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebView.h:343: virtual void
willEnterFullScreen(WebRemoteFrame*) = 0;
On 2016/05/19 00:21:34, dcheng wrote:
> On 2016/05/18 at 15:08:33, alexmos wrote:
> > On 2016/05/18 01:04:35, dcheng wrote:
> > > Out of scope for this patch, but is there any hope of unifying this with
> > > WebWidget::didEnterFullScreen in the future?
> > 
> > It could probably be done, though it might be a bit tricky.  Basically, I
> think you're suggesting to just call didEnterFullScreen, optionally passing in
> this WebRemoteFrame, and inside its implementation, if a remote frame is
passed
> in, do the additional work to set up the fullscreen events, etc (which
> willEnterFullScreen currently does) before proceeding with actual fullscreen
> activation - right?
> > 
> > The tricky part is that didEnterFullScreen is triggered by the resize
message,
> which would now need to carry a routing ID for the remote fullscreen frame, if
> any, which means that browser process will have to maintain some state about
> which frame has a fullscreened element, so it can tag its routing ID along on
> ViewMsg_Resize, and also this routing ID must be computable from
> RenderWidgetHostImpl, which doesn't know about proxies, etc.  I actually
started
> going that route at one point, but abandoned that approach as it grew too
> complex.
> 
> I looked around the code at this: it looks like we explicitly call
WasResized()
> when entering/leaving fullscreen. Instead of using WasResized(), maybe it'd
make
> sense to have an explicit RWH::EnterFullscreen() / ExitFullscreen() with a
> corresponding IPC? And maybe we won't need the fullscreen member on
> ResizeMsg_Params then... WDYT?

I wish that were true. But actually, in the common case the resize happens from
the ui layer, deep inside the delegate_->EnterFullscreenMode() call, once the
native widgets are actually resized.  That WasResized() call is just insurance
to make sure the resize message is sent, as it's apparently possible to enter
HTML fullscreen without resizing (I'm guessing tab fullscreen followed by HTML
fullscreen would do this).  I've updated the comment there to reflect this.

Here's the stack for the common case:

#1 0x7f81819e01b1 content::RenderWidgetHostImpl::GetResizeParams()
#2 0x7f81819e0492 content::RenderWidgetHostImpl::WasResized()
#3 0x7f8181a00a5c content::RenderWidgetHostViewAura::InternalSetBounds()
#4 0x7f8181a009e9 content::RenderWidgetHostViewAura::SetSize()
#5 0x7f8181a0560b content::RenderWidgetHostViewAura::OnBoundsChanged()
#6 0x7f817cdccd81 aura::Window::OnWindowBoundsChanged()
#7 0x7f817cddf5fc
_ZN4base8internal15RunnableAdapterIMN4aura6WindowEFvRKN3gfx4RectEEE3RunIPS3_JS7_EEEvOT_DpOT0_
#8 0x7f817cddf51e
_ZN4base8internal12InvokeHelperILb0EvNS0_15RunnableAdapterIMN4aura6WindowEFvRKN3gfx4RectEEEEE8MakeItSoIJPS4_S8_EEEvSB_DpOT_
#9 0x7f817cddf4d8
_ZN4base8internal7InvokerINS_13IndexSequenceIJLm0ELm1EEEENS0_9BindStateINS0_15RunnableAdapterIMN4aura6WindowEFvRKN3gfx4RectEEEEFvPS7_SB_EJNS0_17UnretainedWrapperIS7_EESB_EEENS0_12InvokeHelperILb0EvSE_EEFvvEE3RunEPNS0_13BindStateBaseE
#10 0x7f817cf921ce base::Callback<>::Run()
#11 0x7f817cfb415d ui::Layer::SetBoundsFromAnimation()
#12 0x7f817cfcea8c ui::LayerAnimator::SetBounds()
#13 0x7f817cfb037c ui::Layer::SetBounds()
#14 0x7f817cdc96c0 aura::Window::SetBoundsInternal()
#15 0x7f817cdc966c aura::Window::SetBounds()
#16 0x7f8181a00a45 content::RenderWidgetHostViewAura::InternalSetBounds()
#17 0x7f8181a009e9 content::RenderWidgetHostViewAura::SetSize()
#18 0x7f8181cd8e03 content::WebContentsViewAura::SizeChangedCommon()
#19 0x7f8181cdb051 content::WebContentsViewAura::OnBoundsChanged()
#20 0x7f817cdccd81 aura::Window::OnWindowBoundsChanged()
#21 0x7f817cddf5fc
_ZN4base8internal15RunnableAdapterIMN4aura6WindowEFvRKN3gfx4RectEEE3RunIPS3_JS7_EEEvOT_DpOT0_
#22 0x7f817cddf51e
_ZN4base8internal12InvokeHelperILb0EvNS0_15RunnableAdapterIMN4aura6WindowEFvRKN3gfx4RectEEEEE8MakeItSoIJPS4_S8_EEEvSB_DpOT_
#23 0x7f817cddf4d8
_ZN4base8internal7InvokerINS_13IndexSequenceIJLm0ELm1EEEENS0_9BindStateINS0_15RunnableAdapterIMN4aura6WindowEFvRKN3gfx4RectEEEEFvPS7_SB_EJNS0_17UnretainedWrapperIS7_EESB_EEENS0_12InvokeHelperILb0EvSE_EEFvvEE3RunEPNS0_13BindStateBaseE
#24 0x7f817cf921ce base::Callback<>::Run()
#25 0x7f817cfb415d ui::Layer::SetBoundsFromAnimation()
#26 0x7f817cfcea8c ui::LayerAnimator::SetBounds()
#27 0x7f817cfb037c ui::Layer::SetBounds()
#28 0x7f817cdc96c0 aura::Window::SetBoundsInternal()
#29 0x7f817cdc966c aura::Window::SetBounds()
#30 0x7f817d2b20f3 views::NativeViewHostAura::ShowWidget()
#31 0x7f817d1d886c views::NativeViewHost::Layout()
#32 0x7f817d25f5d2 views::View::BoundsChanged()
#33 0x7f817d25f229 views::View::SetBoundsRect()
#34 0x7f817ce30f2a views::WebView::OnBoundsChanged()
#35 0x7f817d25f56c views::View::BoundsChanged()
#36 0x7f817d25f229 views::View::SetBoundsRect()
#37 0x7f818b1e4a7c ContentsLayoutManager::Layout()
#38 0x7f817d260f50 views::View::Layout()
#39 0x7f817d25f5d2 views::View::BoundsChanged()
#40 0x7f817d25f229 views::View::SetBoundsRect()
#41 0x7f818b1de525 BrowserViewLayout::LayoutContentsContainerView()
#42 0x7f818b1ddaad BrowserViewLayout::Layout()
#43 0x7f817d260f50 views::View::Layout()
#44 0x7f818b1d7917 BrowserView::Layout()
#45 0x7f818b1d30a5 BrowserView::UpdateUIForContents()
#46 0x7f818b1d419b BrowserView::ToolbarSizeChanged()
#47 0x7f818b1d37e7 BrowserView::ProcessFullscreen()
#48 0x7f818b1d350b BrowserView::EnterFullscreen()
#49 0x7f818af15a75 FullscreenController::EnterFullscreenModeInternal()
#50 0x7f818af13f77 FullscreenController::ToggleFullscreenModeInternal()
#51 0x7f818af14650 FullscreenController::EnterFullscreenModeForTab()
#52 0x7f818aed13df Browser::EnterFullscreenModeForTab()
#53 0x7f8181ca6436 content::WebContentsImpl::EnterFullscreenMode()
#54 0x7f81814bfdd2 content::RenderFrameHostImpl::OnToggleFullscreen()

I was planning to do what you suggested originally (decoupling enter fullscreen
from resize), but realized that it probably makes sense to do the widget resize
+ fullscreen restyling in one message, in which case layout can be fixed up all
at once.  It's also desirable to fire fullscreen events as a result of the
fullscreen resize so that event handlers can observe the new width/height
(though that has its own issues).

dcheng

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/public/web/WebView.h File third_party/WebKit/public/web/WebView.h (right): https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/public/web/WebView.h#newcode343 third_party/WebKit/public/web/WebView.h:343: virtual void willEnterFullScreen(WebRemoteFrame*) = 0; On 2016/05/19 at 01:08:25, ...

4 years, 7 months ago (2016-05-19 05:29:14 UTC) #20

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebView.h (right):

https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebView.h:343: virtual void
willEnterFullScreen(WebRemoteFrame*) = 0;
On 2016/05/19 at 01:08:25, alexmos wrote:
> On 2016/05/19 00:21:34, dcheng wrote:
> > On 2016/05/18 at 15:08:33, alexmos wrote:
> > > On 2016/05/18 01:04:35, dcheng wrote:
> > > > Out of scope for this patch, but is there any hope of unifying this with
> > > > WebWidget::didEnterFullScreen in the future?
> > > 
> > > It could probably be done, though it might be a bit tricky.  Basically, I
> > think you're suggesting to just call didEnterFullScreen, optionally passing
in
> > this WebRemoteFrame, and inside its implementation, if a remote frame is
passed
> > in, do the additional work to set up the fullscreen events, etc (which
> > willEnterFullScreen currently does) before proceeding with actual fullscreen
> > activation - right?
> > > 
> > > The tricky part is that didEnterFullScreen is triggered by the resize
message,
> > which would now need to carry a routing ID for the remote fullscreen frame,
if
> > any, which means that browser process will have to maintain some state about
> > which frame has a fullscreened element, so it can tag its routing ID along
on
> > ViewMsg_Resize, and also this routing ID must be computable from
> > RenderWidgetHostImpl, which doesn't know about proxies, etc.  I actually
started
> > going that route at one point, but abandoned that approach as it grew too
> > complex.
> > 
> > I looked around the code at this: it looks like we explicitly call
WasResized()
> > when entering/leaving fullscreen. Instead of using WasResized(), maybe it'd
make
> > sense to have an explicit RWH::EnterFullscreen() / ExitFullscreen() with a
> > corresponding IPC? And maybe we won't need the fullscreen member on
> > ResizeMsg_Params then... WDYT?
> 
> I wish that were true. But actually, in the common case the resize happens
from the ui layer, deep inside the delegate_->EnterFullscreenMode() call, once
the native widgets are actually resized.  That WasResized() call is just
insurance to make sure the resize message is sent, as it's apparently possible
to enter HTML fullscreen without resizing (I'm guessing tab fullscreen followed
by HTML fullscreen would do this).  I've updated the comment there to reflect
this.

Hmm. Tricky. It's particularly weird because we call RWH::WasResized() in
WebContentsImpl::ExitFullscreenMode()... and we also call it from
RenderFrameHostImpl::OnToggleFullscreen, right before we call into that delegate
code. Maybe something to cleanup for the future I guess.

> 
> Here's the stack for the common case:
> 
> #1 0x7f81819e01b1 content::RenderWidgetHostImpl::GetResizeParams()
> #2 0x7f81819e0492 content::RenderWidgetHostImpl::WasResized()
> #3 0x7f8181a00a5c content::RenderWidgetHostViewAura::InternalSetBounds()
> #4 0x7f8181a009e9 content::RenderWidgetHostViewAura::SetSize()
> #5 0x7f8181a0560b content::RenderWidgetHostViewAura::OnBoundsChanged()
> #6 0x7f817cdccd81 aura::Window::OnWindowBoundsChanged()
> #7 0x7f817cddf5fc
_ZN4base8internal15RunnableAdapterIMN4aura6WindowEFvRKN3gfx4RectEEE3RunIPS3_JS7_EEEvOT_DpOT0_
> #8 0x7f817cddf51e
_ZN4base8internal12InvokeHelperILb0EvNS0_15RunnableAdapterIMN4aura6WindowEFvRKN3gfx4RectEEEEE8MakeItSoIJPS4_S8_EEEvSB_DpOT_
> #9 0x7f817cddf4d8
_ZN4base8internal7InvokerINS_13IndexSequenceIJLm0ELm1EEEENS0_9BindStateINS0_15RunnableAdapterIMN4aura6WindowEFvRKN3gfx4RectEEEEFvPS7_SB_EJNS0_17UnretainedWrapperIS7_EESB_EEENS0_12InvokeHelperILb0EvSE_EEFvvEE3RunEPNS0_13BindStateBaseE
> #10 0x7f817cf921ce base::Callback<>::Run()
> #11 0x7f817cfb415d ui::Layer::SetBoundsFromAnimation()
> #12 0x7f817cfcea8c ui::LayerAnimator::SetBounds()
> #13 0x7f817cfb037c ui::Layer::SetBounds()
> #14 0x7f817cdc96c0 aura::Window::SetBoundsInternal()
> #15 0x7f817cdc966c aura::Window::SetBounds()
> #16 0x7f8181a00a45 content::RenderWidgetHostViewAura::InternalSetBounds()
> #17 0x7f8181a009e9 content::RenderWidgetHostViewAura::SetSize()
> #18 0x7f8181cd8e03 content::WebContentsViewAura::SizeChangedCommon()
> #19 0x7f8181cdb051 content::WebContentsViewAura::OnBoundsChanged()
> #20 0x7f817cdccd81 aura::Window::OnWindowBoundsChanged()
> #21 0x7f817cddf5fc
_ZN4base8internal15RunnableAdapterIMN4aura6WindowEFvRKN3gfx4RectEEE3RunIPS3_JS7_EEEvOT_DpOT0_
> #22 0x7f817cddf51e
_ZN4base8internal12InvokeHelperILb0EvNS0_15RunnableAdapterIMN4aura6WindowEFvRKN3gfx4RectEEEEE8MakeItSoIJPS4_S8_EEEvSB_DpOT_
> #23 0x7f817cddf4d8
_ZN4base8internal7InvokerINS_13IndexSequenceIJLm0ELm1EEEENS0_9BindStateINS0_15RunnableAdapterIMN4aura6WindowEFvRKN3gfx4RectEEEEFvPS7_SB_EJNS0_17UnretainedWrapperIS7_EESB_EEENS0_12InvokeHelperILb0EvSE_EEFvvEE3RunEPNS0_13BindStateBaseE
> #24 0x7f817cf921ce base::Callback<>::Run()
> #25 0x7f817cfb415d ui::Layer::SetBoundsFromAnimation()
> #26 0x7f817cfcea8c ui::LayerAnimator::SetBounds()
> #27 0x7f817cfb037c ui::Layer::SetBounds()
> #28 0x7f817cdc96c0 aura::Window::SetBoundsInternal()
> #29 0x7f817cdc966c aura::Window::SetBounds()
> #30 0x7f817d2b20f3 views::NativeViewHostAura::ShowWidget()
> #31 0x7f817d1d886c views::NativeViewHost::Layout()
> #32 0x7f817d25f5d2 views::View::BoundsChanged()
> #33 0x7f817d25f229 views::View::SetBoundsRect()
> #34 0x7f817ce30f2a views::WebView::OnBoundsChanged()
> #35 0x7f817d25f56c views::View::BoundsChanged()
> #36 0x7f817d25f229 views::View::SetBoundsRect()
> #37 0x7f818b1e4a7c ContentsLayoutManager::Layout()
> #38 0x7f817d260f50 views::View::Layout()
> #39 0x7f817d25f5d2 views::View::BoundsChanged()
> #40 0x7f817d25f229 views::View::SetBoundsRect()
> #41 0x7f818b1de525 BrowserViewLayout::LayoutContentsContainerView()
> #42 0x7f818b1ddaad BrowserViewLayout::Layout()
> #43 0x7f817d260f50 views::View::Layout()
> #44 0x7f818b1d7917 BrowserView::Layout()
> #45 0x7f818b1d30a5 BrowserView::UpdateUIForContents()
> #46 0x7f818b1d419b BrowserView::ToolbarSizeChanged()
> #47 0x7f818b1d37e7 BrowserView::ProcessFullscreen()
> #48 0x7f818b1d350b BrowserView::EnterFullscreen()
> #49 0x7f818af15a75 FullscreenController::EnterFullscreenModeInternal()
> #50 0x7f818af13f77 FullscreenController::ToggleFullscreenModeInternal()
> #51 0x7f818af14650 FullscreenController::EnterFullscreenModeForTab()
> #52 0x7f818aed13df Browser::EnterFullscreenModeForTab()
> #53 0x7f8181ca6436 content::WebContentsImpl::EnterFullscreenMode()
> #54 0x7f81814bfdd2 content::RenderFrameHostImpl::OnToggleFullscreen()
> 
> I was planning to do what you suggested originally (decoupling enter
fullscreen from resize), but realized that it probably makes sense to do the
widget resize + fullscreen restyling in one message, in which case layout can be
fixed up all at once.  It's also desirable to fire fullscreen events as a result
of the fullscreen resize so that event handlers can observe the new width/height
(though that has its own issues).

I agree it makes sense to keep them together.
Another proposal: is it possible to have WasResized() send a distinct IPC for
the fullscreen change case? On the browser side, it could send a
ResizeMsg_Params + optional routing ID in this new IPC. On the renderer side,
there could be a new OnFullscreen handler that delegates to
RenderWidget::Resize(...) to share the core bits, and then calls
DidToggleFullscreen()? Or are there other reasons that we can't separate them
out like this?

alexmos

On 2016/05/19 05:29:14, dcheng wrote: > https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/public/web/WebView.h > File third_party/WebKit/public/web/WebView.h (right): > > https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/public/web/WebView.h#newcode343 > ...

4 years, 7 months ago (2016-05-19 16:48:22 UTC) #21

On 2016/05/19 05:29:14, dcheng wrote:
>
https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/pub...
> File third_party/WebKit/public/web/WebView.h (right):
> 
>
https://codereview.chromium.org/1914643005/diff/200001/third_party/WebKit/pub...
> third_party/WebKit/public/web/WebView.h:343: virtual void
> willEnterFullScreen(WebRemoteFrame*) = 0;
> On 2016/05/19 at 01:08:25, alexmos wrote:
> > On 2016/05/19 00:21:34, dcheng wrote:
> > > On 2016/05/18 at 15:08:33, alexmos wrote:
> > > > On 2016/05/18 01:04:35, dcheng wrote:
> > > > > Out of scope for this patch, but is there any hope of unifying this
with
> > > > > WebWidget::didEnterFullScreen in the future?
> > > > 
> > > > It could probably be done, though it might be a bit tricky.  Basically,
I
> > > think you're suggesting to just call didEnterFullScreen, optionally
passing
> in
> > > this WebRemoteFrame, and inside its implementation, if a remote frame is
> passed
> > > in, do the additional work to set up the fullscreen events, etc (which
> > > willEnterFullScreen currently does) before proceeding with actual
fullscreen
> > > activation - right?
> > > > 
> > > > The tricky part is that didEnterFullScreen is triggered by the resize
> message,
> > > which would now need to carry a routing ID for the remote fullscreen
frame,
> if
> > > any, which means that browser process will have to maintain some state
about
> > > which frame has a fullscreened element, so it can tag its routing ID along
> on
> > > ViewMsg_Resize, and also this routing ID must be computable from
> > > RenderWidgetHostImpl, which doesn't know about proxies, etc.  I actually
> started
> > > going that route at one point, but abandoned that approach as it grew too
> > > complex.
> > > 
> > > I looked around the code at this: it looks like we explicitly call
> WasResized()
> > > when entering/leaving fullscreen. Instead of using WasResized(), maybe
it'd
> make
> > > sense to have an explicit RWH::EnterFullscreen() / ExitFullscreen() with a
> > > corresponding IPC? And maybe we won't need the fullscreen member on
> > > ResizeMsg_Params then... WDYT?
> > 
> > I wish that were true. But actually, in the common case the resize happens
> from the ui layer, deep inside the delegate_->EnterFullscreenMode() call, once
> the native widgets are actually resized.  That WasResized() call is just
> insurance to make sure the resize message is sent, as it's apparently possible
> to enter HTML fullscreen without resizing (I'm guessing tab fullscreen
followed
> by HTML fullscreen would do this).  I've updated the comment there to reflect
> this.
> 
> Hmm. Tricky. It's particularly weird because we call RWH::WasResized() in
> WebContentsImpl::ExitFullscreenMode()... and we also call it from
> RenderFrameHostImpl::OnToggleFullscreen, right before we call into that
delegate
> code. Maybe something to cleanup for the future I guess.

Yes, agreed that this could be cleaned up in the future. 
(RFHI::OnToggleFullscreen passes a flag to prevent that WasResized from being
triggered in WebContentsImpl::ExitFullscreenMode, since it does its own
WasResized, and this all looks messy.)

> > I was planning to do what you suggested originally (decoupling enter
> fullscreen from resize), but realized that it probably makes sense to do the
> widget resize + fullscreen restyling in one message, in which case layout can
be
> fixed up all at once.  It's also desirable to fire fullscreen events as a
result
> of the fullscreen resize so that event handlers can observe the new
width/height
> (though that has its own issues).
> 
> I agree it makes sense to keep them together.
> Another proposal: is it possible to have WasResized() send a distinct IPC for
> the fullscreen change case? On the browser side, it could send a
> ResizeMsg_Params + optional routing ID in this new IPC. On the renderer side,
> there could be a new OnFullscreen handler that delegates to
> RenderWidget::Resize(...) to share the core bits, and then calls
> DidToggleFullscreen()? Or are there other reasons that we can't separate them
> out like this?

That is possible, yes.  That will be cleaner on the renderer side.  However,
getting that routing ID will be kind of ugly -- from RWH::WasResized, you don't
know which frame caused the fullscreen enter, and the RWH being resized doesn't
have a reference to which frames it's serving.  So you have to do something like
keep track of the current fullscreened frame in WebContents, call out to the
delegate from RWH::WasResized, then try to find which frames/SiteInstance the
RWH corresponds to and find the right routing ID to send.  I've tried this --
see PS1, WebContentsImpl::GetFullscreenFrameRoutingID -- maybe there's an easier
way, but it seemed really error-prone (e.g., what if there's a simultaneous
resize of a frame that's unrelated to fullscreen - you might send OnFullscreen
to it when you shouldn't) and I abandoned it early because I didn't like the
direction it was heading.  I also didn't like keeping and managing that extra
fullscreen state in WebContents when I could avoid it with the current approach.

dcheng

https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Source/core/dom/Element.cpp File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Source/core/dom/Element.cpp#newcode3059 third_party/WebKit/Source/core/dom/Element.cpp:3059: static Element* nextLocalAncestorElement(Element* element) Suggestion: nextAncestorElement, since local is ...

4 years, 7 months ago (2016-05-19 22:08:39 UTC) #22

alexmos

https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Source/core/dom/Element.cpp File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Source/core/dom/Element.cpp#newcode3059 third_party/WebKit/Source/core/dom/Element.cpp:3059: static Element* nextLocalAncestorElement(Element* element) On 2016/05/19 22:08:39, dcheng wrote: ...

4 years, 7 months ago (2016-05-19 23:58:19 UTC) #23

https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:3059: static Element*
nextLocalAncestorElement(Element* element)
On 2016/05/19 22:08:39, dcheng wrote:
> Suggestion: nextAncestorElement, since local is redundant with Element.
> 
> And maybe note that unlike Node::parentNode(), this can cross frame boundaries
> in a comment.

Done.

https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:198:
view()->didUpdateFullScreenSize();
On 2016/05/19 22:08:39, dcheng wrote:
> Why do we need to call this here now?

This is needed for things like opening/resizing a devtools page on the side
while in fullscreen mode.
(I put this comment when I first sent out the CL, but it probably got buried)

https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:1947: FrameOwner* owner =
toWebRemoteFrameImpl(fullscreenFrame)->frame()->owner();
On 2016/05/19 22:08:39, dcheng wrote:
> In a future patch, let's try moving this to WebRemoteFrame.

Will do, I think that should be possible.

https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:1957:
m_fullscreenController->setFullscreenIsForCrossProcessAncestor();
On 2016/05/19 22:08:39, dcheng wrote:
> This seems like the main blocker against moving this: is it possible/feasible
to
> plumb this information through requestFullscreen(), similar to how we're going
> to have to plumb the prefix information through?

Yes, I think that should be feasible.  In fact, once we have that information
plumbed into requestFullscreen(), it can skip some work that's already been
performed in the process of the actual fullscreened element (like the user
gesture check) and just set up the fullscreen element stack and the events. 
(Maybe requestFullscreen() can be split into two functions based on this.)  I'm
assuming it's ok to tackle this in a followup per your comment above?  I've
already got a followup in the works for requestFullscreen() (to support A-B-A
hierarchies), so I'll be tinkering with that code anyway.

https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:1970: WebElement
element(ownerElement);
On 2016/05/19 22:08:39, dcheng wrote:
> Nit: let's use the core API directly, I looked at this, and I think we're
> probably going to remove requestFullScreen() from the public API and move it
to
> WebPluginContainer.

Done.

dcheng

https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp#newcode198 third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:198: view()->didUpdateFullScreenSize(); On 2016/05/19 at 23:58:19, alexmos wrote: > On ...

4 years, 7 months ago (2016-05-20 07:01:44 UTC) #24

https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:198:
view()->didUpdateFullScreenSize();
On 2016/05/19 at 23:58:19, alexmos wrote:
> On 2016/05/19 22:08:39, dcheng wrote:
> > Why do we need to call this here now?
> 
> This is needed for things like opening/resizing a devtools page on the side
> while in fullscreen mode.
> (I put this comment when I first sent out the CL, but it probably got buried)

Hmm. Is WebViewImpl getting this behavior via
WebViewImpl::resizeViewWhileAnchored?

https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:1957:
m_fullscreenController->setFullscreenIsForCrossProcessAncestor();
On 2016/05/19 at 23:58:19, alexmos wrote:
> On 2016/05/19 22:08:39, dcheng wrote:
> > This seems like the main blocker against moving this: is it
possible/feasible to
> > plumb this information through requestFullscreen(), similar to how we're
going
> > to have to plumb the prefix information through?
> 
> Yes, I think that should be feasible.  In fact, once we have that information
plumbed into requestFullscreen(), it can skip some work that's already been
performed in the process of the actual fullscreened element (like the user
gesture check) and just set up the fullscreen element stack and the events. 
(Maybe requestFullscreen() can be split into two functions based on this.)  I'm
assuming it's ok to tackle this in a followup per your comment above?  I've
already got a followup in the works for requestFullscreen() (to support A-B-A
hierarchies), so I'll be tinkering with that code anyway.

Yeah, followup is fine. Thanks!

alexmos

https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp#newcode198 third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:198: view()->didUpdateFullScreenSize(); On 2016/05/20 07:01:44, dcheng wrote: > On 2016/05/19 ...

4 years, 7 months ago (2016-05-20 17:20:13 UTC) #25

https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:198:
view()->didUpdateFullScreenSize();
On 2016/05/20 07:01:44, dcheng wrote:
> On 2016/05/19 at 23:58:19, alexmos wrote:
> > On 2016/05/19 22:08:39, dcheng wrote:
> > > Why do we need to call this here now?
> > 
> > This is needed for things like opening/resizing a devtools page on the side
> > while in fullscreen mode.
> > (I put this comment when I first sent out the CL, but it probably got
buried)
> 
> Hmm. Is WebViewImpl getting this behavior via
> WebViewImpl::resizeViewWhileAnchored?

Yes, in the WebViewImpl case, the resizing happens via this path:

#1 0x7f042cfbe1a9 blink::FullscreenController::updateSize()
#2 0x7f042d0ba3f3 blink::WebViewImpl::resizeViewWhileAnchored()
#3 0x7f042d0ba7e2 blink::WebViewImpl::resizeWithTopControls()
#4 0x7f0435a68391 content::RenderViewImpl::ResizeWebWidget()
#5 0x7f0435a8d587 content::RenderWidget::Resize()
#6 0x7f0435a89f22 content::RenderWidget::OnResize()
#7 0x7f0435a685ee content::RenderViewImpl::OnResize()

In the WebFrameWidgetImpl case, the path is:

#1 0x7f2747fc31a9 blink::FullscreenController::updateSize()
#2 0x7f27480bfb84 blink::WebViewImpl::didUpdateFullScreenSize()
#3 0x7f274804c850 blink::WebFrameWidgetImpl::resizeVisualViewport()
#4 0x7f2750a9264e content::RenderWidget::Resize()
#5 0x7f2750a8ef22 content::RenderWidget::OnResize()

When devtools is open/resized in fullscreen (or when there are gold info bars on
top and they get dismissed, etc.), the thing that matters is that
LayoutFullScreen::updateStyle (called from FullscreenController::updateSize)
picks up the new visual viewport, hence the reason why I put the call to
updateSize in resizeVisualViewport for WebFrameWidgetImpl.

(Note that for this scenario in the WebViewImpl case, you might wonder how
things work since it appears that the explicit resizeVisualViewport update in
RenderWidget::Resize() happens after it returns from ResizeWebWidget.  Actually,
on that path the visual viewport is updated from
blink::WebViewImpl::performResize, while in
blink::WebViewImpl::resizeViewWhileAnchored in the stack above, and that happens
before FullscreenController::updateSize.)

dcheng

On 2016/05/20 at 17:20:13, alexmos wrote: > https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp > File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): > > https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp#newcode198 ...

4 years, 7 months ago (2016-05-20 17:39:12 UTC) #26

On 2016/05/20 at 17:20:13, alexmos wrote:
>
https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):
> 
>
https://codereview.chromium.org/1914643005/diff/220001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:198:
view()->didUpdateFullScreenSize();
> On 2016/05/20 07:01:44, dcheng wrote:
> > On 2016/05/19 at 23:58:19, alexmos wrote:
> > > On 2016/05/19 22:08:39, dcheng wrote:
> > > > Why do we need to call this here now?
> > > 
> > > This is needed for things like opening/resizing a devtools page on the
side
> > > while in fullscreen mode.
> > > (I put this comment when I first sent out the CL, but it probably got
buried)
> > 
> > Hmm. Is WebViewImpl getting this behavior via
> > WebViewImpl::resizeViewWhileAnchored?
> 
> Yes, in the WebViewImpl case, the resizing happens via this path:
> 
> #1 0x7f042cfbe1a9 blink::FullscreenController::updateSize()
> #2 0x7f042d0ba3f3 blink::WebViewImpl::resizeViewWhileAnchored()
> #3 0x7f042d0ba7e2 blink::WebViewImpl::resizeWithTopControls()
> #4 0x7f0435a68391 content::RenderViewImpl::ResizeWebWidget()
> #5 0x7f0435a8d587 content::RenderWidget::Resize()
> #6 0x7f0435a89f22 content::RenderWidget::OnResize()
> #7 0x7f0435a685ee content::RenderViewImpl::OnResize()
> 
> In the WebFrameWidgetImpl case, the path is:
> 
> #1 0x7f2747fc31a9 blink::FullscreenController::updateSize()
> #2 0x7f27480bfb84 blink::WebViewImpl::didUpdateFullScreenSize()
> #3 0x7f274804c850 blink::WebFrameWidgetImpl::resizeVisualViewport()
> #4 0x7f2750a9264e content::RenderWidget::Resize()
> #5 0x7f2750a8ef22 content::RenderWidget::OnResize()
> 
> When devtools is open/resized in fullscreen (or when there are gold info bars
on top and they get dismissed, etc.), the thing that matters is that
LayoutFullScreen::updateStyle (called from FullscreenController::updateSize)
picks up the new visual viewport, hence the reason why I put the call to
updateSize in resizeVisualViewport for WebFrameWidgetImpl.
> 
> (Note that for this scenario in the WebViewImpl case, you might wonder how
things work since it appears that the explicit resizeVisualViewport update in
RenderWidget::Resize() happens after it returns from ResizeWebWidget.  Actually,
on that path the visual viewport is updated from
blink::WebViewImpl::performResize, while in
blink::WebViewImpl::resizeViewWhileAnchored in the stack above, and that happens
before FullscreenController::updateSize.)

This is some scary complexity. It feels like
FullscreenController::didEnterFullscreen() should be doing this work (I think
this path gets hit for both the local and remote frames, right?), but let's
investigate that in a followup.

LGTM

alexmos

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

4 years, 7 months ago (2016-05-23 20:34:43 UTC) #27