[webkit-reviews] review denied: [Bug 91008] Web Inspector: Override the DeviceOrientation : [Attachment 154961] 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 154961: Patch
https://bugs.webkit.org/attachment.cgi?id=154961&action=review

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


> Source/WebCore/inspector/InspectorPageAgent.cpp:1040
> +	   return;

Please report an error here.

> Source/WebCore/inspector/front-end/SettingsScreen.js:512
> +    _createInput: function(parentElement, id, defaultText, eventListener)

Please annotate new methods with closure.

> Source/WebCore/inspector/front-end/SettingsScreen.js:517
> +	   element.style.width = "80px";

Please use CSS for this one.

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

Please do not use "const" for variables.
Also, sounds like _createCheckbox. Or even better, you could create a "section"
object with the checkbox that would automatically enable / disable a list of
given controls.

> Source/WebCore/inspector/front-end/SettingsScreen.js:783
> +	   var controlsDisabled =
!this._deviceOrientationOverrideCheckboxElement.checked;

I.e. automate this code once for all.

> Source/WebCore/inspector/front-end/UserAgentSupport.js:240
> +    this._alpha = alpha;

You want to make them public so that JSON.stringify created a nice string.

> Source/WebCore/inspector/front-end/UserAgentSupport.js:270
> +WebInspector.UserAgentSupport.DeviceOrientation.parseUserInput =
function(alphaString, betaString, gammaString)

It is really close to
WebInspector.UserAgentSupport.GeolocationPosition.parseUserInput. Could you do:

parseUserInputs(propertyNames, inputValues) where both are arrays and you would
return JSON object? You would not need
WebInspector.UserAgentSupport.DeviceOrientation and
WebInspector.UserAgentSupport.GeolocationPosition then. Loads of boilerplate
code does not make front-end code pretty.

> Source/WebCore/inspector/front-end/UserAgentSupport.js:272
> +    function isUserInputValid(value)

I'm sure I've already seen this method.