[webkit-reviews] review denied: [Bug 82612] Web Inspector: [Chromium] Implement Chromium-specific part of the device metrics emulation : [Attachment 134574] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Mar 29 07:26:45 PDT 2012
Pavel Feldman <pfeldman at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 82612: Web Inspector: [Chromium] Implement Chromium-specific part of the
device metrics emulation
https://bugs.webkit.org/show_bug.cgi?id=82612
Attachment 134574: Patch
https://bugs.webkit.org/attachment.cgi?id=134574&action=review
------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.
More information about the webkit-reviews
mailing list