[webkit-reviews] review denied: [Bug 101797] Web Inspector: Add graphical interface for device orientation override : [Attachment 177788] 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 177788: Patch
https://bugs.webkit.org/attachment.cgi?id=177788&action=review

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


> Source/WebCore/inspector/front-end/OverridesView.js:574
> +	   var updateShapeOrientation;

You should store this._boxElement when you created it and access it using that
property.

> Source/WebCore/inspector/front-end/OverridesView.js:602
> +	   stage.id = "stage";

You should use classes in case you want to show multiple boxes at some point:
createChild("div", "stage")

> Source/WebCore/inspector/front-end/OverridesView.js:617
> +	   importScript("accelerometer.js");

I think we should load it lazily together with the settings screen, just
include it into inspector.html for now.

> Source/WebCore/inspector/front-end/OverridesView.js:618
> +	   WebInspector.Accelerometer.createShape(stage); // Creates a 3D
object in the specified section for changing the Device Orientation Override.

The comment does not help much. Only comment the code that is otherwise
impossible to understand.

> Source/WebCore/inspector/front-end/OverridesView.js:671
> +WebInspector.OverridesView.updateDeviceOrientationOverride = function(alpha,
beta, gamma) {

This should be a private instance method that operates field properties
this._alphaElement, etc.

> Source/WebCore/inspector/front-end/accelerometer.css:1
> +/*

WebKit prefers BSD license for the new files. LGPL v2.1 would also do.

> Source/WebCore/inspector/front-end/accelerometer.css:19
> +section#stage{

Here and below: space before the {

> Source/WebCore/inspector/front-end/accelerometer.css:38
> +    -webkit-transform: rotate3d(1,0,0,-40deg);

Spaces after comas

> Source/WebCore/inspector/front-end/accelerometer.js:1
> +/*

We can only accept BSD or LGPL 2.1 licenses.

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

Please follow the inspector js style  (declare on prototype using : notation or
on object using = notation), { should go on the next line.

> Source/WebCore/inspector/front-end/accelerometer.js:29
> +	       box.style.webkitTransform = "rotateZ("+alpha+"deg)
rotateX("+beta+"deg) rotateY("+gamma+"deg)";

You should use String.sprintf()

> Source/WebCore/inspector/front-end/accelerometer.js:30
> +	      
WebInspector.OverridesView.updateDeviceOrientationOverride(alpha, beta, gamma);


Accelerometer, reusable component should not depend on the embedding view.

> Source/WebCore/inspector/front-end/accelerometer.js:35
> +	   WebInspector.View.prototype._doLoadCSS("accelerometer.css");

Private methods (starting with _) can only be called within the same file. You
should inherit form View in order to use lazy css loading.

> Source/WebCore/inspector/front-end/accelerometer.js:36
> +	   var _mouseDown;

we don't use _ notation for variables.

> Source/WebCore/inspector/front-end/accelerometer.js:56
> +	   node.addEventListener("mousemove", function(mouseMoveEvent) {

Functions should be named and probably bound in this case.