[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