WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 82612
Web Inspector: [Chromium] Implement Chromium-specific part of the device metrics emulation
https://bugs.webkit.org/show_bug.cgi?id=82612
Summary
Web Inspector: [Chromium] Implement Chromium-specific part of the device metr...
Alexander Pavlov (apavlov)
Reported
2012-03-29 07:03:19 PDT
Patch to follow.
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Alexander Pavlov (apavlov)
Comment 1
2012-03-29 07:13:07 PDT
Created
attachment 134574
[details]
Patch
Pavel Feldman
Comment 2
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.
Alexander Pavlov (apavlov)
Comment 3
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.
Alexander Pavlov (apavlov)
Comment 4
2012-03-29 10:47:18 PDT
Created
attachment 134615
[details]
Patch
Pavel Feldman
Comment 5
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.
Pavel Feldman
Comment 6
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.
Alexander Pavlov (apavlov)
Comment 7
2012-03-30 07:13:22 PDT
Created
attachment 134804
[details]
Patch
Pavel Feldman
Comment 8
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?
Alexander Pavlov (apavlov)
Comment 9
2012-03-30 09:21:15 PDT
Created
attachment 134825
[details]
Patch
Pavel Feldman
Comment 10
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.
Nat Duca
Comment 11
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.
Alexander Pavlov (apavlov)
Comment 12
2012-03-30 10:54:25 PDT
Committed
r112690
: <
http://trac.webkit.org/changeset/112690
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug