[webkit-reviews] review denied: [Bug 101797] Web Inspector: Add graphical interface for device orientation override : [Attachment 180539] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Pouya Sadegholvad
<psadegholvad at rim.com>'s request for review:
Bug 101797: Web Inspector: Add graphical interface for device orientation
override
https://bugs.webkit.org/show_bug.cgi?id=101797

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

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


I think you should structure the code as reusable "accelerometer" component
that can be embedded in settings. So far this looks like you are creating
Accelerometer class that you don't even instantiate and introduce bidirectional
Accelerometer <->OverridesView dependencies.

> Source/WebCore/inspector/front-end/Accelerometer.js:21
> +WebInspector.Accelerometer = function(alpha, beta, gamma, node)

Please annotate with @constructor annotation. You could also run
WebCore/inspector/compile-front-end.sh to figure out compiler issues.

> Source/WebCore/inspector/front-end/Accelerometer.js:31
> +    updateShape: function(alpha, beta, gamma, box)

Please annotate.

> Source/WebCore/inspector/front-end/Accelerometer.js:33
> +	   if (!document.getElementById("device-orientation-override-checkbox")
|| !document.getElementById("device-orientation-override-checkbox").checked)

We never do device-orientation-override-checkbox. As I mentioned, your
component should not depend on the embedder.

> Source/WebCore/inspector/front-end/Accelerometer.js:43
> +	       var value = deviceOrientation.toSetting();

Ditto - dependency from the embedder.

> Source/WebCore/inspector/front-end/Accelerometer.js:72
> +	   function _mouseMoveEvent(event)

Local functions should not be declared as private. You should declare large
functions on prototype and bind them.

> Source/WebCore/inspector/front-end/OverridesView.js:582
> +		   WebInspector.Accelerometer.call(this);

You can't call constructors.