[webkit-reviews] review denied: [Bug 82612] Web Inspector: [Chromium] Implement Chromium-specific part of the device metrics emulation : [Attachment 134615] Patch

[bugzilla-daemon at webkit.org]

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 134615: Patch
https://bugs.webkit.org/attachment.cgi?id=134615&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.