Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23364004:
Implementation of device metrics emulation in render view. (Closed)
|
Created:
7 years, 4 months ago by dgozman Modified:
7 years, 2 months ago CC:
chromium-reviews, jbauman+watch_chromium.org, yusukes+watch_chromium.org, jam, penghuang+watch_chromium.org, apatrick_chromium, sievers+watch_chromium.org, joi+watch-content_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, danakj+watch_chromium.org, James Su, miu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionImplementation of device metrics emulation in render view.
This is used by DevTools to emulate various devices on desktop browser.
Emulation includes changing view size, view/window screen positions and
device scale factor. Also, the whole view is scaled down if required to
fit the browser window.
To achieve this, render widget creates an emulator, which handles metrics
override by intercepting size/position-related messages.
Scaling down is done by setting transform on the root composited layer
on the blink side.
This emulation is transparent to browser side.
Depends on Blink-side patch: https://codereview.chromium.org/23187005/
BUG=288959
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226663
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 9
Patch Set 6 : #Patch Set 7 : Style #
Total comments: 16
Patch Set 8 : Fixed review comments #Patch Set 9 : Moved helper class to cc #Patch Set 10 : Moved helper class to cc #
Total comments: 36
Patch Set 11 : Fixed reviewers comments #Patch Set 12 : Fixed reviewers comments #
Total comments: 8
Patch Set 13 : Producing image of view size #
Total comments: 10
Patch Set 14 : Fixed review comments #Patch Set 15 : #Patch Set 16 : Rebase #Patch Set 17 : Rebase #
Messages
Total messages: 39 (0 generated)
Hello, Could you please look at this patch? This is to properly emulate device on desktop browser. Here I create a special helper, which alters the render widget behavior when device emulation is enabled. Render widget host thinks that nothing happened while blink thinks that we are on device. Note: there is no fit-to-view support yet. Thanks, Dmitry
Please put in a BUG= line. Only trivial changes can go without one, and this doesn't look trivial. A bug can have rationale, design docs, etc.
Adding John for thoughts. This is a bit out of my area of expertise, but it's not clear to me what's going on here. Can we expose this more cleanly rather than "emulation" which packs a whole bunch of functionality that changes how things work in? Can we do this at any other layer?
On 2013/09/04 14:55:45, Avi wrote: > Adding John for thoughts. > > This is a bit out of my area of expertise, but it's not clear to me what's going > on here. Can we expose this more cleanly rather than "emulation" which packs a > whole bunch of functionality that changes how things work in? This "emulation" doesn't appear to change that many things, it just resizes and translates the view and changes a few WebSettings. So it might clear up confusion just to choose a less generic name like "ScopedResizeHelper". > > Can we do this at any other layer? The input events need to be done at the RenderWidgetHostImpl level to avoid bugs, and it looks the resize could be moved there as well.
https://codereview.chromium.org/23364004/diff/46001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/23364004/diff/46001/content/renderer/render_v... content/renderer/render_view_impl.cc:2459: webview()->settings()->setForceCompositingMode(false); This one is usually true on desktop as well, this would clobber it. https://codereview.chromium.org/23364004/diff/46001/content/renderer/render_w... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/23364004/diff/46001/content/renderer/render_w... content/renderer/render_widget.cc:2702: WebKit::WebInputEvent* RenderWidget::DeviceEmulationHelper::ConvertInputEventToEmulated( The CC impl thread captures input events as well. For example, all touch-based gesture events, and mousewheel when threaded compositing is enabled. So translating them here won't work in those cases. I'd suggest translating on the browser side instead.
I'm not really sure this should be moved to the host side. See below. https://codereview.chromium.org/23364004/diff/46001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/23364004/diff/46001/content/renderer/render_v... content/renderer/render_view_impl.cc:2459: webview()->settings()->setForceCompositingMode(false); On 2013/09/05 08:26:49, aelias wrote: > This one is usually true on desktop as well, this would clobber it. Nice catch, thanks! https://codereview.chromium.org/23364004/diff/46001/content/renderer/render_w... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/23364004/diff/46001/content/renderer/render_w... content/renderer/render_widget.cc:2702: WebKit::WebInputEvent* RenderWidget::DeviceEmulationHelper::ConvertInputEventToEmulated( On 2013/09/05 08:26:49, aelias wrote: > The CC impl thread captures input events as well. For example, all touch-based > gesture events, and mousewheel when threaded compositing is enabled. So > translating them here won't work in those cases. I'd suggest translating on the > browser side instead. As I understand, CC event handling is not interested in screen positions, which are the only ones changed here. Also, if/when I add scaling support via CC impl, it would be logical to scale events there (since layerTreeHostImpl would know the scale factor). On the other hand, moving more code to the host size will spread my changes a lot, while I prefer to keep them more or less in one place (render widget in this CL). What do you think?
https://codereview.chromium.org/23364004/diff/46001/content/renderer/render_w... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/23364004/diff/46001/content/renderer/render_w... content/renderer/render_widget.cc:2702: WebKit::WebInputEvent* RenderWidget::DeviceEmulationHelper::ConvertInputEventToEmulated( On 2013/09/10 19:54:02, dgozman wrote: > On 2013/09/05 08:26:49, aelias wrote: > > The CC impl thread captures input events as well. For example, all > touch-based > > gesture events, and mousewheel when threaded compositing is enabled. So > > translating them here won't work in those cases. I'd suggest translating on > the > > browser side instead. > > As I understand, CC event handling is not interested in screen positions, which > are the only ones changed here. > Also, if/when I add scaling support via CC impl, it would be logical to scale > events there (since layerTreeHostImpl would know the scale factor). > > On the other hand, moving more code to the host size will spread my changes a > lot, while I prefer to keep them more or less in one place (render widget in > this CL). What do you think? OK, I didn't notice you were only changing those side fields. How about just not making any changes to input event at all then? This is a lot of complex logic for no clear benefit. You could just forward the true screen position of input events (modulo DIP adjustment). There's no clear use case for screenPosition anyway and the intent of it is that it should be true screen position.
jam, may you have a look please? https://codereview.chromium.org/23364004/diff/46001/content/renderer/render_w... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/23364004/diff/46001/content/renderer/render_w... content/renderer/render_widget.cc:2702: WebKit::WebInputEvent* RenderWidget::DeviceEmulationHelper::ConvertInputEventToEmulated( On 2013/09/10 20:33:34, aelias wrote: > On 2013/09/10 19:54:02, dgozman wrote: > > On 2013/09/05 08:26:49, aelias wrote: > > > The CC impl thread captures input events as well. For example, all > > touch-based > > > gesture events, and mousewheel when threaded compositing is enabled. So > > > translating them here won't work in those cases. I'd suggest translating on > > the > > > browser side instead. > > > > As I understand, CC event handling is not interested in screen positions, > which > > are the only ones changed here. > > Also, if/when I add scaling support via CC impl, it would be logical to scale > > events there (since layerTreeHostImpl would know the scale factor). > > > > On the other hand, moving more code to the host size will spread my changes a > > lot, while I prefer to keep them more or less in one place (render widget in > > this CL). What do you think? > > OK, I didn't notice you were only changing those side fields. How about just not > making any changes to input event at all then? This is a lot of complex logic > for no clear benefit. You could just forward the true screen position of input > events (modulo DIP adjustment). There's no clear use case for screenPosition > anyway and the intent of it is that it should be true screen position. Well, wrong screen position of events will conflict with window.screenX/Y, but this is not a big deal. Let's remove for now. However, when implementing render-side scale, this input method conversion should be restored back to apply backward scale.
https://codereview.chromium.org/23364004/diff/46001/content/renderer/render_w... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/23364004/diff/46001/content/renderer/render_w... content/renderer/render_widget.cc:2702: WebKit::WebInputEvent* RenderWidget::DeviceEmulationHelper::ConvertInputEventToEmulated( > However, when implementing render-side scale, this input method conversion should be restored back to apply backward scale. OK, so it sounds like you don't have this really working locally yet. So I think we should delay reviewing this change until you have an end-to-end solution or we might go the wrong design direction.
I'm not familiar with devtools very much, or this feature, so I'll defer my lgtm to pfeldman. I looked through this change and nothing jumped at me. https://codereview.chromium.org/23364004/diff/46001/content/renderer/render_w... File content/renderer/render_widget.h (right): https://codereview.chromium.org/23364004/diff/46001/content/renderer/render_w... content/renderer/render_widget.h:750: class DeviceEmulationHelper { nit: since this is a private class, just forward declare it here and declare/define it in the cc file in an anonymous namespace
pfeldman: may you have a look please? aelias: scaling is now done via root layer transform, so this is ready to review. Blink side patch (not ready to commit yet though): https://codereview.chromium.org/23187005/. The remaining issues are all related to pageScale on desktop: - wrong popup size/position with viewport enabled; - scrollbars don't respond to mouse events. I'm working with wjmaclean and bokan on that. For now, viewport is not turned on. https://codereview.chromium.org/23364004/diff/46001/content/renderer/render_w... File content/renderer/render_widget.h (right): https://codereview.chromium.org/23364004/diff/46001/content/renderer/render_w... content/renderer/render_widget.h:750: class DeviceEmulationHelper { On 2013/09/11 02:59:47, jam wrote: > nit: since this is a private class, just forward declare it here and > declare/define it in the cc file in an anonymous namespace Unfortunately, this class is also used by RenderViewImpl.
Could you explain what a "popup" is in this context and what UX you're trying to achieve?
On 2013/09/13 18:29:46, aelias wrote: > Could you explain what a "popup" is in this context and what UX you're trying to > achieve? Popups are created for html select element in WebViewClient::CreatePopupMenu. They are pure render widgets without compositor, except for Android and MacOS, which have system-supplied popups. Here popups are relocated to be in right place (near select element) when the view is scaled. You can see current UX by enabling device metrics override in DevTools. New way has pretty similar experience, but with proper emulation (i.e. there should be no obvious difference from the html page side between real device and emulation). The addition is device scale factor, which was not emulated before. Also, there will be some runtime settings changed on blink side to match the device experience (see https://codereview.chromium.org/23187005/).
OK. Positioning browser objects like popups relative to content is a loose end in Chrome's current architecture. That class of objects has caused constant pain on Android as well. My suggestion in the short term would be to use the page scale and scroll offset specified in the CompositorFrameMetadata to position them. In the longer run we probably want to integrate them more deeply in the compositing architecture (particularly after Aura launches everywhere).
https://codereview.chromium.org/23364004/diff/78001/cc/layers/heads_up_displa... File cc/layers/heads_up_display_layer.cc (right): https://codereview.chromium.org/23364004/diff/78001/cc/layers/heads_up_displa... cc/layers/heads_up_display_layer.cc:31: // The root layer may have the transform set. As we still want to be shown This should be addressed on the Blink side instead. You can set a inverse transform on the HUD layer there or change where the transform is applied so that the HUD layer isn't under it. https://codereview.chromium.org/23364004/diff/78001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/23364004/diff/78001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_aura.cc:811: !device_emulation_enabled_ && Is it really that you want to accept an image of *any* size? It seems to me you still expect a particular size, just a different one than usual? Could you set up the code that way instead? Secondly, is a different size buffer needed in the first place now that you're applying scale on the renderer side? Generally speaking, rather than adding a new boolean mode that turns off some logic, it would be better for the existing code to be refactored and generalized to cover both your use case and the normal one. https://codereview.chromium.org/23364004/diff/78001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/23364004/diff/78001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_aura.h:642: bool device_emulation_enabled_; This name sounds too generic considering it only affects resize. https://codereview.chromium.org/23364004/diff/78001/content/renderer/gpu/inpu... File content/renderer/gpu/input_handler_manager.h (right): https://codereview.chromium.org/23364004/diff/78001/content/renderer/gpu/inpu... content/renderer/gpu/input_handler_manager.h:33: // InputEventRewriter class allows client to rewrite incoming input events I don't see why you need this on the impl-thread side. CC already should apply the transform tree to hit test input events. (You'll probably need something on the main thread side though.) If it's to correct some browser-side translate or scale that the renderer CC is not aware of, then I think the right thing to do would be to transform the event on the browser side instead of here (or perhaps, move that transformation to be on the renderer CC root layer as well). https://codereview.chromium.org/23364004/diff/78001/content/renderer/gpu/rend... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/23364004/diff/78001/content/renderer/gpu/rend... content/renderer/gpu/render_widget_compositor.h:57: void OverrideDeviceScaleFactor(float device_scale); Obsolete?
Thanks for comments. The ones not addressed will be fixed. https://codereview.chromium.org/23364004/diff/78001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/23364004/diff/78001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_aura.cc:811: !device_emulation_enabled_ && On 2013/09/13 23:51:44, aelias wrote: > Is it really that you want to accept an image of *any* size? It seems to me you > still expect a particular size, just a different one than usual? Could you set > up the code that way instead? I think, this would be a right choice. > > Secondly, is a different size buffer needed in the first place now that you're > applying scale on the renderer side? To supply image of required size we probably need a new root layer filled with background color under the contents. I'm afraid that will break many things assuming that root layer is what passed from blink. > > Generally speaking, rather than adding a new boolean mode that turns off some > logic, it would be better for the existing code to be refactored and generalized > to cover both your use case and the normal one. https://codereview.chromium.org/23364004/diff/78001/content/renderer/gpu/inpu... File content/renderer/gpu/input_handler_manager.h (right): https://codereview.chromium.org/23364004/diff/78001/content/renderer/gpu/inpu... content/renderer/gpu/input_handler_manager.h:33: // InputEventRewriter class allows client to rewrite incoming input events On 2013/09/13 23:51:44, aelias wrote: > I don't see why you need this on the impl-thread side. CC already should apply > the transform tree to hit test input events. (You'll probably need something on > the main thread side though.) Doesn't main thread get events through regular render widget input event handler? If so, that's already handled. > > If it's to correct some browser-side translate or scale that the renderer CC is > not aware of, then I think the right thing to do would be to transform the event > on the browser side instead of here (or perhaps, move that transformation to be > on the renderer CC root layer as well). There is no browser-side transformation. I was not aware that CC takes root layer transform into account. This thing is not needed then. https://codereview.chromium.org/23364004/diff/78001/content/renderer/gpu/rend... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/23364004/diff/78001/content/renderer/gpu/rend... content/renderer/gpu/render_widget_compositor.h:57: void OverrideDeviceScaleFactor(float device_scale); On 2013/09/13 23:51:44, aelias wrote: > Obsolete? Nope. This is to ensure that compositor has the real deviceScaleFactor instead of emulated one passed from blink.
https://codereview.chromium.org/23364004/diff/78001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/23364004/diff/78001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_aura.cc:811: !device_emulation_enabled_ && On 2013/09/14 07:42:42, dgozman wrote: > To supply image of required size we probably need a new root layer filled with > background color under the contents. I'm afraid that will break many things > assuming that root layer is what passed from blink. Not necessarily. First, note that CC is rather flexible about taking any kind of layer structure because it's also used for Aura windowing. But I think you should actually be able to get this working without changing the layer structure by taking advantage of AppendQuadsToFillScreen() in layer_tree_host_impl.cc, which is designed to deal with such cases where the layers don't fill the entire screen. I recommend giving that a try. The RWHV resize logic is rather subtle and bug-prone, and it would be nice to handle the sizing issues entirely within renderer CC.
https://codereview.chromium.org/23364004/diff/78001/content/renderer/gpu/rend... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/23364004/diff/78001/content/renderer/gpu/rend... content/renderer/gpu/render_widget_compositor.h:57: void OverrideDeviceScaleFactor(float device_scale); On 2013/09/14 07:42:42, dgozman wrote: > On 2013/09/13 23:51:44, aelias wrote: > > Obsolete? > > Nope. This is to ensure that compositor has the real deviceScaleFactor instead > of emulated one passed from blink. Hmm, in that case I would prefer that your override be made within Blink at the moment of passing the device_scale_factor to CC, instead of overriding it on the side here.
https://codereview.chromium.org/23364004/diff/78001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/23364004/diff/78001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_aura.cc:811: !device_emulation_enabled_ && On 2013/09/14 07:50:53, aelias wrote: > On 2013/09/14 07:42:42, dgozman wrote: > > To supply image of required size we probably need a new root layer filled with > > background color under the contents. I'm afraid that will break many things > > assuming that root layer is what passed from blink. > > Not necessarily. First, note that CC is rather flexible about taking any kind > of layer structure because it's also used for Aura windowing. But I think you > should actually be able to get this working without changing the layer structure > by taking advantage of AppendQuadsToFillScreen() in layer_tree_host_impl.cc, > which is designed to deal with such cases where the layers don't fill the entire > screen. > > I recommend giving that a try. The RWHV resize logic is rather subtle and > bug-prone, and it would be nice to handle the sizing issues entirely within > renderer CC. On second thought, the real difficulty if you go that route would be to separate the scroll and draw viewports, i.e. you would be forced to create a new OutputSurface type and use SetExternalDrawConstraints from it. It's not obvious that's a better architecture than the one you're currently going with, it may well be worse. So I withdraw that suggestion for now, please go ahead and clean up the resize-based stuff like we were saying.
PTAL https://codereview.chromium.org/23364004/diff/78001/cc/layers/heads_up_displa... File cc/layers/heads_up_display_layer.cc (right): https://codereview.chromium.org/23364004/diff/78001/cc/layers/heads_up_displa... cc/layers/heads_up_display_layer.cc:31: // The root layer may have the transform set. As we still want to be shown On 2013/09/13 23:51:44, aelias wrote: > This should be addressed on the Blink side instead. You can set a inverse > transform on the HUD layer there or change where the transform is applied so > that the HUD layer isn't under it. AFAIK, blink doesn't know about the HUD layer. Ideally, we should place HUD layer as a sibling to blink root layer. Not sure if that's the right choice though. WDYT? https://codereview.chromium.org/23364004/diff/78001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/23364004/diff/78001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_aura.cc:811: !device_emulation_enabled_ && On 2013/09/14 08:06:51, aelias wrote: > On 2013/09/14 07:50:53, aelias wrote: > > On 2013/09/14 07:42:42, dgozman wrote: > > > To supply image of required size we probably need a new root layer filled > with > > > background color under the contents. I'm afraid that will break many things > > > assuming that root layer is what passed from blink. > > > > Not necessarily. First, note that CC is rather flexible about taking any kind > > of layer structure because it's also used for Aura windowing. But I think you > > should actually be able to get this working without changing the layer > structure > > by taking advantage of AppendQuadsToFillScreen() in layer_tree_host_impl.cc, > > which is designed to deal with such cases where the layers don't fill the > entire > > screen. > > > > I recommend giving that a try. The RWHV resize logic is rather subtle and > > bug-prone, and it would be nice to handle the sizing issues entirely within > > renderer CC. > > On second thought, the real difficulty if you go that route would be to separate > the scroll and draw viewports, i.e. you would be forced to create a new > OutputSurface type and use SetExternalDrawConstraints from it. It's not obvious > that's a better architecture than the one you're currently going with, it may > well be worse. So I withdraw that suggestion for now, please go ahead and clean > up the resize-based stuff like we were saying. Done. https://codereview.chromium.org/23364004/diff/78001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/23364004/diff/78001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_aura.h:642: bool device_emulation_enabled_; On 2013/09/13 23:51:44, aelias wrote: > This name sounds too generic considering it only affects resize. Created |expected_rendered_size_| instead. https://codereview.chromium.org/23364004/diff/78001/content/renderer/gpu/inpu... File content/renderer/gpu/input_handler_manager.h (right): https://codereview.chromium.org/23364004/diff/78001/content/renderer/gpu/inpu... content/renderer/gpu/input_handler_manager.h:33: // InputEventRewriter class allows client to rewrite incoming input events On 2013/09/13 23:51:44, aelias wrote: > I don't see why you need this on the impl-thread side. CC already should apply > the transform tree to hit test input events. (You'll probably need something on > the main thread side though.) > > If it's to correct some browser-side translate or scale that the renderer CC is > not aware of, then I think the right thing to do would be to transform the event > on the browser side instead of here (or perhaps, move that transformation to be > on the renderer CC root layer as well). This is removed. https://codereview.chromium.org/23364004/diff/78001/content/renderer/gpu/rend... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/23364004/diff/78001/content/renderer/gpu/rend... content/renderer/gpu/render_widget_compositor.h:57: void OverrideDeviceScaleFactor(float device_scale); On 2013/09/14 07:53:10, aelias wrote: > On 2013/09/14 07:42:42, dgozman wrote: > > On 2013/09/13 23:51:44, aelias wrote: > > > Obsolete? > > > > Nope. This is to ensure that compositor has the real deviceScaleFactor instead > > of emulated one passed from blink. > > Hmm, in that case I would prefer that your override be made within Blink at the > moment of passing the device_scale_factor to CC, instead of overriding it on the > side here. Done.
Chris, may you have a look at view_messages.h please?
https://codereview.chromium.org/23364004/diff/111001/cc/layers/heads_up_displ... File cc/layers/heads_up_display_layer.cc (right): https://codereview.chromium.org/23364004/diff/111001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer.cc:30: if (parent()) { I don't know how you'd get a HUD layer without a parent(). Is this something you are seeing? https://codereview.chromium.org/23364004/diff/111001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer.cc:36: device_viewport_in_layout_pixels = gfx::ToFlooredSize(rect.size()); You're projecting from the root surface to the root layer, but then dropping the offset and only using the size. Are you saying the root layer has a scale transform set on it?
devtools lgtm https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:195: const gfx::Rect& widget_rect, Poor indent. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:252: bool fit_to_view) scale_ is not initialized. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:378: WebKeyboardEvent* result = new WebKeyboardEvent(keyboard_event); return new ... https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:413: for (size_t index = 0; index < result->touchesLength; ++index) { One line blocks don't need {} https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:448: base::WeakPtr<RenderWidget::DeviceEmulator> RenderWidget::DeviceEmulator::AsWeakPtr() { 80 chars limit please. base::WeakPtr<RenderWidget::DeviceEmulator> RenderWidget::DeviceEmulator::AsWeakPtr() { ... } https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:452: WebKit::WebRect RenderWidget::DeviceEmulator::ConvertPopupScreenRectFromEmulated( ditto https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:658: device_emulator_->BeginEmulation(this); I prefer StartEmulation / UpdateEmulation / StopEmulation. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:1141: owner.reset(input_event); You can reset unconditionally.
https://codereview.chromium.org/23364004/diff/111001/cc/layers/heads_up_displ... File cc/layers/heads_up_display_layer.cc (right): https://codereview.chromium.org/23364004/diff/111001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer.cc:30: if (parent()) { On 2013/09/19 17:07:45, danakj wrote: > I don't know how you'd get a HUD layer without a parent(). Is this something you > are seeing? I saw some code like this around hud layer. I'm probably wrong. Will check once again and remove |if|. https://codereview.chromium.org/23364004/diff/111001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer.cc:36: device_viewport_in_layout_pixels = gfx::ToFlooredSize(rect.size()); On 2013/09/19 17:07:45, danakj wrote: > You're projecting from the root surface to the root layer, but then dropping the > offset and only using the size. Are you saying the root layer has a scale > transform set on it? Yep, root layer has a scale transform. I'm not sure whether it's possible to compensate any transform though. Ideally, hud layer may be a sibling of blink supplied root layer. Is that a right way?
https://codereview.chromium.org/23364004/diff/111001/cc/layers/heads_up_displ... File cc/layers/heads_up_display_layer.cc (right): https://codereview.chromium.org/23364004/diff/111001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer.cc:36: device_viewport_in_layout_pixels = gfx::ToFlooredSize(rect.size()); On 2013/09/19 18:42:18, dgozman wrote: > Yep, root layer has a scale transform. I'm not sure whether it's possible to > compensate any transform though. Ideally, hud layer may be a sibling of blink > supplied root layer. Is that a right way? The Blink-supplied root layer is the one and only root layer so I don't think we can have a sibling to it. Could you try positioning it relative to the RootContainerLayer (root scroll layer's parent) bounds() instead of starting from the device_viewport_size? This logic seems to be just computing that by a different means anyway. Maybe you could move the HUD layer under the root container layer so you could just check the parent bounds. (Alternatively, set Blink root layer bounds to viewport size like in my patch https://codereview.chromium.org/21084007/ (didn't end up needing to land that at the time, but we could resurrect that), and keep the parenting of the HUD layer the same.)
I took a closer look at the whole patch. (I don't care that much about the HUD stuff incidentally, you can fix it in a later patch if you prefer.) This is getting closer, but I still have a some issues with it. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_view_impl.cc:2365: widget->set_popup_device_emulator(device_emulator_.get()); This weak pointer is sketchy. The popup didn't need a connection to the original RenderWidget for anything before but it needs it for this? I don't know popups very well, but there must be a cleaner way to do this. Perhaps do something from the Blink side in WebPagePopupImpl? https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:192: class RenderWidget::DeviceEmulator { I suggest renaming this to ScreenMetricsEmulator. And basically everywhere else that says "device" in this patch I'd like to have renamed to "screen metrics", since that wording is more specific. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:202: void BeginEmulation(RenderWidget* widget); If it's always the same RenderWidget, can we just pass it in the constructor and keep it in a member variable? https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:351: widget->didInvalidateRect(gfx::Rect(widget->size_)); I thought accelerated compositing was mandatory for device emulation support? Is this line needed? https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:370: WebInputEvent* RenderWidget::DeviceEmulator::ConvertInputEventToEmulated( It's the job of Source/web/WebInputEventConversion.cpp in Blink to apply translation and scale due to scroll offset and pageScaleFactor. It seems to me you're doing a very similar thing here (and probably missing a few subtleties that are applied correctly there) and you should find a way to get your extra scale factor applied there instead. For scale, perhaps simply adjusting the return value of FrameView::visibleContentScaleFactor() would do what you need? And again I'm not sure what you're trying to achieve exactly by changing the screen positions by widgetRect so I suggest deleting that part and forgetting about it. (It's an "emulated" environment anyway, we're under no obligation to give it accurate screen position?) https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:660: device_emulator_->ChangeEmulationParams(this, helper); You're leaking the "helper" object" in this codepath. I suggest not doing it this way and just pass the four arguments to ChangeEmulationParams. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:664: device_emulator_->EndEmulation(this); Doesn't look like keeping BeginEmulation/EndEmulation separate is useful, please merge them into the constructor and destructor.
I am no longer an OWNER for this _messages.h file. Adding kenrb instead.
See Blink-side implementation here: https://codereview.chromium.org/23187005/ PTAL https://codereview.chromium.org/23364004/diff/111001/cc/layers/heads_up_displ... File cc/layers/heads_up_display_layer.cc (right): https://codereview.chromium.org/23364004/diff/111001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer.cc:36: device_viewport_in_layout_pixels = gfx::ToFlooredSize(rect.size()); On 2013/09/19 21:35:28, aelias wrote: > On 2013/09/19 18:42:18, dgozman wrote: > > Yep, root layer has a scale transform. I'm not sure whether it's possible to > > compensate any transform though. Ideally, hud layer may be a sibling of blink > > supplied root layer. Is that a right way? > > The Blink-supplied root layer is the one and only root layer so I don't think we > can have a sibling to it. > > Could you try positioning it relative to the RootContainerLayer (root scroll > layer's parent) bounds() instead of starting from the device_viewport_size? > This logic seems to be just computing that by a different means anyway. Maybe > you could move the HUD layer under the root container layer so you could just > check the parent bounds. > > (Alternatively, set Blink root layer bounds to viewport size like in my patch > https://codereview.chromium.org/21084007/ (didn't end up needing to land that at > the time, but we could resurrect that), and keep the parenting of the HUD layer > the same.) Filed a separate bug for this. I will look into moving HUD layer under root container layer. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_view_impl.cc:2365: widget->set_popup_device_emulator(device_emulator_.get()); On 2013/09/20 07:10:14, aelias wrote: > This weak pointer is sketchy. The popup didn't need a connection to the > original RenderWidget for anything before but it needs it for this? I don't > know popups very well, but there must be a cleaner way to do this. Perhaps do > something from the Blink side in WebPagePopupImpl? I don't really want to move this logic to Blink, especially as I don't see any clear single place for it. I've changed this to store the numbers instead of pointer to emulator. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:192: class RenderWidget::DeviceEmulator { On 2013/09/20 07:10:14, aelias wrote: > I suggest renaming this to ScreenMetricsEmulator. And basically everywhere else > that says "device" in this patch I'd like to have renamed to "screen metrics", > since that wording is more specific. Done. I've left "device" in Blink API though, as "device emulation" is already heavily used there. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:195: const gfx::Rect& widget_rect, On 2013/09/19 17:27:16, pfeldman wrote: > Poor indent. Done. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:202: void BeginEmulation(RenderWidget* widget); On 2013/09/20 07:10:14, aelias wrote: > If it's always the same RenderWidget, can we just pass it in the constructor and > keep it in a member variable? Done. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:252: bool fit_to_view) On 2013/09/19 17:27:16, pfeldman wrote: > scale_ is not initialized. Done. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:351: widget->didInvalidateRect(gfx::Rect(widget->size_)); On 2013/09/20 07:10:14, aelias wrote: > I thought accelerated compositing was mandatory for device emulation support? > Is this line needed? You are right. Removed. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:370: WebInputEvent* RenderWidget::DeviceEmulator::ConvertInputEventToEmulated( On 2013/09/20 07:10:14, aelias wrote: > It's the job of Source/web/WebInputEventConversion.cpp in Blink to apply > translation and scale due to scroll offset and pageScaleFactor. It seems to me > you're doing a very similar thing here (and probably missing a few subtleties > that are applied correctly there) and you should find a way to get your extra > scale factor applied there instead. > > For scale, perhaps simply adjusting the return value of > FrameView::visibleContentScaleFactor() would do what you need? And again I'm > not sure what you're trying to achieve exactly by changing the screen positions > by widgetRect so I suggest deleting that part and forgetting about it. (It's an > "emulated" environment anyway, we're under no obligation to give it accurate > screen position?) Done. Please see https://codereview.chromium.org/23187005/ for implementation details. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:378: WebKeyboardEvent* result = new WebKeyboardEvent(keyboard_event); On 2013/09/19 17:27:16, pfeldman wrote: > return new ... Done. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:413: for (size_t index = 0; index < result->touchesLength; ++index) { On 2013/09/19 17:27:16, pfeldman wrote: > One line blocks don't need {} Done. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:448: base::WeakPtr<RenderWidget::DeviceEmulator> RenderWidget::DeviceEmulator::AsWeakPtr() { On 2013/09/19 17:27:16, pfeldman wrote: > 80 chars limit please. > > base::WeakPtr<RenderWidget::DeviceEmulator> > RenderWidget::DeviceEmulator::AsWeakPtr() { > ... > } Done. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:452: WebKit::WebRect RenderWidget::DeviceEmulator::ConvertPopupScreenRectFromEmulated( On 2013/09/19 17:27:16, pfeldman wrote: > ditto Done. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:658: device_emulator_->BeginEmulation(this); On 2013/09/19 17:27:16, pfeldman wrote: > I prefer StartEmulation / UpdateEmulation / StopEmulation. BeginEmulation/EndEmulation are removed. I'd keep ChangeEmulationParams as it's the only remaining method. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:660: device_emulator_->ChangeEmulationParams(this, helper); On 2013/09/20 07:10:14, aelias wrote: > You're leaking the "helper" object" in this codepath. I suggest not doing it > this way and just pass the four arguments to ChangeEmulationParams. Done. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:664: device_emulator_->EndEmulation(this); On 2013/09/20 07:10:14, aelias wrote: > Doesn't look like keeping BeginEmulation/EndEmulation separate is useful, please > merge them into the constructor and destructor. Done. https://codereview.chromium.org/23364004/diff/111001/content/renderer/render_... content/renderer/render_widget.cc:1141: owner.reset(input_event); On 2013/09/19 17:27:16, pfeldman wrote: > You can reset unconditionally. Removed.
Thanks for all the changes, this is getting much better. https://codereview.chromium.org/23364004/diff/132001/content/common/view_mess... File content/common/view_messages.h (right): https://codereview.chromium.org/23364004/diff/132001/content/common/view_mess... content/common/view_messages.h:1675: IPC_MESSAGE_ROUTED1(ViewHostMsg_SetExpectedSize, Hmm, this SetExpectedSize thing still feels wrong. The problem is that resize logic is already complex and deadlock-prone and I don't want to make it worse, and the override is also kind of unprincipled. Thinking about it further, I think this can be solved entirely on the render process side with negligible new code after all. You can set the main-frame root layer to masksToBounds in RenderLayerCompositor::ensureRootLayer() when your metrics emulation is active (or even better, just make it masksToBounds for all platforms except Android using a WebSetting), so that the content isn't drawn outside Blink's idea of the viewport. Then just give the true physical_backing_size to CC but the emulated DIP size to Blink. You would also need to tell renderer CC that the background color is white if that's not already the case, so that it fills in the area that's clipped out. I think that should work. My only concern is that the tile management logic may get a bit confused by the overly large physical backing size, but I think it probably won't cause any visible issues, and we have to make that work for OS X overscroll anyway. Let me know if you run into any problems with this approach. https://codereview.chromium.org/23364004/diff/132001/content/renderer/render_... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/23364004/diff/132001/content/renderer/render_... content/renderer/render_widget.cc:217: void OnUpdateScreenRectsMessage(const gfx::Rect view_screen_rect, Either add & or remove the "const". https://codereview.chromium.org/23364004/diff/132001/content/renderer/render_... content/renderer/render_widget.cc:296: float width_ratio = !original_size_.width() ? 1.f : Setting a default value when a divide-by-zero would occur is not very principled and I've seen this kind of thing cause subtle bugs. In reality, when would original_size_.width() be zero? If never, can you DCHECK it's non-empty instead? If sometimes, are you sure you don't need to suppress emulation entirely until it resolves to a sane value? https://codereview.chromium.org/23364004/diff/132001/content/renderer/render_... content/renderer/render_widget.cc:301: scale_ = 1.f / ratio; widget_rect_ being zero would also cause a divide-by-zero here so you should DCHECK that one too.
> You can set the main-frame root layer to masksToBounds in > RenderLayerCompositor::ensureRootLayer() when your metrics emulation is active > (or even better, just make it masksToBounds for all platforms except Android > using a WebSetting), so that the content isn't drawn outside Blink's idea of the > viewport. Then just give the true physical_backing_size to CC but the emulated > DIP size to Blink. You would also need to tell renderer CC that the background > color is white if that's not already the case, so that it fills in the area > that's clipped out. > > I think that should work. My only concern is that the tile management logic may > get a bit confused by the overly large physical backing size, but I think it > probably won't cause any visible issues, and we have to make that work for OS X > overscroll anyway. Let me know if you run into any problems with this approach. I've got one problem with this approach. When I set root layer scale transform, the gutter quads are looking like being transformed twice thus occupying only a part of the empty space. Interestingly, when I move them by one pixel, they fill the whole space (without one-pixel gap). To move them, I change |layer_rect| here: https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... See screenshots here: http://pbrd.co/16Rco5p http://pbrd.co/16Rct9e Any ideas? Thanks, Dmitry
Hmm, it's a bit weird how it sets "root_layer->draw_transform()" on the shared_quad_state then inverts "root_layer->screen_space_transform()". I'm not sure what that achieves to begin with. You might try just setting them both to identity matrix, or the first one to screen_space_transform(), and see what happens. Alternatively, try applying your transform as a sublayer_transform from Blink and see if that hels.
PTAL https://codereview.chromium.org/23364004/diff/132001/content/common/view_mess... File content/common/view_messages.h (right): https://codereview.chromium.org/23364004/diff/132001/content/common/view_mess... content/common/view_messages.h:1675: IPC_MESSAGE_ROUTED1(ViewHostMsg_SetExpectedSize, On 2013/09/27 22:51:14, aelias wrote: > Hmm, this SetExpectedSize thing still feels wrong. The problem is that resize > logic is already complex and deadlock-prone and I don't want to make it worse, > and the override is also kind of unprincipled. Thinking about it further, I > think this can be solved entirely on the render process side with negligible new > code after all. > > You can set the main-frame root layer to masksToBounds in > RenderLayerCompositor::ensureRootLayer() when your metrics emulation is active > (or even better, just make it masksToBounds for all platforms except Android > using a WebSetting), so that the content isn't drawn outside Blink's idea of the > viewport. Then just give the true physical_backing_size to CC but the emulated > DIP size to Blink. You would also need to tell renderer CC that the background > color is white if that's not already the case, so that it fills in the area > that's clipped out. > > I think that should work. My only concern is that the tile management logic may > get a bit confused by the overly large physical backing size, but I think it > probably won't cause any visible issues, and we have to make that work for OS X > overscroll anyway. Let me know if you run into any problems with this approach. This works when setting childrenTransform instead of transform on rootGraphicsLayer. Added "rootLayerClipsContent" web setting. https://codereview.chromium.org/23364004/diff/132001/content/renderer/render_... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/23364004/diff/132001/content/renderer/render_... content/renderer/render_widget.cc:217: void OnUpdateScreenRectsMessage(const gfx::Rect view_screen_rect, On 2013/09/27 22:51:14, aelias wrote: > Either add & or remove the "const". Done. https://codereview.chromium.org/23364004/diff/132001/content/renderer/render_... content/renderer/render_widget.cc:296: float width_ratio = !original_size_.width() ? 1.f : On 2013/09/27 22:51:14, aelias wrote: > Setting a default value when a divide-by-zero would occur is not very principled > and I've seen this kind of thing cause subtle bugs. In reality, when would > original_size_.width() be zero? If never, can you DCHECK it's non-empty > instead? If sometimes, are you sure you don't need to suppress emulation > entirely until it resolves to a sane value? Added DCHECK. https://codereview.chromium.org/23364004/diff/132001/content/renderer/render_... content/renderer/render_widget.cc:301: scale_ = 1.f / ratio; On 2013/09/27 22:51:14, aelias wrote: > widget_rect_ being zero would also cause a divide-by-zero here so you should > DCHECK that one too. Ratio is at least 1.f (see line above).
lgtm modulo minor comments below. Thanks for all the work on this. Please note my comments about the Blink API though in https://codereview.chromium.org/23187005/ that will require some changes on this side as well though. https://codereview.chromium.org/23364004/diff/138001/content/renderer/devtool... File content/renderer/devtools/devtools_agent.cc (right): https://codereview.chromium.org/23364004/diff/138001/content/renderer/devtool... content/renderer/devtools/devtools_agent.cc:155: if (enabled && impl->webview()) No need for the NULL pointer check of impl->webview(), since it's created in the constructor. https://codereview.chromium.org/23364004/diff/138001/content/renderer/render_... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/23364004/diff/138001/content/renderer/render_... content/renderer/render_widget.cc:328: bool need_ack = params.new_size != original_size_; && !params.new_size.IsEmpty() && !params.physical_backing_size.IsEmpty() https://codereview.chromium.org/23364004/diff/138001/content/renderer/render_... content/renderer/render_widget.cc:1043: nit: unnecessary newline https://codereview.chromium.org/23364004/diff/138001/content/renderer/render_... content/renderer/render_widget.cc:2266: return; Could you avoid this "return;" and make use of the ACK send below to avoid a bit of code duplication? https://codereview.chromium.org/23364004/diff/138001/content/renderer/web_pre... File content/renderer/web_preferences.cc (right): https://codereview.chromium.org/23364004/diff/138001/content/renderer/web_pre... content/renderer/web_preferences.cc:350: settings->setRootLayerClipsContent(false); Let's call this setMainFrameClipsContent since it's actually the "container layer" (first child of the root layer) that clips.
Thank you for review! https://codereview.chromium.org/23364004/diff/138001/content/renderer/devtool... File content/renderer/devtools/devtools_agent.cc (right): https://codereview.chromium.org/23364004/diff/138001/content/renderer/devtool... content/renderer/devtools/devtools_agent.cc:155: if (enabled && impl->webview()) On 2013/10/01 08:08:22, aelias wrote: > No need for the NULL pointer check of impl->webview(), since it's created in the > constructor. Done. https://codereview.chromium.org/23364004/diff/138001/content/renderer/render_... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/23364004/diff/138001/content/renderer/render_... content/renderer/render_widget.cc:328: bool need_ack = params.new_size != original_size_; On 2013/10/01 08:08:22, aelias wrote: > && !params.new_size.IsEmpty() && !params.physical_backing_size.IsEmpty() Done. https://codereview.chromium.org/23364004/diff/138001/content/renderer/render_... content/renderer/render_widget.cc:1043: On 2013/10/01 08:08:22, aelias wrote: > nit: unnecessary newline Done. https://codereview.chromium.org/23364004/diff/138001/content/renderer/render_... content/renderer/render_widget.cc:2266: return; On 2013/10/01 08:08:22, aelias wrote: > Could you avoid this "return;" and make use of the ACK send below to avoid a bit > of code duplication? Done. https://codereview.chromium.org/23364004/diff/138001/content/renderer/web_pre... File content/renderer/web_preferences.cc (right): https://codereview.chromium.org/23364004/diff/138001/content/renderer/web_pre... content/renderer/web_preferences.cc:350: settings->setRootLayerClipsContent(false); On 2013/10/01 08:08:22, aelias wrote: > Let's call this setMainFrameClipsContent since it's actually the "container > layer" (first child of the root layer) that clips. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgozman@chromium.org/23364004/162001
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgozman@chromium.org/23364004/162001
Message was sent while issue was closed.
Change committed as 226663 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
