Bug 82612 - Web Inspector: [Chromium] Implement Chromium-specific part of the device metrics emulation
Summary: Web Inspector: [Chromium] Implement Chromium-specific part of the device metr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks: 75963
  Show dependency treegraph
 
Reported: 2012-03-29 07:03 PDT by Alexander Pavlov (apavlov)
Modified: 2012-03-30 10:54 PDT (History)
10 users (show)

See Also:


Attachments
Patch (21.39 KB, patch)
2012-03-29 07:13 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (19.77 KB, patch)
2012-03-29 10:47 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (19.26 KB, patch)
2012-03-30 07:13 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (18.56 KB, patch)
2012-03-30 09:21 PDT, Alexander Pavlov (apavlov)
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2012-03-29 07:03:19 PDT
Patch to follow.
Comment 1 Alexander Pavlov (apavlov) 2012-03-29 07:13:07 PDT
Created attachment 134574 [details]
Patch
Comment 2 Pavel Feldman 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.
Comment 3 Alexander Pavlov (apavlov) 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.
Comment 4 Alexander Pavlov (apavlov) 2012-03-29 10:47:18 PDT
Created attachment 134615 [details]
Patch
Comment 5 Pavel Feldman 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.
Comment 6 Pavel Feldman 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.
Comment 7 Alexander Pavlov (apavlov) 2012-03-30 07:13:22 PDT
Created attachment 134804 [details]
Patch
Comment 8 Pavel Feldman 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?
Comment 9 Alexander Pavlov (apavlov) 2012-03-30 09:21:15 PDT
Created attachment 134825 [details]
Patch
Comment 10 Pavel Feldman 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.
Comment 11 Nat Duca 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.
Comment 12 Alexander Pavlov (apavlov) 2012-03-30 10:54:25 PDT
Committed r112690: <http://trac.webkit.org/changeset/112690>