Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2832093003:
Implement screen orientation for out-of-process iframes. (Closed)
|
Created:
3 years, 8 months ago by lfg Modified:
3 years, 7 months ago CC:
blink-reviews, blink-reviews-frames_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, mlamouri+watch-content_chromium.org, mlamouri+watch-screen-orientation_chromium.org, nasko+codewatch_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement screen orientation for out-of-process iframes.
This change adds support for sending screen orientation change events
to out-of-process iframes as well as fixing the Screen.orientation
property on OOPIFs.
This change adds a PageMsg_UpdateScreenInfo that updates the
ScreenInfo for subframe renderers. The screen orientation is
still updated on the ViewMsg_Resize for the main frame.
The flow of dispatching the screen orientation update is:
1. The browser sends a ViewMsg_Resize to the main frame's renderer. The browser also sends PageMsg_UpdateScreenInfo to the OOPIF renderers.
2. The main frame renderer dispatches FrameHostMsg_FrameRectsChanged back to the browser process.
3. The browser process dispatches ViewMsg_Resize to the subframe renderers.
4. The subframe renderers dispatch the screen orientation event when they receive the resize IPC.
BUG=498287
Review-Url: https://codereview.chromium.org/2832093003
Cr-Commit-Position: refs/heads/master@{#468057}
Committed: https://chromium.googlesource.com/chromium/src/+/8d649cc1eb710240e71a02c6b44d4f1d245c3f19
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : fix dcheck #
Total comments: 12
Patch Set 4 : rebase #Patch Set 5 : add comments #
Messages
Total messages: 40 (24 generated)
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lfg@chromium.org changed reviewers: + mlamouri@chromium.org
mlamouri, can you take a look?
Description was changed from ========== Implement screen orientation for out-of-process iframes. This change adds support for sending screen orientation change events to out-of-process iframes as well as fixing the Screen.orientation property on OOPIFs. BUG=498287 ========== to ========== Implement screen orientation for out-of-process iframes. This change adds support for sending screen orientation change events to out-of-process iframes as well as fixing the Screen.orientation property on OOPIFs. BUG=498287 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with the question inline adressed https://codereview.chromium.org/2832093003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2832093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:549: Event::Create(EventTypeNames::orientationchange)); Note that screen orientation has two parts. One of them lives in ScreenOrientationControllerImpl::NotifyOrientationChanged(). It seems that it should work too but I prefer to make sure you are aware of this :)
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2832093003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2832093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:545: } What is this change for? Does TraverseNext not traverse all child frames? https://codereview.chromium.org/2832093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:549: Event::Create(EventTypeNames::orientationchange)); On 2017/04/25 11:06:47, mlamouri wrote: > Note that screen orientation has two parts. One of them lives in > ScreenOrientationControllerImpl::NotifyOrientationChanged(). It seems that it > should work too but I prefer to make sure you are aware of this :) Yeah , I'd also prefer not adding feature-specific code to LocalDOMWindow :)
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/2832093003/diff/40001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2832093003/diff/40001/content/renderer/render... content/renderer/render_view_impl.cc:2300: // ViewMsg_Resize. Is this something we'll be able to consolidate eventually? It's hard to follow exactly how the various IPCs here interact to result in screen orientation events being generated for everyone. (Maybe the CL description could have a brief overview of how the screen orientation change event plumbing works for both cases?)
Please, take another look. https://codereview.chromium.org/2832093003/diff/40001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2832093003/diff/40001/content/renderer/render... content/renderer/render_view_impl.cc:2300: // ViewMsg_Resize. On 2017/04/25 13:10:44, dcheng (OOO through May 2) wrote: > Is this something we'll be able to consolidate eventually? It's hard to follow > exactly how the various IPCs here interact to result in screen orientation > events being generated for everyone. I've implemented it this way so the OOPIF-change won't affect the way local frames are rendered. I'm of the opinion that ScreenInfo and ResizeParams should be decoupled, I think this is possible but its difficult (see https://crbug.com/366848 and https://crbug.com/239624) and not something I'm planning on tackling. > (Maybe the CL description could have a brief overview of how the screen > orientation change event plumbing works for both cases?) I can change that. https://codereview.chromium.org/2832093003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2832093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:545: } On 2017/04/25 12:35:39, haraken wrote: > > What is this change for? Does TraverseNext not traverse all child frames? The problem with TraverseNext is that it traverses all child frames (including remote frames), but we only want to traverse the local frames within the current local root. For example, if we have A->B->A (a.com with a subframe in b.com with a subframe in a.com), then using TraverseNext would traverse the local frames within the subframe in A. But we don't want to do that, as the WebFrameWidget in A's subframe will also get the event. https://codereview.chromium.org/2832093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:549: Event::Create(EventTypeNames::orientationchange)); On 2017/04/25 11:06:47, mlamouri wrote: > Note that screen orientation has two parts. One of them lives in > ScreenOrientationControllerImpl::NotifyOrientationChanged(). It seems that it > should work too but I prefer to make sure you are aware of this :) Yes, this change is basically aligning LocalDOMWindow with ScreenOrientationControllerImpl. The implementation in ScreenOrientationControllerImpl is recursive, so it's slightly different, but it essentially does the same thing (skips the RemoteFrames and its children). See the comment above to haraken@'s question for a detailed explanation.
Description was changed from ========== Implement screen orientation for out-of-process iframes. This change adds support for sending screen orientation change events to out-of-process iframes as well as fixing the Screen.orientation property on OOPIFs. BUG=498287 ========== to ========== Implement screen orientation for out-of-process iframes. This change adds support for sending screen orientation change events to out-of-process iframes as well as fixing the Screen.orientation property on OOPIFs. This change adds a PageMsg_UpdateScreenInfo that updates the ScreenInfo for subframe renderers. The screen orientation is still updated on the ViewMsg_Resize for the main frame. BUG=498287 ==========
WebKit LGTM https://codereview.chromium.org/2832093003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2832093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:548: frame->DomWindow()->DispatchEvent( Nit: We should have renamed DomWindow to DOMWindow :) https://codereview.chromium.org/2832093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:549: Event::Create(EventTypeNames::orientationchange)); On 2017/04/25 15:44:31, lfg wrote: > On 2017/04/25 11:06:47, mlamouri wrote: > > Note that screen orientation has two parts. One of them lives in > > ScreenOrientationControllerImpl::NotifyOrientationChanged(). It seems that it > > should work too but I prefer to make sure you are aware of this :) > > Yes, this change is basically aligning LocalDOMWindow with > ScreenOrientationControllerImpl. The implementation in > ScreenOrientationControllerImpl is recursive, so it's slightly different, but it > essentially does the same thing (skips the RemoteFrames and its children). See > the comment above to haraken@'s question for a detailed explanation. Can we clean up the code a bit more so that we don't need to have slightly different logics in two different files? A follow-up CL is fine.
Thanks, I'll put other CLs for the cleanups. On 2017/04/25 16:21:03, haraken wrote: > https://codereview.chromium.org/2832093003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:548: > frame->DomWindow()->DispatchEvent( > > Nit: We should have renamed DomWindow to DOMWindow :) > > https://codereview.chromium.org/2832093003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:549: > Event::Create(EventTypeNames::orientationchange)); > On 2017/04/25 15:44:31, lfg wrote: > > On 2017/04/25 11:06:47, mlamouri wrote: > > > Note that screen orientation has two parts. One of them lives in > > > ScreenOrientationControllerImpl::NotifyOrientationChanged(). It seems that > it > > > should work too but I prefer to make sure you are aware of this :) > > > > Yes, this change is basically aligning LocalDOMWindow with > > ScreenOrientationControllerImpl. The implementation in > > ScreenOrientationControllerImpl is recursive, so it's slightly different, but > it > > essentially does the same thing (skips the RemoteFrames and its children). See > > the comment above to haraken@'s question for a detailed explanation. > > Can we clean up the code a bit more so that we don't need to have slightly > different logics in two different files? A follow-up CL is fine.
lfg@chromium.org changed reviewers: + nasko@chromium.org
Nasko, can you take a look?
Description was changed from ========== Implement screen orientation for out-of-process iframes. This change adds support for sending screen orientation change events to out-of-process iframes as well as fixing the Screen.orientation property on OOPIFs. This change adds a PageMsg_UpdateScreenInfo that updates the ScreenInfo for subframe renderers. The screen orientation is still updated on the ViewMsg_Resize for the main frame. BUG=498287 ========== to ========== Implement screen orientation for out-of-process iframes. This change adds support for sending screen orientation change events to out-of-process iframes as well as fixing the Screen.orientation property on OOPIFs. This change adds a PageMsg_UpdateScreenInfo that updates the ScreenInfo for subframe renderers. The screen orientation is still updated on the ViewMsg_Resize for the main frame. BUG=498287 ==========
I have one question, but overall LGTM. https://codereview.chromium.org/2832093003/diff/40001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_browsertest.cc (right): https://codereview.chromium.org/2832093003/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_browsertest.cc:88: rwh->Send(new ViewMsg_Resize(rwh->GetRoutingID(), params)); Do we actually iterate through RWHs in real code when changing orientation?
https://codereview.chromium.org/2832093003/diff/40001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_browsertest.cc (right): https://codereview.chromium.org/2832093003/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_browsertest.cc:88: rwh->Send(new ViewMsg_Resize(rwh->GetRoutingID(), params)); On 2017/04/25 19:59:08, nasko wrote: > Do we actually iterate through RWHs in real code when changing orientation? Not exactly like this, the test is faking it. What really happens is: 1. The top-level frame is resized, we send the ViewMsg_Resize to it. 2. This calls delegate->RenderWidgetWasResized, which triggers the MainFrameWasResized observer that sends the PageMsg_UpdateScreenInfo to the OOPIF subframes. 3. (1) will cause the renderer to call FrameRectsChanged on the RenderFrameProxy. 4. This will send an IPC to the browser that will cause the browser to send ViewMsg_Resize to the subframe renderer. 5. Repeat 3-4 for nested subframe renderers. I tried writing a test that actually used the whole code path for this, but it proved to be fairly complicated, as we would have to mock either WebContentsImpl itself or all of the RenderWidgetHostViews for every platform plus RenderWidgetHostViewChildFrame. So this test only really tests that the renderer is doing the correct thing (it is the same for the other screen orientation tests).
On 2017/04/25 20:08:18, lfg wrote: > https://codereview.chromium.org/2832093003/diff/40001/content/browser/screen_... > File content/browser/screen_orientation/screen_orientation_browsertest.cc > (right): > > https://codereview.chromium.org/2832093003/diff/40001/content/browser/screen_... > content/browser/screen_orientation/screen_orientation_browsertest.cc:88: > rwh->Send(new ViewMsg_Resize(rwh->GetRoutingID(), params)); > On 2017/04/25 19:59:08, nasko wrote: > > Do we actually iterate through RWHs in real code when changing orientation? > > Not exactly like this, the test is faking it. What really happens is: > 1. The top-level frame is resized, we send the ViewMsg_Resize to it. > 2. This calls delegate->RenderWidgetWasResized, which triggers the > MainFrameWasResized observer that sends the PageMsg_UpdateScreenInfo to the > OOPIF subframes. > 3. (1) will cause the renderer to call FrameRectsChanged on the > RenderFrameProxy. > 4. This will send an IPC to the browser that will cause the browser to send > ViewMsg_Resize to the subframe renderer. > 5. Repeat 3-4 for nested subframe renderers. > > I tried writing a test that actually used the whole code path for this, but it > proved to be fairly complicated, as we would have to mock either WebContentsImpl > itself or all of the RenderWidgetHostViews for every platform plus > RenderWidgetHostViewChildFrame. So this test only really tests that the renderer > is doing the correct thing (it is the same for the other screen orientation > tests). Thanks for the details! If you think it will be useful to have some of that in a comment, I'd totally support it ;).
https://codereview.chromium.org/2832093003/diff/40001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2832093003/diff/40001/content/renderer/render... content/renderer/render_view_impl.cc:2300: // ViewMsg_Resize. On 2017/04/25 15:44:31, lfg wrote: > On 2017/04/25 13:10:44, dcheng (OOO through May 2) wrote: > > Is this something we'll be able to consolidate eventually? It's hard to follow > > exactly how the various IPCs here interact to result in screen orientation > > events being generated for everyone. > > I've implemented it this way so the OOPIF-change won't affect the way local > frames are rendered. I'm of the opinion that ScreenInfo and ResizeParams should > be decoupled, I think this is possible but its difficult (see > https://crbug.com/366848 and https://crbug.com/239624) and not something I'm > planning on tackling. > > > (Maybe the CL description could have a brief overview of how the screen > > orientation change event plumbing works for both cases?) > > I can change that. It's not clear to me from the CL description how simply setting a class field results in screen orientation events being dispatched.
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Implement screen orientation for out-of-process iframes. This change adds support for sending screen orientation change events to out-of-process iframes as well as fixing the Screen.orientation property on OOPIFs. This change adds a PageMsg_UpdateScreenInfo that updates the ScreenInfo for subframe renderers. The screen orientation is still updated on the ViewMsg_Resize for the main frame. BUG=498287 ========== to ========== Implement screen orientation for out-of-process iframes. This change adds support for sending screen orientation change events to out-of-process iframes as well as fixing the Screen.orientation property on OOPIFs. This change adds a PageMsg_UpdateScreenInfo that updates the ScreenInfo for subframe renderers. The screen orientation is still updated on the ViewMsg_Resize for the main frame. The flow of dispatching the screen orientation update is: 1. The browser sends a ViewMsg_Resize to the main frame's renderer. The browser also sends PageMsg_UpdateScreenInfo to the OOPIF renderers. 2. The main frame renderer dispatches FrameHostMsg_FrameRectsChanged back to the browser process. 3. The browser process dispatches ViewMsg_Resize to the subframe renderers. 4. The subframe renderers dispatch the screen orientation event when they receive the resize IPC. BUG=498287 ==========
On 2017/04/25 21:25:33, nasko wrote: > Thanks for the details! If you think it will be useful to have some of that in a > comment, I'd totally support it ;). Done. On 2017/04/25 23:32:05, dcheng (OOO through May 2) wrote: > It's not clear to me from the CL description how simply setting a class field > results in screen orientation events being dispatched. Ah, sorry, I think the main point is that ViewMsg_Resize is still the cause of the events being dispatched. Can you do another pass to check if the description is ok now?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, makes sense. LGTM
The CQ bit was checked by lfg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, nasko@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2832093003/#ps80001 (title: "add comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493395592269950,
"parent_rev": "91f1e343eb3fcc6694c740548e2928519b0350f3", "commit_rev":
"8d649cc1eb710240e71a02c6b44d4f1d245c3f19"}
Message was sent while issue was closed.
Description was changed from ========== Implement screen orientation for out-of-process iframes. This change adds support for sending screen orientation change events to out-of-process iframes as well as fixing the Screen.orientation property on OOPIFs. This change adds a PageMsg_UpdateScreenInfo that updates the ScreenInfo for subframe renderers. The screen orientation is still updated on the ViewMsg_Resize for the main frame. The flow of dispatching the screen orientation update is: 1. The browser sends a ViewMsg_Resize to the main frame's renderer. The browser also sends PageMsg_UpdateScreenInfo to the OOPIF renderers. 2. The main frame renderer dispatches FrameHostMsg_FrameRectsChanged back to the browser process. 3. The browser process dispatches ViewMsg_Resize to the subframe renderers. 4. The subframe renderers dispatch the screen orientation event when they receive the resize IPC. BUG=498287 ========== to ========== Implement screen orientation for out-of-process iframes. This change adds support for sending screen orientation change events to out-of-process iframes as well as fixing the Screen.orientation property on OOPIFs. This change adds a PageMsg_UpdateScreenInfo that updates the ScreenInfo for subframe renderers. The screen orientation is still updated on the ViewMsg_Resize for the main frame. The flow of dispatching the screen orientation update is: 1. The browser sends a ViewMsg_Resize to the main frame's renderer. The browser also sends PageMsg_UpdateScreenInfo to the OOPIF renderers. 2. The main frame renderer dispatches FrameHostMsg_FrameRectsChanged back to the browser process. 3. The browser process dispatches ViewMsg_Resize to the subframe renderers. 4. The subframe renderers dispatch the screen orientation event when they receive the resize IPC. BUG=498287 Review-Url: https://codereview.chromium.org/2832093003 Cr-Commit-Position: refs/heads/master@{#468057} Committed: https://chromium.googlesource.com/chromium/src/+/8d649cc1eb710240e71a02c6b44d... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/8d649cc1eb710240e71a02c6b44d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
