Bug 76532 – Web Inspector: Implement screen resolution emulation backend

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Alexander Pavlov (apavlov)]

Patch to follow.

[Alexander Pavlov (apavlov)]

Created attachment 122925 [details]
Patch

[Pavel Feldman]

Comment on attachment 122925 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122925&action=review

> Source/WebCore/inspector/Inspector.json:304
> +                "name": "setScreenSizeOverride",

"hidden": true

> Source/WebCore/inspector/InspectorCSSAgent.cpp:238
> +    m_instrumentingAgents->setInspectorCSSAgent(this);

Is this a part of this change? It seems like you ended up putting screen size logic into the page agent.

> Source/WebCore/inspector/InspectorPageAgent.cpp:575
> +void InspectorPageAgent::setScreenSizeOverride(ErrorString* errorString, const int width, const int height)

When navigating, you need to call this method upon agent's ::restore. You will need to use width and height from the cookie in that case.

> Source/WebCore/inspector/InspectorPageAgent.cpp:584
> +    if (height < 0) {

Combine these all into a single check.

> Source/WebCore/inspector/InspectorPageAgent.cpp:611
> +            m_originalFixedLayoutSize = adoptPtr(new IntSize(mainFrame()->view()->fixedLayoutSize()));

// Turning on.

[Alexander Pavlov (apavlov)]

Created attachment 122939 [details]
Patch

[Alexander Pavlov (apavlov)]

(In reply to comment #2)
> (From update of attachment 122925 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122925&action=review
> 
> > Source/WebCore/inspector/Inspector.json:304
> > +                "name": "setScreenSizeOverride",
> 
> "hidden": true

Done.

> > Source/WebCore/inspector/InspectorCSSAgent.cpp:238
> > +    m_instrumentingAgents->setInspectorCSSAgent(this);
> 
> Is this a part of this change? It seems like you ended up putting screen size logic into the page agent.

Right, we need to use the CSS agent even when it is disabled.

> > Source/WebCore/inspector/InspectorPageAgent.cpp:575
> > +void InspectorPageAgent::setScreenSizeOverride(ErrorString* errorString, const int width, const int height)
> 
> When navigating, you need to call this method upon agent's ::restore. You will need to use width and height from the cookie in that case.

Done. I ended up fixing the enable() and disable() methods. Is that correct?

> > Source/WebCore/inspector/InspectorPageAgent.cpp:584
> > +    if (height < 0) {
> 
> Combine these all into a single check.

Done.

> > Source/WebCore/inspector/InspectorPageAgent.cpp:611
> > +            m_originalFixedLayoutSize = adoptPtr(new IntSize(mainFrame()->view()->fixedLayoutSize()));
> 
> // Turning on.

Commented the branches.

[Pavel Feldman]

Comment on attachment 122939 [details]
Patch

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.

[Alexander Pavlov (apavlov)]

Created attachment 123119 [details]
Patch

[Pavel Feldman]

Comment on attachment 123119 [details]
Patch

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...

[Alexander Pavlov (apavlov)]

Updated patch to be attached shortly.

(In reply to comment #7)
> (From update of attachment 123119 [details])
> 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

Shouldn't we revert the FrameView size overrides when the agent is disabled and restore them when the agent is enabled?

> > 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.

Does InspectorState not survive agent disablements? In the disabled case apply...Override calls will be cut off by the absence of the agent in the InstrumentingAgents store but the respective settings will be still there, correct? I agree that there is no point in resetting the overrides when disabling the agent, so that they will be restored on enabling it.

> > Source/WebCore/inspector/InspectorPageAgent.cpp:613
> > +    if (width == currentWidth && height == currentHeight)
> 
> Should you be comparing to the currently installed ones? (in mainFrame()->view()) ?

What if we didn't override anything but coincidentally user-specified width and height are the same as those found in mainFrame()->view()? We will bail out without storing the overrides in m_state, and the window.screen/device-width|height will not get overridden.

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

Done.

> > Source/WebCore/inspector/InspectorPageAgent.cpp:618
> > +    if (shouldNotify)
> 
> No need to check this - we passed the check above.

Done.

> > Source/WebCore/inspector/InspectorPageAgent.cpp:814
> > +    if (!width && !height) {
> 
> I would split this method into two methods: clearFrameViewFixedLayout and update...

Methods extracted.

[Alexander Pavlov (apavlov)]

Created attachment 123131 [details]
Patch

[Pavel Feldman]

Comment on attachment 123131 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123131&action=review

Few nits, otherwise lgtm.

> Source/WebCore/inspector/InspectorPageAgent.cpp:289
> +    if (m_originalFixedLayoutSize) {

nuke this code?

> Source/WebCore/inspector/InspectorPageAgent.cpp:621
> +    notifyScreenSizeChanged();

Inline this into update.

[Alexander Pavlov (apavlov)]

Committed r105415: <http://trac.webkit.org/changeset/105415>