[webkit-reviews] review denied: [Bug 91008] Web Inspector: Override the DeviceOrientation : [Attachment 155584] Patch

[bugzilla-daemon at webkit.org]

Alexander Pavlov (apavlov) <apavlov at chromium.org> has denied Konrad Piascik
<kpiascik at rim.com>'s request for review:
Bug 91008: Web Inspector: Override the DeviceOrientation
https://bugs.webkit.org/show_bug.cgi?id=91008

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

------- Additional Comments from Alexander Pavlov (apavlov)
<apavlov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=155584&action=review


It would be awesome if this feature could play together with the "Device
Metrics Emulation". Meaning, if the device gets rotated from portrait to
landscape (or vice versa), this would also get reflected in the @media
(orientation: ...) evaluation results.

> Source/WebCore/inspector/Inspector.json:388
> +		   "name": "clearDeviceOrientationOverride",

I seem to have seen a similar discussion elsewhere (perhaps, on the Geolocation
mocking patch), mentioning that we usually clear the override by providing
out-of-domain values to a respective set...Override() method (e.g. a screen
resolution of 0x0). What was the consensus?

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1221
> +DeviceOrientationData*
InspectorInstrumentation::overrideDeviceOrientationImpl(InstrumentingAgents*
instrumentingAgents, DeviceOrientationData* deviceOrientation)

The usual way to override something by Instrumentation is to modify an in-out
parameter (see
InspectorInstrumentation::apply[ScreenWidth|ScreenHeight|UserAgent]Override()
and the agent methods they invoke).

> Source/WebCore/inspector/InspectorPageAgent.cpp:1041
> +	   *error = "Internal error: unable to override device orientaiton.";

Typo: "orientaITon" -> "orientation"

> Source/WebCore/inspector/InspectorPageAgent.cpp:1046
> +    clearDeviceOrientationOverride(&clearError);

No need to clear the current override first. Since m_deviceOrientation is a
RefPtr, its contents will get deref'ed (and possibly destroyed) just fine when
a new value is assigned to it.

> Source/WebCore/inspector/InspectorPageAgent.cpp:1068
> +    if (m_deviceOrientation)

The same idea about the apply...Override() approach holds here.

> Source/WebCore/inspector/front-end/SettingsScreen.js:845
> +

Too many blank lines

> Source/WebCore/inspector/front-end/SettingsScreen.js:850
> +	   cellElement.appendChild(document.createTextNode("\u03B1: "));

I have no good ideas about this, but it would be nice to have some kind of a
(graphical?) hint next to the fields, for the user to see which of the angles
is alpha, beta, and gamma. If have something to say, please speak up! :) At the
very least, we can implement it later.

> LayoutTests/inspector/device-orientation-success.html:25
> +	  
InspectorTest.evaluateInPage("window.removeEventListener('deviceorientation',
handler, false);", callbackComplete);

You don't seem to need to remove the listener, as it will not impact the
further test flow (and there will be a new window object for the next test,
right?). Just invoke completeTest() at this point.

> LayoutTests/inspector/device-orientation-success.html:27
> +	   function callbackComplete()

Bad indentation. This also needs a blank line above