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

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman 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 154132: Patch
https://bugs.webkit.org/attachment.cgi?id=154132&action=review

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


Overall looks good.

> Source/WebCore/inspector/Inspector.json:355
> +		   "name": "setDeviceOrientationData",

setDeviceOrientationOverride

> Source/WebCore/inspector/Inspector.json:365
> +		   "name": "clearDeviceOrientationData",

clearDeviceOrientationOverride

> Source/WebCore/inspector/front-end/SettingsScreen.js:352
> +    if (Capabilities.canOverrideDeviceOrientation)

Lets put it behind experiment and check for
WebInspector.experimentsSettings.deviceLocationAndOrientation.isEnabled() (same
applies for device location change).

> Source/WebCore/inspector/front-end/SettingsScreen.js:661
> +	   const p = document.createElement("p");

It looks very similar to the device location / metrics. Could we extract this
checkbox construction into a method?

> Source/WebCore/inspector/front-end/SettingsScreen.js:737
> +	       var element = parentElement.createChild("input");

Please use CSS for styling.

> Source/WebCore/inspector/front-end/UserAgentSupport.js:180
> +	   return (this._alpha || this._alpha == 0) && (this._beta ||
this._beta == 0) && (this._gamma || this._gamma == 0) ? this._alpha + "," +
this._beta + "," + this._gamma : "";

Why not JSON.parse / JSON.stringify ?

> Source/WebCore/inspector/front-end/UserAgentSupport.js:206
> +	   return /^[-]?[0-9]*[.]?[0-9]*$/.test(value);

nit: It is better to prohibit non-digit input in the text field.

> LayoutTests/inspector/device-orientation-success.html:32
> +    InspectorTest.evaluateInPage("var handler = function(evt)
{console.log('alpha: ' + evt.alpha + 'beta: ' + evt.beta + 'gamma: ' +
evt.gamma);}, false);", setupHandler);

This function can be declared in the inline script above.