[webkit-reviews] review denied: [Bug 76532] Web Inspector: Implement screen resolution emulation backend : [Attachment 123119] 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 76532: Web Inspector: Implement screen resolution emulation backend
https://bugs.webkit.org/show_bug.cgi?id=76532

Attachment 123119: Patch
https://bugs.webkit.org/attachment.cgi?id=123119&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=123119&action=review


> Source/WebCore/inspector/InspectorPageAgent.cpp:309
> +    // When enabling the agent, override values are restored into the
FrameView.

This code belongs to the ::restore

> Source/WebCore/inspector/InspectorPageAgent.cpp:322
> +    storeScreenSizeOverrides(0, 0);

You don't need to store anything upon disabling agent. InspectorState would not
persist them anyways.

> Source/WebCore/inspector/InspectorPageAgent.cpp:613
> +    if (width == currentWidth && height == currentHeight)

Should you be comparing to the currently installed ones? (in
mainFrame()->view()) ?

> Source/WebCore/inspector/InspectorPageAgent.cpp:617
> +    bool shouldNotify = storeScreenSizeOverrides(width, height);

I would inline this.

> Source/WebCore/inspector/InspectorPageAgent.cpp:618
> +    if (shouldNotify)

No need to check this - we passed the check above.

> Source/WebCore/inspector/InspectorPageAgent.cpp:814
> +    if (!width && !height) {

I would split this method into two methods: clearFrameViewFixedLayout and
update...