WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
76532
Web Inspector: Implement screen resolution emulation backend
https://bugs.webkit.org/show_bug.cgi?id=76532
Summary
Web Inspector: Implement screen resolution emulation backend
Alexander Pavlov (apavlov)
Reported
2012-01-18 05:24:37 PST
Patch to follow.
Attachments
Patch
(25.45 KB, patch)
2012-01-18 07:20 PST
,
Alexander Pavlov (apavlov)
no flags
Details
Formatted Diff
Diff
Patch
(29.14 KB, patch)
2012-01-18 08:49 PST
,
Alexander Pavlov (apavlov)
no flags
Details
Formatted Diff
Diff
Patch
(29.12 KB, patch)
2012-01-19 06:15 PST
,
Alexander Pavlov (apavlov)
no flags
Details
Formatted Diff
Diff
Patch
(30.60 KB, patch)
2012-01-19 08:07 PST
,
Alexander Pavlov (apavlov)
pfeldman
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Alexander Pavlov (apavlov)
Comment 1
2012-01-18 07:20:16 PST
Created
attachment 122925
[details]
Patch
Pavel Feldman
Comment 2
2012-01-18 07:38:41 PST
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)
Comment 3
2012-01-18 08:49:17 PST
Created
attachment 122939
[details]
Patch
Alexander Pavlov (apavlov)
Comment 4
2012-01-18 08:51:16 PST
(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 5
2012-01-19 03:57:11 PST
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)
Comment 6
2012-01-19 06:15:39 PST
Created
attachment 123119
[details]
Patch
Pavel Feldman
Comment 7
2012-01-19 06:32:01 PST
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)
Comment 8
2012-01-19 07:32:54 PST
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)
Comment 9
2012-01-19 08:07:28 PST
Created
attachment 123131
[details]
Patch
Pavel Feldman
Comment 10
2012-01-19 08:26:28 PST
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)
Comment 11
2012-01-19 08:40:16 PST
Committed
r105415
: <
http://trac.webkit.org/changeset/105415
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug