Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1586923002:
[UseZoomForDSF] Guest view support (Closed)
|
Created:
4 years, 11 months ago by oshima Modified:
4 years, 11 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, extensions-reviews_chromium.org, nasko+codewatch_chromium.org, jam, blink-reviews, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, chromium-apps-reviews_chromium.org, tdresser+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[UseZoomForDSF] Guest view support
* Do not convert inputs for child frames because they're already converted
* Use 1x dsf for the child frame's surface. This is a stopgap solution. I'll address it in a separate CL.
* Convert the guest view size to css in browser_plugin as all use of the size expects css coordinates.
BUG=485650
TEST=WebViewWithZoomForDSFTest.*
Committed: https://crrev.com/9000e422526a1795be84f2571b5e65f431c73f2f
Cr-Commit-Position: refs/heads/master@{#371695}
Patch Set 1 : #Patch Set 2 : #
Total comments: 7
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 1
Messages
Total messages: 69 (29 generated)
The CQ bit was checked by oshima@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/1586923002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1586923002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by oshima@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/1586923002/70001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1586923002/70001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by oshima@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/1586923002/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1586923002/110001
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:70001) has been deleted
Patchset #1 (id:90001) has been deleted
Description was changed from ========== GuestView BUG=485650 ========== to ========== [UseZoomForDSF] Guest view support * Do not convert inputs for child frames because they're already converted * Use 1x dsf for the child frame's surface. This is a stopgap fix, and should be handled in cc/. I'll address it in a separate CL. * Convert the guest view size to css in browser_plugin as all use of the size expects css coordinates. BUG=485650 TEST=WebViewWithZoomForDSFTest.* ==========
oshima@chromium.org changed reviewers: + lazyboy@chromium.org
Description was changed from ========== [UseZoomForDSF] Guest view support * Do not convert inputs for child frames because they're already converted * Use 1x dsf for the child frame's surface. This is a stopgap fix, and should be handled in cc/. I'll address it in a separate CL. * Convert the guest view size to css in browser_plugin as all use of the size expects css coordinates. BUG=485650 TEST=WebViewWithZoomForDSFTest.* ========== to ========== [UseZoomForDSF] Guest view support * Do not convert inputs for child frames because they're already converted * Use 1x dsf for the child frame's surface. This is a stopgap solution. I'll address it in a separate CL. * Convert the guest view size to css in browser_plugin as all use of the size expects css coordinates. BUG=485650 TEST=WebViewWithZoomForDSFTest.* ==========
The CQ bit was checked by oshima@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/1586923002/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1586923002/130001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by oshima@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/1586923002/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1586923002/130001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lazyboy@chromium.org changed reviewers: + kenrb@chromium.org, wjmaclean@chromium.org
+James for reviewing the browser plugin code and +Ken for RWHVChildFrame. Think they are more familiar in this area than I am.
On 2016/01/14 21:18:11, lazyboy wrote: > +James for reviewing the browser plugin code and +Ken for RWHVChildFrame. Think > they are more familiar in this area than I am. kenrb@, wjmaclean@, let me know if you have any question.
https://codereview.chromium.org/1586923002/diff/130001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/1586923002/diff/130001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:52: host_->set_scale_input_to_viewport(false); I think that comment might not be generally true. If you are only testing with BrowserPlugin, then I suspect it is true for mouse events but not touch events (wjmaclean might be able to speak more definitively on that). For out-of-process iframes, I don't think any of the input will have been scaled already, because mouse and mousewheel events do not pass through the top-level RenderWidgetHost anymore. https://codereview.chromium.org/1586923002/diff/130001/content/renderer/brows... File content/renderer/browser_plugin/browser_plugin.h (right): https://codereview.chromium.org/1586923002/diff/130001/content/renderer/brows... content/renderer/browser_plugin/browser_plugin.h:177: // The plugins rect in css pixels. Nit: s/plugins/plugin's https://codereview.chromium.org/1586923002/diff/130001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1586923002/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3088: LocalFrame* frame = mainFrameImpl() ? mainFrameImpl()->frame() : nullptr; It isn't clear to me why mainFrameImpl() would return nullptr here, given that the previous check guarantees there is not a remote main frame. If this is needed, though, then we can remove lines 3085 and 3086, which become redundant.
Patchset #3 (id:150001) has been deleted
https://codereview.chromium.org/1586923002/diff/130001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/1586923002/diff/130001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:52: host_->set_scale_input_to_viewport(false); On 2016/01/15 20:09:12, kenrb wrote: > I think that comment might not be generally true. If you are only testing with > BrowserPlugin, then I suspect it is true for mouse events but not touch events > (wjmaclean might be able to speak more definitively on that). For out-of-process > iframes, I don't think any of the input will have been scaled already, because > mouse and mousewheel events do not pass through the top-level RenderWidgetHost > anymore. Thanks for heads up. That's actually good news because it simplifies this logic. I added TODO for OOPIF support. Touch event seems to be working fine though. https://codereview.chromium.org/1586923002/diff/130001/content/renderer/brows... File content/renderer/browser_plugin/browser_plugin.h (right): https://codereview.chromium.org/1586923002/diff/130001/content/renderer/brows... content/renderer/browser_plugin/browser_plugin.h:177: // The plugins rect in css pixels. On 2016/01/15 20:09:12, kenrb wrote: > Nit: s/plugins/plugin's Done. https://codereview.chromium.org/1586923002/diff/130001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1586923002/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3088: LocalFrame* frame = mainFrameImpl() ? mainFrameImpl()->frame() : nullptr; On 2016/01/15 20:09:12, kenrb wrote: > It isn't clear to me why mainFrameImpl() would return nullptr here, given that > the previous check guarantees there is not a remote main frame. If this is > needed, though, then we can remove lines 3085 and 3086, which become redundant. It was crashing when I made the original version of this CL, but that seems to be fixed in https://codereview.chromium.org/1546603003. Reverted this file, thanks.
The CQ bit was checked by oshima@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/1586923002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1586923002/170001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
friendly ping?
I have a question: I'm assuming by 'Zoom' we're referring to PinchZoom (a.k.a. page scale factor), and not HostZoomMap-zoom, is that correct? Perhaps you can make this explicit in the description. Generally looks ok. A few comments below: https://codereview.chromium.org/1586923002/diff/170001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/1586923002/diff/170001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:53: host_->set_scale_input_to_viewport(false); If it should be true for OOPIF, can't we just do host_->set_scale_input_to_viewport(AreCrossProcessFramesPossible() || <the new WebView specific OOPIF flag>); now, as opposed to later? https://codereview.chromium.org/1586923002/diff/170001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1586923002/diff/170001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1268: if (scale_input_to_viewport_) What is the advantage to storing this state? I guess it never changes during the lifetime of the RWHI ... https://codereview.chromium.org/1586923002/diff/170001/content/renderer/brows... File content/renderer/browser_plugin/browser_plugin.cc (right): https://codereview.chromium.org/1586923002/diff/170001/content/renderer/brows... content/renderer/browser_plugin/browser_plugin.cc:445: RenderView::FromWebView(webview)->convertViewportToWindow(&rect_in_css); This sort of looks like we're converting a viewport_rect to a window_rect ... is the input parameter to this function perhaps badly named? Should it be |viewport_rect|?
On 2016/01/20 19:41:55, wjmaclean wrote: > content/browser/frame_host/render_widget_host_view_child_frame.cc:53: > host_->set_scale_input_to_viewport(false); > If it should be true for OOPIF, can't we just do > > host_->set_scale_input_to_viewport(AreCrossProcessFramesPossible() || BrowserPluginGuestMode::UseCrossProcessFramesForGuests()); ******************************************************** > > now, as opposed to later?
Patchset #4 (id:190001) has been deleted
The CQ bit was checked by oshima@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/1586923002/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1586923002/210001
> I have a question: I'm assuming by 'Zoom' we're referring to PinchZoom (a.k.a. page scale factor), and not HostZoomMap-zoom, is that correct? No, it's page-zoom (the one used in browser zoom). In short, chrome will use the page-zoom mechanism to scale the content for devie scale factor. Here is the design doc: https://docs.google.com/document/d/1_40BGUBIGCmII3FaXdh-daOoxcCar7TnVpH0hldYgO8 Please see the bug for discussion. https://codereview.chromium.org/1586923002/diff/170001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/1586923002/diff/170001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:53: host_->set_scale_input_to_viewport(false); On 2016/01/20 19:41:54, wjmaclean wrote: > If it should be true for OOPIF, can't we just do > > host_->set_scale_input_to_viewport(AreCrossProcessFramesPossible() || <the new > WebView specific OOPIF flag>); > > now, as opposed to later? I looked into more, and I think we can just move this to RenderWidgetHostViewGuest, because this is used only when scaling is necessary. (and not used when oopif is used) https://codereview.chromium.org/1586923002/diff/170001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1586923002/diff/170001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1268: if (scale_input_to_viewport_) On 2016/01/20 19:41:54, wjmaclean wrote: > What is the advantage to storing this state? I guess it never changes during the > lifetime of the RWHI ... It will change per RWHImpl instance. https://codereview.chromium.org/1586923002/diff/170001/content/renderer/brows... File content/renderer/browser_plugin/browser_plugin.cc (right): https://codereview.chromium.org/1586923002/diff/170001/content/renderer/brows... content/renderer/browser_plugin/browser_plugin.cc:445: RenderView::FromWebView(webview)->convertViewportToWindow(&rect_in_css); On 2016/01/20 19:41:54, wjmaclean wrote: > This sort of looks like we're converting a viewport_rect to a window_rect ... is > the input parameter to this function perhaps badly named? Should it be > |viewport_rect|? I changed it in plugin_rect_in_viewport. WDYT?
https://codereview.chromium.org/1586923002/diff/130001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/1586923002/diff/130001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:52: host_->set_scale_input_to_viewport(false); On 2016/01/15 22:24:10, oshima wrote: > On 2016/01/15 20:09:12, kenrb wrote: > > I think that comment might not be generally true. If you are only testing with > > BrowserPlugin, then I suspect it is true for mouse events but not touch events > > (wjmaclean might be able to speak more definitively on that). For > out-of-process > > iframes, I don't think any of the input will have been scaled already, because > > mouse and mousewheel events do not pass through the top-level RenderWidgetHost > > anymore. > > Thanks for heads up. That's actually good news because it simplifies this logic. > I added TODO for OOPIF support. Touch event seems to be working fine though. For most of this code I would defer to wjmaclean, but will this break input events on OOPIFs? I would strongly prefer we not do that.
On 2016/01/20 21:43:11, kenrb wrote: > https://codereview.chromium.org/1586923002/diff/130001/content/browser/frame_... > File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): > > https://codereview.chromium.org/1586923002/diff/130001/content/browser/frame_... > content/browser/frame_host/render_widget_host_view_child_frame.cc:52: > host_->set_scale_input_to_viewport(false); > On 2016/01/15 22:24:10, oshima wrote: > > On 2016/01/15 20:09:12, kenrb wrote: > > > I think that comment might not be generally true. If you are only testing > with > > > BrowserPlugin, then I suspect it is true for mouse events but not touch > events > > > (wjmaclean might be able to speak more definitively on that). For > > out-of-process > > > iframes, I don't think any of the input will have been scaled already, > because > > > mouse and mousewheel events do not pass through the top-level > RenderWidgetHost > > > anymore. > > > > Thanks for heads up. That's actually good news because it simplifies this > logic. > > I added TODO for OOPIF support. Touch event seems to be working fine though. > > For most of this code I would defer to wjmaclean, but will this break input > events on OOPIFs? I would strongly prefer we not do that. We are aiming to ship to Beta users in rougly 2-3 months. We cannot be breaking OOPIF.
On 2016/01/20 21:46:19, nasko wrote: > On 2016/01/20 21:43:11, kenrb wrote: > > > https://codereview.chromium.org/1586923002/diff/130001/content/browser/frame_... > > File content/browser/frame_host/render_widget_host_view_child_frame.cc > (right): > > > > > https://codereview.chromium.org/1586923002/diff/130001/content/browser/frame_... > > content/browser/frame_host/render_widget_host_view_child_frame.cc:52: > > host_->set_scale_input_to_viewport(false); > > On 2016/01/15 22:24:10, oshima wrote: > > > On 2016/01/15 20:09:12, kenrb wrote: > > > > I think that comment might not be generally true. If you are only testing > > with > > > > BrowserPlugin, then I suspect it is true for mouse events but not touch > > events > > > > (wjmaclean might be able to speak more definitively on that). For > > > out-of-process > > > > iframes, I don't think any of the input will have been scaled already, > > because > > > > mouse and mousewheel events do not pass through the top-level > > RenderWidgetHost > > > > anymore. > > > > > > Thanks for heads up. That's actually good news because it simplifies this > > logic. > > > I added TODO for OOPIF support. Touch event seems to be working fine though. > > > > For most of this code I would defer to wjmaclean, but will this break input > > events on OOPIFs? I would strongly prefer we not do that. > > We are aiming to ship to Beta users in rougly 2-3 months. We cannot be breaking > OOPIF. I've been making series of changes to get this working and OOPIF is definitely one thing I'm going to fix if there is any issue.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping?
This looks fine ... lgtm
oshima@chromium.org changed reviewers: + sievers@chromium.org
sievers@ -> OWNERS review
https://codereview.chromium.org/1586923002/diff/210001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/1586923002/diff/210001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:66: host_->set_scale_input_to_viewport(false); Is it possible for this |host_| to host an interstitial page or other kind of view? I'm wondering if it's better for the RWHV to query the RWHV for this flag. wdyt?
On 2016/01/26 19:42:35, sievers wrote: > https://codereview.chromium.org/1586923002/diff/210001/content/browser/frame_... > File content/browser/frame_host/render_widget_host_view_guest.cc (right): > > https://codereview.chromium.org/1586923002/diff/210001/content/browser/frame_... > content/browser/frame_host/render_widget_host_view_guest.cc:66: > host_->set_scale_input_to_viewport(false); > Is it possible for this |host_| to host an interstitial page or other kind of > view? Yes, I think it can host an interstitial, e.g. if a navigation of the guest fails (ssh/network/404 errors), or the guest renderer crashes. > I'm wondering if it's better for the RWHV to query the RWHV for this flag. wdyt?
On 2016/01/26 19:47:28, wjmaclean wrote: > On 2016/01/26 19:42:35, sievers wrote: > > > https://codereview.chromium.org/1586923002/diff/210001/content/browser/frame_... > > File content/browser/frame_host/render_widget_host_view_guest.cc (right): > > > > > https://codereview.chromium.org/1586923002/diff/210001/content/browser/frame_... > > content/browser/frame_host/render_widget_host_view_guest.cc:66: > > host_->set_scale_input_to_viewport(false); > > Is it possible for this |host_| to host an interstitial page or other kind of > > view? > > Yes, I think it can host an interstitial, e.g. if a navigation of the guest > fails (ssh/network/404 errors), or the guest renderer crashes. > > > I'm wondering if it's better for the RWHV to query the RWHV for this flag. > wdyt? Thank you for review, but I didn't fully understand your comment. (as this should always be false in Guest View) Can you elaborate? Did you mean "for the RWHImpl to query the RWHV"?
On 2016/01/26 20:46:16, oshima wrote: > On 2016/01/26 19:47:28, wjmaclean wrote: > > On 2016/01/26 19:42:35, sievers wrote: > > > > > > https://codereview.chromium.org/1586923002/diff/210001/content/browser/frame_... > > > File content/browser/frame_host/render_widget_host_view_guest.cc (right): > > > > > > > > > https://codereview.chromium.org/1586923002/diff/210001/content/browser/frame_... > > > content/browser/frame_host/render_widget_host_view_guest.cc:66: > > > host_->set_scale_input_to_viewport(false); > > > Is it possible for this |host_| to host an interstitial page or other kind > of > > > view? > > > > Yes, I think it can host an interstitial, e.g. if a navigation of the guest > > fails (ssh/network/404 errors), or the guest renderer crashes. > > > > > I'm wondering if it's better for the RWHV to query the RWHV for this flag. > > wdyt? > > Thank you for review, but I didn't fully understand your comment. (as this > should always be false in Guest View) I'm just wondering if we end up using the right input scale for the interstitial if you set this flag and the existing |host_| then later holds a |view_| that is not a RWHVGuest. > Can you elaborate? Did you mean "for the RWHImpl to query the RWHV"? For example in RWHImpl::SetView() (not sure if that would necessarily be the right place), it could call |view_|->needs_scale_input() to configure the input router.
On 2016/01/26 21:03:32, sievers wrote: > On 2016/01/26 20:46:16, oshima wrote: > > On 2016/01/26 19:47:28, wjmaclean wrote: > > > On 2016/01/26 19:42:35, sievers wrote: > > > > > > > > > > https://codereview.chromium.org/1586923002/diff/210001/content/browser/frame_... > > > > File content/browser/frame_host/render_widget_host_view_guest.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1586923002/diff/210001/content/browser/frame_... > > > > content/browser/frame_host/render_widget_host_view_guest.cc:66: > > > > host_->set_scale_input_to_viewport(false); > > > > Is it possible for this |host_| to host an interstitial page or other kind > > of > > > > view? > > > > > > Yes, I think it can host an interstitial, e.g. if a navigation of the guest > > > fails (ssh/network/404 errors), or the guest renderer crashes. > > > > > > > I'm wondering if it's better for the RWHV to query the RWHV for this flag. > > > wdyt? > > > > Thank you for review, but I didn't fully understand your comment. (as this > > should always be false in Guest View) > > > I'm just wondering if we end up using the right input scale for the interstitial > if you set this flag and the existing |host_| then later holds a |view_| that is > not a RWHVGuest. According to the following an interstitial page in RWHVGuest creates RWHVGuest as well. https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... The input still should not be scaled in that case because input for interstitial page is sent back from renderer. I just tested with the test site with revoked ssl certificate, and confirmed that it's working. > > > Can you elaborate? Did you mean "for the RWHImpl to query the RWHV"? > > For example in RWHImpl::SetView() (not sure if that would necessarily be the > right place), it could call |view_|->needs_scale_input() to configure the input > router. I'm happy to change that way. wjmaclean@, what is your opinion? I can either move the variable and accessor to RenderWidgetHostViewBase, or add NeedsScaleInput() and return false if it's RWHVGuest. Please let me know which way you prefer.
On 2016/01/26 23:03:10, oshima wrote: > On 2016/01/26 21:03:32, sievers wrote: > > On 2016/01/26 20:46:16, oshima wrote: > > > On 2016/01/26 19:47:28, wjmaclean wrote: > > > > On 2016/01/26 19:42:35, sievers wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1586923002/diff/210001/content/browser/frame_... > > > > > File content/browser/frame_host/render_widget_host_view_guest.cc > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1586923002/diff/210001/content/browser/frame_... > > > > > content/browser/frame_host/render_widget_host_view_guest.cc:66: > > > > > host_->set_scale_input_to_viewport(false); > > > > > Is it possible for this |host_| to host an interstitial page or other > kind > > > of > > > > > view? > > > > > > > > Yes, I think it can host an interstitial, e.g. if a navigation of the > guest > > > > fails (ssh/network/404 errors), or the guest renderer crashes. > > > > > > > > > I'm wondering if it's better for the RWHV to query the RWHV for this > flag. > > > > wdyt? > > > > > > Thank you for review, but I didn't fully understand your comment. (as this > > > should always be false in Guest View) > > > > > > I'm just wondering if we end up using the right input scale for the > interstitial > > if you set this flag and the existing |host_| then later holds a |view_| that > is > > not a RWHVGuest. > > According to the following an interstitial page in RWHVGuest creates RWHVGuest > as well. > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > > > The input still should not be scaled in that case because input for interstitial > page is sent back from renderer. I just tested with the test site with revoked > ssl certificate, and confirmed that it's working. > > > > > > > Can you elaborate? Did you mean "for the RWHImpl to query the RWHV"? > > > > For example in RWHImpl::SetView() (not sure if that would necessarily be the > > right place), it could call |view_|->needs_scale_input() to configure the > input > > router. > > > I'm happy to change that way. wjmaclean@, what is your opinion? > > I can either move the variable and accessor to RenderWidgetHostViewBase, or add > NeedsScaleInput() and return false if it's RWHVGuest. > Please let me know which way you prefer. I don't really have a strong preference. I would say pick whichever you think is easiest to reason about when others read the code.
On 2016/01/27 01:19:46, wjmaclean wrote: > On 2016/01/26 23:03:10, oshima wrote: > > On 2016/01/26 21:03:32, sievers wrote: > > > On 2016/01/26 20:46:16, oshima wrote: > > > > On 2016/01/26 19:47:28, wjmaclean wrote: > > > > > On 2016/01/26 19:42:35, sievers wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1586923002/diff/210001/content/browser/frame_... > > > > > > File content/browser/frame_host/render_widget_host_view_guest.cc > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1586923002/diff/210001/content/browser/frame_... > > > > > > content/browser/frame_host/render_widget_host_view_guest.cc:66: > > > > > > host_->set_scale_input_to_viewport(false); > > > > > > Is it possible for this |host_| to host an interstitial page or other > > kind > > > > of > > > > > > view? > > > > > > > > > > Yes, I think it can host an interstitial, e.g. if a navigation of the > > guest > > > > > fails (ssh/network/404 errors), or the guest renderer crashes. > > > > > > > > > > > I'm wondering if it's better for the RWHV to query the RWHV for this > > flag. > > > > > wdyt? > > > > > > > > Thank you for review, but I didn't fully understand your comment. (as this > > > > should always be false in Guest View) > > > > > > > > > I'm just wondering if we end up using the right input scale for the > > interstitial > > > if you set this flag and the existing |host_| then later holds a |view_| > that > > is > > > not a RWHVGuest. > > > > According to the following an interstitial page in RWHVGuest creates RWHVGuest > > as well. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > > > > > > The input still should not be scaled in that case because input for > interstitial > > page is sent back from renderer. I just tested with the test site with revoked > > ssl certificate, and confirmed that it's working. > > > > > > > > > > > Can you elaborate? Did you mean "for the RWHImpl to query the RWHV"? > > > > > > For example in RWHImpl::SetView() (not sure if that would necessarily be the > > > right place), it could call |view_|->needs_scale_input() to configure the > > input > > > router. > > > > > > I'm happy to change that way. wjmaclean@, what is your opinion? > > > > I can either move the variable and accessor to RenderWidgetHostViewBase, or > add > > NeedsScaleInput() and return false if it's RWHVGuest. > > Please let me know which way you prefer. > > I don't really have a strong preference. I would say pick whichever you think is > easiest to reason about when others read the code. I also don't feel strongly if you feel confident that you can't end up with a RWHVGuest setting the bit to the wrong state for a RWHV(whatever) that is later attached to the RWHost. (Also given that this is a temporary measure.) Therefore lgtm if you want.
On 2016/01/27 01:35:57, sievers wrote: > On 2016/01/27 01:19:46, wjmaclean wrote: > > On 2016/01/26 23:03:10, oshima wrote: > > > On 2016/01/26 21:03:32, sievers wrote: > > > > On 2016/01/26 20:46:16, oshima wrote: > > > > > On 2016/01/26 19:47:28, wjmaclean wrote: > > > > > > On 2016/01/26 19:42:35, sievers wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1586923002/diff/210001/content/browser/frame_... > > > > > > > File content/browser/frame_host/render_widget_host_view_guest.cc > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1586923002/diff/210001/content/browser/frame_... > > > > > > > content/browser/frame_host/render_widget_host_view_guest.cc:66: > > > > > > > host_->set_scale_input_to_viewport(false); > > > > > > > Is it possible for this |host_| to host an interstitial page or > other > > > kind > > > > > of > > > > > > > view? > > > > > > > > > > > > Yes, I think it can host an interstitial, e.g. if a navigation of the > > > guest > > > > > > fails (ssh/network/404 errors), or the guest renderer crashes. > > > > > > > > > > > > > I'm wondering if it's better for the RWHV to query the RWHV for this > > > flag. > > > > > > wdyt? > > > > > > > > > > Thank you for review, but I didn't fully understand your comment. (as > this > > > > > should always be false in Guest View) > > > > > > > > > > > > I'm just wondering if we end up using the right input scale for the > > > interstitial > > > > if you set this flag and the existing |host_| then later holds a |view_| > > that > > > is > > > > not a RWHVGuest. > > > > > > According to the following an interstitial page in RWHVGuest creates > RWHVGuest > > > as well. > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > > > > > > > > > The input still should not be scaled in that case because input for > > interstitial > > > page is sent back from renderer. I just tested with the test site with > revoked > > > ssl certificate, and confirmed that it's working. > > > > > > > > > > > > > > > Can you elaborate? Did you mean "for the RWHImpl to query the RWHV"? > > > > > > > > For example in RWHImpl::SetView() (not sure if that would necessarily be > the > > > > right place), it could call |view_|->needs_scale_input() to configure the > > > input > > > > router. > > > > > > > > > I'm happy to change that way. wjmaclean@, what is your opinion? > > > > > > I can either move the variable and accessor to RenderWidgetHostViewBase, or > > add > > > NeedsScaleInput() and return false if it's RWHVGuest. > > > Please let me know which way you prefer. > > > > I don't really have a strong preference. I would say pick whichever you think > is > > easiest to reason about when others read the code. > > I also don't feel strongly if you feel confident that you can't end up with a > RWHVGuest setting the bit to the wrong state for a RWHV(whatever) that is later > attached to the RWHost. (Also given that this is a temporary measure.) > > Therefore lgtm if you want. If that's the case, let me land this as I did a lot of testing myself with this CL. Thanks a lot!
On 2016/01/27 01:35:57, sievers wrote: > On 2016/01/27 01:19:46, wjmaclean wrote: > > On 2016/01/26 23:03:10, oshima wrote: > > > On 2016/01/26 21:03:32, sievers wrote: > > > > On 2016/01/26 20:46:16, oshima wrote: > > > > > On 2016/01/26 19:47:28, wjmaclean wrote: > > > > > > On 2016/01/26 19:42:35, sievers wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1586923002/diff/210001/content/browser/frame_... > > > > > > > File content/browser/frame_host/render_widget_host_view_guest.cc > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1586923002/diff/210001/content/browser/frame_... > > > > > > > content/browser/frame_host/render_widget_host_view_guest.cc:66: > > > > > > > host_->set_scale_input_to_viewport(false); > > > > > > > Is it possible for this |host_| to host an interstitial page or > other > > > kind > > > > > of > > > > > > > view? > > > > > > > > > > > > Yes, I think it can host an interstitial, e.g. if a navigation of the > > > guest > > > > > > fails (ssh/network/404 errors), or the guest renderer crashes. > > > > > > > > > > > > > I'm wondering if it's better for the RWHV to query the RWHV for this > > > flag. > > > > > > wdyt? > > > > > > > > > > Thank you for review, but I didn't fully understand your comment. (as > this > > > > > should always be false in Guest View) > > > > > > > > > > > > I'm just wondering if we end up using the right input scale for the > > > interstitial > > > > if you set this flag and the existing |host_| then later holds a |view_| > > that > > > is > > > > not a RWHVGuest. > > > > > > According to the following an interstitial page in RWHVGuest creates > RWHVGuest > > > as well. > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > > > > > > > > > The input still should not be scaled in that case because input for > > interstitial > > > page is sent back from renderer. I just tested with the test site with > revoked > > > ssl certificate, and confirmed that it's working. > > > > > > > > > > > > > > > Can you elaborate? Did you mean "for the RWHImpl to query the RWHV"? > > > > > > > > For example in RWHImpl::SetView() (not sure if that would necessarily be > the > > > > right place), it could call |view_|->needs_scale_input() to configure the > > > input > > > > router. > > > > > > > > > I'm happy to change that way. wjmaclean@, what is your opinion? > > > > > > I can either move the variable and accessor to RenderWidgetHostViewBase, or > > add > > > NeedsScaleInput() and return false if it's RWHVGuest. > > > Please let me know which way you prefer. > > > > I don't really have a strong preference. I would say pick whichever you think > is > > easiest to reason about when others read the code. > > I also don't feel strongly if you feel confident that you can't end up with a > RWHVGuest setting the bit to the wrong state for a RWHV(whatever) that is later > attached to the RWHost. (Also given that this is a temporary measure.) > > Therefore lgtm if you want. If that's the case, let me land this as I did a lot of testing myself with this CL. Thanks a lot!
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1586923002/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1586923002/210001
Message was sent while issue was closed.
Description was changed from ========== [UseZoomForDSF] Guest view support * Do not convert inputs for child frames because they're already converted * Use 1x dsf for the child frame's surface. This is a stopgap solution. I'll address it in a separate CL. * Convert the guest view size to css in browser_plugin as all use of the size expects css coordinates. BUG=485650 TEST=WebViewWithZoomForDSFTest.* ========== to ========== [UseZoomForDSF] Guest view support * Do not convert inputs for child frames because they're already converted * Use 1x dsf for the child frame's surface. This is a stopgap solution. I'll address it in a separate CL. * Convert the guest view size to css in browser_plugin as all use of the size expects css coordinates. BUG=485650 TEST=WebViewWithZoomForDSFTest.* ==========
Message was sent while issue was closed.
Committed patchset #4 (id:210001)
Message was sent while issue was closed.
Description was changed from ========== [UseZoomForDSF] Guest view support * Do not convert inputs for child frames because they're already converted * Use 1x dsf for the child frame's surface. This is a stopgap solution. I'll address it in a separate CL. * Convert the guest view size to css in browser_plugin as all use of the size expects css coordinates. BUG=485650 TEST=WebViewWithZoomForDSFTest.* ========== to ========== [UseZoomForDSF] Guest view support * Do not convert inputs for child frames because they're already converted * Use 1x dsf for the child frame's surface. This is a stopgap solution. I'll address it in a separate CL. * Convert the guest view size to css in browser_plugin as all use of the size expects css coordinates. BUG=485650 TEST=WebViewWithZoomForDSFTest.* Committed: https://crrev.com/9000e422526a1795be84f2571b5e65f431c73f2f Cr-Commit-Position: refs/heads/master@{#371695} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9000e422526a1795be84f2571b5e65f431c73f2f Cr-Commit-Position: refs/heads/master@{#371695} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
