RESOLVED FIXED Bug 82612
Web Inspector: [Chromium] Implement Chromium-specific part of the device metrics emulation
https://bugs.webkit.org/show_bug.cgi?id=82612
Summary Web Inspector: [Chromium] Implement Chromium-specific part of the device metr...
Alexander Pavlov (apavlov)
Reported 2012-03-29 07:03:19 PDT
Patch to follow.
Attachments
Patch (21.39 KB, patch)
2012-03-29 07:13 PDT, Alexander Pavlov (apavlov)
no flags
Patch (19.77 KB, patch)
2012-03-29 10:47 PDT, Alexander Pavlov (apavlov)
no flags
Patch (19.26 KB, patch)
2012-03-30 07:13 PDT, Alexander Pavlov (apavlov)
no flags
Patch (18.56 KB, patch)
2012-03-30 09:21 PDT, Alexander Pavlov (apavlov)
pfeldman: review+
Alexander Pavlov (apavlov)
Comment 1 2012-03-29 07:13:07 PDT
Pavel Feldman
Comment 2 2012-03-29 07:26:45 PDT
Comment on attachment 134574 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134574&action=review > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:179 > +WebDevToolsAgentImpl::WidgetFrameMetrics::WidgetFrameMetrics(WebViewImpl* webView) It sounds like you could define it entirely in the cpp. I would call it FrameViewSizeEmulationSupport or similarly. > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:182 > + WebCore::FrameView* view = frameView(); Does it make sense to create these for 0 frame view? > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:318 > + long overrideWidth = 0; Sounds like this code could be next to setDeviceMetrics. > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:321 > + InspectorInstrumentation::applyScreenWidthOverride(frame, &overrideWidth); I think there should be a single method fetching those. > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:343 > +void WebDevToolsAgentImpl::overrideDeviceMetrics(int width, int height, float fontScaleFactor) I would delegate it into the WidgetFrameMetrics > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:365 > +void WebDevToolsAgentImpl::autoZoomPageToFitWidth() ditto > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:406 > +void WebDevToolsAgentImpl::paintViewportGutter(WebCanvas* canvas) ditto > Source/WebKit/chromium/src/WebViewImpl.cpp:1269 > + devToolsAgentPrivate()->didWebFrameResize(webFrame, newSize); You should make it clear that didWebFrameResize resizes frame view as well. Something like: if (devToolsAgentPrivate() && devToolsAgentPrivate()->shouldEmulateSize()) { ... } else { ... } > Source/WebKit/chromium/src/WebViewImpl.cpp:2232 > + frame->setPageAndTextZoomFactors(zoomFactor, m_textZoomFactor); I would use the approach as above.
Alexander Pavlov (apavlov)
Comment 3 2012-03-29 10:38:23 PDT
Attaching a new patch shortly. (In reply to comment #2) > (From update of attachment 134574 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134574&action=review > > > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:179 > > +WebDevToolsAgentImpl::WidgetFrameMetrics::WidgetFrameMetrics(WebViewImpl* webView) > > It sounds like you could define it entirely in the cpp. I would call it FrameViewSizeEmulationSupport or similarly. Fixed. > > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:182 > > + WebCore::FrameView* view = frameView(); > > Does it make sense to create these for 0 frame view? Fixed. > > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:318 > > + long overrideWidth = 0; > > Sounds like this code could be next to setDeviceMetrics. Refactored. > > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:321 > > + InspectorInstrumentation::applyScreenWidthOverride(frame, &overrideWidth); > > I think there should be a single method fetching those. Let's handle this in a separate change, as this requires touching the WebCore. > > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:343 > > +void WebDevToolsAgentImpl::overrideDeviceMetrics(int width, int height, float fontScaleFactor) > > I would delegate it into the WidgetFrameMetrics Done. > > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:365 > > +void WebDevToolsAgentImpl::autoZoomPageToFitWidth() > > ditto Done. > > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:406 > > +void WebDevToolsAgentImpl::paintViewportGutter(WebCanvas* canvas) > > ditto Done. > > Source/WebKit/chromium/src/WebViewImpl.cpp:1269 > > + devToolsAgentPrivate()->didWebFrameResize(webFrame, newSize); > > You should make it clear that didWebFrameResize resizes frame view as well. Something like: > > if (devToolsAgentPrivate() && devToolsAgentPrivate()->shouldEmulateSize()) { > ... > } else { > ... > } Done. > > Source/WebKit/chromium/src/WebViewImpl.cpp:2232 > > + frame->setPageAndTextZoomFactors(zoomFactor, m_textZoomFactor); > > I would use the approach as above. This is a different situation discussed offline.
Alexander Pavlov (apavlov)
Comment 4 2012-03-29 10:47:18 PDT
Pavel Feldman
Comment 5 2012-03-29 11:38:30 PDT
Comment on attachment 134615 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134615&action=review I'll continue review later. > Source/WebKit/chromium/src/WebDevToolsAgentImpl.h:62 > +class DeviceMetricsSupport; Please mind the alphabetic order. > Source/WebKit/chromium/src/WebDevToolsAgentPrivate.h:48 > + virtual void didWebFrameResize(WebFrameImpl*, const WebSize&) = 0; Could you rename this to didResizeWebFrame? Also, please provide documentation for these methods. > Source/WebKit/chromium/src/WebViewImpl.cpp:3001 > + mainFrameImpl()->frame()->setTextZoomFactor(m_emulatedTextZoomFactor); It sounds inconsistent with the cases above. > Source/WebKit/chromium/src/WebViewImpl.h:403 > + float emulatedTextZoomFactor() const Please document these.
Pavel Feldman
Comment 6 2012-03-30 03:26:56 PDT
Comment on attachment 134615 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134615&action=review > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:187 > + return !m_emulatedFrameSize.isEmpty(); Could we encode non-emulated mode with 0 device metrics pointer? > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:193 > + WebSize newSize = WebSize(static_cast<int>(width), static_cast<int>(height)); So you call it with int arguments, then accept long arguments to cast them into ints? > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:197 > + m_emulatedFrameSize = newSize; Can this happen? > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:203 > + bool cancelEmulation = !width && !height; You can have explicit method for this. discard/restore? > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:207 > + view->setHorizontalScrollbarLock(false); Then this method will be extracted. > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:243 > + InspectorInstrumentation::applyScreenWidthOverride(frame, &overrideWidth); You should use the values from setDeviceMetrics instead. > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:249 > + float zoomFactor = static_cast<float>(overrideWidth) / frame->view()->contentsWidth(); This seems to be copying the logic from above. > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:255 > + long overrideWidth = 0; I would expect this method to call setDeviceMetrics with stored values. > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:270 > + WebSize applySizeOverrideInternal(FrameView* frameView, long& overrideWidth, long& overrideHeight) private? > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:287 > + void setSize(const WebSize& size) Rename back to overrideResize to better reflect the nature? > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:293 > + IntSize newSize = IntSize(size.width, size.height); It does not seem to override anything. > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:386 > +void WebDevToolsAgentImpl::didWebFrameResize(WebFrameImpl*, const WebSize& newSize) unused? > Source/WebKit/chromium/src/WebViewImpl.cpp:1265 > + if (!devToolsAgentPrivate() || !devToolsAgentPrivate()->areMetricsEmulated()) { Given that you don't have the alternate branch, metricsOverriden() sounds more intuitive.
Alexander Pavlov (apavlov)
Comment 7 2012-03-30 07:13:22 PDT
Pavel Feldman
Comment 8 2012-03-30 09:00:11 PDT
Comment on attachment 134804 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134804&action=review > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:191 > + return; What for were we creating newSize? > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:200 > + if (!wasEmulated) { You should instead split init and update methods. > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:205 > + Too much white space > Source/WebKit/chromium/src/WebViewImpl.cpp:3447 > + m_layerTreeView.setViewportSize(size()); Why did this change?
Alexander Pavlov (apavlov)
Comment 9 2012-03-30 09:21:15 PDT
Pavel Feldman
Comment 10 2012-03-30 09:27:54 PDT
Comment on attachment 134825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134825&action=review > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:183 > + m_webView->addPageOverlay(this, 97); What is 97? We should introduce a common place with these constants. > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:221 > + void autoZoomPageToFitWidth(Frame* frame) You should have two methods for this. > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:356 > + m_metricsSupport->restore(); I think you should call restore from destructor.
Nat Duca
Comment 11 2012-03-30 10:01:59 PDT
Comment on attachment 134825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134825&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:3447 > + m_layerTreeView.setViewportSize(size()); Nice. Yeah, that seems like its needed.
Alexander Pavlov (apavlov)
Comment 12 2012-03-30 10:54:25 PDT
Note You need to log in before you can comment on or make changes to this bug.