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

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


> Source/WebCore/inspector/InspectorPageAgent.cpp:787
> +    m_state->setLong(PageAgentState::pageAgentScreenWidthOverride, width);

This method shares name convention with ::applyScreenWidthOverride, yet it does
not mutate its arguments. Also, it is not clear what current vs non-current
values mean.

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

This logic really confuses me. What we want is to set the frame view size. We
should get current one from cookie first, compare it with the requested one and
either apply or quit.