Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1592573002:
Fixing WebViewVisibilityTests for OOPIF-<webview>. (Closed)
|
Created:
4 years, 11 months ago by EhsanK Modified:
4 years, 10 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixing WebViewVisibilityTests for OOPIF-<webview>.
When a remote frame's visibility changes, the browser receives an IPC which is
handled in the CrossProcessFrameConnector. For the case of <webview> which
is implemented based on OOPIF, this change in visibility should be forwarded to
the inner (guest) WebContents. This CL adds the required plumbing.
BUG=543179
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/a4f42690319fda189c64c0f755bc26665bcf0e4c
Cr-Commit-Position: refs/heads/master@{#374937}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Added Tests and TODO #
Total comments: 12
Patch Set 3 : Addressing Comments #Patch Set 4 : Merged #
Total comments: 2
Patch Set 5 : BrowserPluginGuest does not Change Visibility of OOPIF-<webview> #
Messages
Total messages: 29 (13 generated)
Description was changed from ========== When a remote frame's visibility changes, the browser receives an IPC which is handled in the CrossProcessFrameConnector. For the case of <webview> which is implemented based on OOPIF, this change in visibility should be forwarded to the inner (guest) WebContents. This CL adds the required plumbing. BUG=543179 ========== to ========== When a remote frame's visibility changes, the browser receives an IPC which is handled in the CrossProcessFrameConnector. For the case of <webview> which is implemented based on OOPIF, this change in visibility should be forwarded to the inner (guest) WebContents. This CL adds the required plumbing. BUG=543179 ==========
Description was changed from ========== When a remote frame's visibility changes, the browser receives an IPC which is handled in the CrossProcessFrameConnector. For the case of <webview> which is implemented based on OOPIF, this change in visibility should be forwarded to the inner (guest) WebContents. This CL adds the required plumbing. BUG=543179 ========== to ========== Fixing WebViewVisibilityTests When a remote frame's visibility changes, the browser receives an IPC which is handled in the CrossProcessFrameConnector. For the case of <webview> which is implemented based on OOPIF, this change in visibility should be forwarded to the inner (guest) WebContents. This CL adds the required plumbing. BUG=543179 ==========
Description was changed from ========== Fixing WebViewVisibilityTests When a remote frame's visibility changes, the browser receives an IPC which is handled in the CrossProcessFrameConnector. For the case of <webview> which is implemented based on OOPIF, this change in visibility should be forwarded to the inner (guest) WebContents. This CL adds the required plumbing. BUG=543179 ========== to ========== Fixing WebViewVisibilityTests for OOPIF-<webview>. When a remote frame's visibility changes, the browser receives an IPC which is handled in the CrossProcessFrameConnector. For the case of <webview> which is implemented based on OOPIF, this change in visibility should be forwarded to the inner (guest) WebContents. This CL adds the required plumbing. BUG=543179 ==========
ekaramad@chromium.org changed reviewers: + lazyboy@chromium.org
PTAL.
I think you need to tweak the description of the CL a bit to say you're fixing some visibility issue in webview, not just fixing a test. Ty. https://codereview.chromium.org/1592573002/diff/1/components/guest_view/brows... File components/guest_view/browser/guest_view_base.cc (left): https://codereview.chromium.org/1592573002/diff/1/components/guest_view/brows... components/guest_view/browser/guest_view_base.cc:603: // TODO(lazyboy): This breaks guest visibility in --site-per-process because Great. Does this take care of all the visibility related issues? Also, I don't see any tests being enabled in this CL. https://codereview.chromium.org/1592573002/diff/1/content/browser/frame_host/... File content/browser/frame_host/cross_process_frame_connector.cc (right): https://codereview.chromium.org/1592573002/diff/1/content/browser/frame_host/... content/browser/frame_host/cross_process_frame_connector.cc:236: // For <webview>, the WebContents should be notified of the change. The Avoid using "<webview>". Use "inner WebContents" instead.
Patchset #2 (id:20001) has been deleted
ekaramad@chromium.org changed reviewers: + kenrb@chromium.org, sievers@chromium.org
Adding kenrb@ for RWHVCF. Adding sievers@ for the missing permissions. Please take a look. Thanks! https://codereview.chromium.org/1592573002/diff/1/components/guest_view/brows... File components/guest_view/browser/guest_view_base.cc (left): https://codereview.chromium.org/1592573002/diff/1/components/guest_view/brows... components/guest_view/browser/guest_view_base.cc:603: // TODO(lazyboy): This breaks guest visibility in --site-per-process because On 2016/01/16 00:46:47, lazyboy wrote: > Great. > Does this take care of all the visibility related issues? > Also, I don't see any tests being enabled in this CL. We had three visibility tests where the first two (<webivew>/parent going invisible) passed after the plumbing. These lines were breaking the third test which was testHiddenBeforeNavigate in shim/main. https://codereview.chromium.org/1592573002/diff/1/content/browser/frame_host/... File content/browser/frame_host/cross_process_frame_connector.cc (right): https://codereview.chromium.org/1592573002/diff/1/content/browser/frame_host/... content/browser/frame_host/cross_process_frame_connector.cc:236: // For <webview>, the WebContents should be notified of the change. The On 2016/01/16 00:46:48, lazyboy wrote: > Avoid using "<webview>". Use "inner WebContents" instead. Done. https://codereview.chromium.org/1592573002/diff/40001/content/public/common/c... File content/public/common/content_switches.h (right): https://codereview.chromium.org/1592573002/diff/40001/content/public/common/c... content/public/common/content_switches.h:27: CONTENT_EXPORT extern const char kUseCrossProcessFramesForGuests[]; I need to add this flag at runtime during the command line setup.
guest_view stuffs LGTM. https://chromiumcodereview.appspot.com/1592573002/diff/40001/content/browser/... File content/browser/frame_host/cross_process_frame_connector.cc (right): https://chromiumcodereview.appspot.com/1592573002/diff/40001/content/browser/... content/browser/frame_host/cross_process_frame_connector.cc:236: // If there is an inner WebContents, iot should be notified of the change in s/iot/it.
On 2016/01/20 00:00:03, ehsaaan wrote: > Adding kenrb@ for RWHVCF. > Adding sievers@ for the missing permissions. > > Please take a look. > > Thanks! > > https://codereview.chromium.org/1592573002/diff/1/components/guest_view/brows... > File components/guest_view/browser/guest_view_base.cc (left): > > https://codereview.chromium.org/1592573002/diff/1/components/guest_view/brows... > components/guest_view/browser/guest_view_base.cc:603: // TODO(lazyboy): This > breaks guest visibility in --site-per-process because > On 2016/01/16 00:46:47, lazyboy wrote: > > Great. > > Does this take care of all the visibility related issues? > > Also, I don't see any tests being enabled in this CL. > > We had three visibility tests where the first two (<webivew>/parent going > invisible) passed after the plumbing. These lines were breaking the third test > which was testHiddenBeforeNavigate in shim/main. > > https://codereview.chromium.org/1592573002/diff/1/content/browser/frame_host/... > File content/browser/frame_host/cross_process_frame_connector.cc (right): > > https://codereview.chromium.org/1592573002/diff/1/content/browser/frame_host/... > content/browser/frame_host/cross_process_frame_connector.cc:236: // For > <webview>, the WebContents should be notified of the change. The > On 2016/01/16 00:46:48, lazyboy wrote: > > Avoid using "<webview>". Use "inner WebContents" instead. > > Done. > > https://codereview.chromium.org/1592573002/diff/40001/content/public/common/c... > File content/public/common/content_switches.h (right): > > https://codereview.chromium.org/1592573002/diff/40001/content/public/common/c... > content/public/common/content_switches.h:27: CONTENT_EXPORT extern const char > kUseCrossProcessFramesForGuests[]; > I need to add this flag at runtime during the command line setup. Correction: Adding kenrb@ for CrossProcessFrameConnector changes.
Mostly looks fine. A couple of minor comments. https://codereview.chromium.org/1592573002/diff/40001/content/browser/frame_h... File content/browser/frame_host/cross_process_frame_connector.cc (right): https://codereview.chromium.org/1592573002/diff/40001/content/browser/frame_h... content/browser/frame_host/cross_process_frame_connector.cc:237: // the visibility. I would suggest expanding the comment to explain why the early return is needed here, omitting the call to RenderWidgetHostView::Show()/Hide(). https://codereview.chromium.org/1592573002/diff/40001/content/public/common/c... File content/public/common/content_switches.h (right): https://codereview.chromium.org/1592573002/diff/40001/content/public/common/c... content/public/common/content_switches.h:27: CONTENT_EXPORT extern const char kUseCrossProcessFramesForGuests[]; On 2016/01/20 00:00:03, ehsaaan wrote: > I need to add this flag at runtime during the command line setup. Does kSitePerProcess not work?
PTAL. https://codereview.chromium.org/1592573002/diff/40001/content/browser/frame_h... File content/browser/frame_host/cross_process_frame_connector.cc (right): https://codereview.chromium.org/1592573002/diff/40001/content/browser/frame_h... content/browser/frame_host/cross_process_frame_connector.cc:236: // If there is an inner WebContents, iot should be notified of the change in On 2016/01/20 00:45:35, lazyboy wrote: > s/iot/it. Done. https://codereview.chromium.org/1592573002/diff/40001/content/browser/frame_h... content/browser/frame_host/cross_process_frame_connector.cc:237: // the visibility. On 2016/01/22 22:26:01, kenrb wrote: > I would suggest expanding the comment to explain why the early return is needed > here, omitting the call to RenderWidgetHostView::Show()/Hide(). Done. https://codereview.chromium.org/1592573002/diff/40001/content/public/common/c... File content/public/common/content_switches.h (right): https://codereview.chromium.org/1592573002/diff/40001/content/public/common/c... content/public/common/content_switches.h:27: CONTENT_EXPORT extern const char kUseCrossProcessFramesForGuests[]; On 2016/01/22 22:26:02, kenrb wrote: > On 2016/01/20 00:00:03, ehsaaan wrote: > > I need to add this flag at runtime during the command line setup. > > Does kSitePerProcess not work? This is the switch we should have active for <webview> based on OOPIF. In the tests, I am activating both --site-per-process and --use-cross-process-frames-for-guests.
https://codereview.chromium.org/1592573002/diff/40001/content/browser/frame_h... File content/browser/frame_host/cross_process_frame_connector.cc (right): https://codereview.chromium.org/1592573002/diff/40001/content/browser/frame_h... content/browser/frame_host/cross_process_frame_connector.cc:243: ->ForwardVisibilityChange(visible); Something feels off about this needing to bubble up to the WebContents as discussed offline. It would seem more natural if the |view_| could propagate this on its own, but it sounds like the hierarchy is lost to the RWHVChildFrame, the way things currently are. https://codereview.chromium.org/1592573002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/1592573002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_delegate.h:184: // renderer process. Can you make the comment and function name more specific? Maybe OnChildFrameVisibilityChanged() or something?
Patchset #3 (id:60001) has been deleted
PTAL. https://codereview.chromium.org/1592573002/diff/40001/content/browser/frame_h... File content/browser/frame_host/cross_process_frame_connector.cc (right): https://codereview.chromium.org/1592573002/diff/40001/content/browser/frame_h... content/browser/frame_host/cross_process_frame_connector.cc:243: ->ForwardVisibilityChange(visible); On 2016/01/26 20:33:06, sievers wrote: > Something feels off about this needing to bubble up to the WebContents as > discussed offline. > > It would seem more natural if the |view_| could propagate this on its own, but > it sounds like the hierarchy is lost to the RWHVChildFrame, the way things > currently are. Acknowledged. As there is a separate WebContents here I believe it has to be notified the way things are right now (For oopif-based <webview>). Whether there will be a hierarchy for RWHVs and following that, a better way to seamlessly forward the change to the inner WebContents is something I will try to find out. I should also emphasize that some of the WebViewVisibilityTests directly rely on the WebContentsObserver being called on a visibility change at the WebContents level. That is why calling WebContents::WasShown/Hidden is necessary. https://codereview.chromium.org/1592573002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/1592573002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_delegate.h:184: // renderer process. On 2016/01/26 20:33:07, sievers wrote: > Can you make the comment and function name more specific? > Maybe OnChildFrameVisibilityChanged() or something? Done. I am explicitly naming the function based on what has changed. In this case, the actual entity sending the visibility change is a "RenderFrameProxy".
lgtm from my side. https://codereview.chromium.org/1592573002/diff/40001/content/browser/frame_h... File content/browser/frame_host/cross_process_frame_connector.cc (right): https://codereview.chromium.org/1592573002/diff/40001/content/browser/frame_h... content/browser/frame_host/cross_process_frame_connector.cc:243: ->ForwardVisibilityChange(visible); On 2016/01/26 20:33:06, sievers wrote: > Something feels off about this needing to bubble up to the WebContents as > discussed offline. > > It would seem more natural if the |view_| could propagate this on its own, but > it sounds like the hierarchy is lost to the RWHVChildFrame, the way things > currently are. I might not entirely understand your reasoning, but this approach looks okay to me. This method is for passing down a visibility message from a Frame object in a parent renderer process to the RenderWidgetHostView of the actual content, and it seems like the right place to have the knowledge of the fact that the content within this Frame (or <webview>, in this case) has its own WebContents that should get the message instead. Importantly, it isn't returning the message to the same WebContents that might have initiated the visibility change with WasShown() or WasHidden(). This is basically recursion.
Description was changed from ========== Fixing WebViewVisibilityTests for OOPIF-<webview>. When a remote frame's visibility changes, the browser receives an IPC which is handled in the CrossProcessFrameConnector. For the case of <webview> which is implemented based on OOPIF, this change in visibility should be forwarded to the inner (guest) WebContents. This CL adds the required plumbing. BUG=543179 ========== to ========== Fixing WebViewVisibilityTests for OOPIF-<webview>. When a remote frame's visibility changes, the browser receives an IPC which is handled in the CrossProcessFrameConnector. For the case of <webview> which is implemented based on OOPIF, this change in visibility should be forwarded to the inner (guest) WebContents. This CL adds the required plumbing. BUG=543179 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1592573002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1592573002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm https://codereview.chromium.org/1592573002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/1592573002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_delegate.h:178: // Called when the widget has sent a compositor proto. This is used in Btlimp typo https://codereview.chromium.org/1592573002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_delegate.h:183: // Called when the visibility of the RenderFrameProxyHost in outter nit: s/outter/outer
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org, kenrb@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1592573002/#ps120001 (title: "BrowserPluginGuest does not Change Visibility of OOPIF-<webview>")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1592573002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1592573002/120001
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Fixing WebViewVisibilityTests for OOPIF-<webview>. When a remote frame's visibility changes, the browser receives an IPC which is handled in the CrossProcessFrameConnector. For the case of <webview> which is implemented based on OOPIF, this change in visibility should be forwarded to the inner (guest) WebContents. This CL adds the required plumbing. BUG=543179 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fixing WebViewVisibilityTests for OOPIF-<webview>. When a remote frame's visibility changes, the browser receives an IPC which is handled in the CrossProcessFrameConnector. For the case of <webview> which is implemented based on OOPIF, this change in visibility should be forwarded to the inner (guest) WebContents. This CL adds the required plumbing. BUG=543179 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/a4f42690319fda189c64c0f755bc26665bcf0e4c Cr-Commit-Position: refs/heads/master@{#374937} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a4f42690319fda189c64c0f755bc26665bcf0e4c Cr-Commit-Position: refs/heads/master@{#374937} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
