Bug 82612 – Web Inspector: [Chromium] Implement Chromium-specific part of the device metrics emulation

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

[Alexander Pavlov (apavlov)]

Patch to follow.

[Alexander Pavlov (apavlov)]

Created attachment 134574 [details]
Patch

[Pavel Feldman]

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)]

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)]

Created attachment 134615 [details]
Patch

[Pavel Feldman]

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 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)]

Created attachment 134804 [details]
Patch

[Pavel Feldman]

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)]

Created attachment 134825 [details]
Patch

[Pavel Feldman]

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 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)]

Committed r112690: <http://trac.webkit.org/changeset/112690>