Bug 83280 – Web Inspector: [Device Metrics] Introduce the "Fit window" option

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Alexander Pavlov (apavlov)]

Patch to follow.

[Alexander Pavlov (apavlov)]

Created attachment 135854 [details]
Patch

[WebKit Review Bot]

Comment on attachment 135854 [details]
Patch

Attachment 135854 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12339447

New failing tests:
inspector/styles/override-screen-size.html

[WebKit Review Bot]

Created attachment 135872 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 135854 [details]
Patch

Attachment 135854 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12340468

New failing tests:
inspector/styles/override-screen-size.html

[WebKit Review Bot]

Created attachment 135888 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexander Pavlov (apavlov)]

Created attachment 135991 [details]
Patch

[Pavel Feldman]

Comment on attachment 135991 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135991&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:1285
> +    if (!agentPrivate || !agentPrivate->metricsOverridden()) {

I'd start with the positive case.

[Alexander Pavlov (apavlov)]

Created attachment 136256 [details]
Patch

[Pavel Feldman]

Comment on attachment 136256 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136256&action=review

> Source/WebCore/page/DOMWindow.cpp:1081
> +    InspectorInstrumentation::applyScreenHeightOverride(m_frame, &overrideHeight);

I would refactor it to be of the following form:

if (InspectorInstrumentation::shouldApplyScreenHeightOverride(m_frame))
    return view->mapFromLayoutToCSSUnits(static_cast<int>(view->visibleContentRect(false).height()));

...

[Alexander Pavlov (apavlov)]

Created attachment 136268 [details]
Patch

[Daniel Bates]

Comment on attachment 136268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136268&action=review

I briefly looked over this patch. This patch will cause a compile error when building with Inspector disabled (see remarks below).

> Source/WebCore/inspector/InspectorInstrumentation.h:899
> +inline bool InspectorInstrumentation::shouldApplyScreenWidthOverride(Frame* frame)
> +{
> +#if ENABLE(INSPECTOR)
> +    FAST_RETURN_IF_NO_FRONTENDS(false);
> +    if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForFrame(frame))
> +        return shouldApplyScreenWidthOverrideImpl(instrumentingAgents);
> +    return false;
> +#endif
> +}

This will give a compiler warning when building with the Inspector disabled (and with -Wall) since control reaches the end of a non-void function. I suggest moving the "return false;" outside of the #if ENABLE(INSPECTOR) block.

> Source/WebCore/inspector/InspectorInstrumentation.h:909
> +inline bool InspectorInstrumentation::shouldApplyScreenHeightOverride(Frame* frame)
> +{
> +#if ENABLE(INSPECTOR)
> +    FAST_RETURN_IF_NO_FRONTENDS(false);
> +    if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForFrame(frame))
> +        return shouldApplyScreenHeightOverrideImpl(instrumentingAgents);
> +    return false;
> +#endif
> +}

Ditto.

> Source/WebCore/inspector/InspectorPageAgent.cpp:650
> +void InspectorPageAgent::setDeviceMetricsOverride(ErrorString* errorString, const int width, const int height, double fontScaleFactor, bool fitWindow)

Nit: I know that this isn't part of your patch, it seems excessive to declare width and height to be const int given that such parameters are passed by value. I don't see a significant benefit to such const-ness with respect to the body of this function.

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:248
> +        if (m_fitWindow) {
> +            applySizeOverrideIfNecessary();
> +            autoZoomPageToFitWidth(m_webView->mainFrameImpl()->frame());
> +        }

Nit: I suggest using an early return style here to remove one level of indentation.

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:268
> +        if (!m_originalZoomFactor) {
> +            m_webView->setPageScaleFactor(1, WebPoint());
> +            m_webView->setZoomLevel(false, 0);
> +            WebSize scaledEmulatedSize = scaledEmulatedFrameSize(frameView);
> +            m_originalZoomFactor = static_cast<double>(scaledEmulatedSize.width) / frameView->contentsWidth();
> +        }

Nit: I suggest using an early return style here so as to make the control flow for this function standout a bit more and to remove one level of indentation.

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:325
> +    void applySizeOverrideInternal(FrameView* frameView, bool allowFitWindow)

You may want to consider defining an enum for the second parameter, even though this is a private function thats inlined into the class definition, so as to make the value of this parameter clear at the callsite.

[Alexander Pavlov (apavlov)]

Created attachment 136423 [details]
Patch

[Pavel Feldman]

Comment on attachment 136423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136423&action=review

> Source/WebCore/page/DOMWindow.cpp:1080
> +    if (InspectorInstrumentation::shouldApplyScreenHeightOverride(m_frame)) {

bool includeScrollbars = !InspectorInstrumentation::shouldApplyScreenHeightOverride(m_frame);

[Alexander Pavlov (apavlov)]

Committed r113709: <http://trac.webkit.org/changeset/113709>