Patch to follow.
Created attachment 122925 [details] Patch
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.
Created attachment 122939 [details] Patch
(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.
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.
Created attachment 123119 [details] Patch
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...
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.
Created attachment 123131 [details] Patch
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.
Committed r105415: <http://trac.webkit.org/changeset/105415>