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

[bugzilla-daemon at webkit.org]

Alexander Pavlov (apavlov) <apavlov at chromium.org> has canceled 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 173642: Patch
https://bugs.webkit.org/attachment.cgi?id=173642&action=review

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


http://www.webkit.org/coding/coding-style.html is a nice guide to the WebKit
coding style.

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

Not sure if these removed lines should have actually been removed. They
separate the logical chunks of code inside methods.

> Source/WebCore/inspector/front-end/SettingsScreen.js:893
> +	   if (canvas = document.getElementById('accelerometer-canvas'))

Web Inspector uses double quotes for strings.

> Source/WebCore/inspector/front-end/SettingsScreen.js:1047
> +	   if(document.getElementById('device-orientation-override-beta')){

Lacks whitespace around (...)

> Source/WebCore/inspector/front-end/accelerometer.js:6
> + * Permission is hereby granted, free of charge, to any person obtaining a
copy of this software and associated documentation files (the "Software"), to
deal in the Software without restriction, including without limitation the
rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
sell copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

Please re-format this to follow the other files' license header formatting.

> Source/WebCore/inspector/front-end/accelerometer.js:19
> +	   // for some strange reason for y -100 is top, 100 is bottom

IIRC, this depends on the direction of the Z-axis and the coordinate system
orientation (left-handed vs. right-handed).

> Source/WebCore/inspector/front-end/accelerometer.js:55
> +	   if(document.getElementById('device-orientation-override-checkbox')
&& document.getElementById('device-orientation-override-checkbox').checked) {

Whitespace after "if".

Also, this should be turned into a guard return to avoid the extra indentation.


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

Odd whitespace before '('

> Source/WebCore/inspector/front-end/accelerometer.js:73
> +	       _offsets = {

AFAIK we have a separate "var" for every var.

> Source/WebCore/inspector/front-end/accelerometer.js:74
> +	       x: 0,

wrong indentation

> Source/WebCore/inspector/front-end/accelerometer.js:85
> +	   importScript("3d.js");

Please avoid this and include/load the necessary scripts as other scripts do.

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

Web Inspector does not use anonymous functions. You should create nested named
functions instead.

> Source/WebCore/inspector/front-end/accelerometer.js:97
> +	       }      

"else" on this line in all cases, please

> Source/WebCore/inspector/front-end/accelerometer.js:105
> +		   if (gamma < -90) {

No braces for 1-line blocks.

> Source/WebCore/inspector/front-end/accelerometer.js:112
> +		   // enforce beta in [-180,180] as per w3c spec

Should be "Sentence-capitalized text." with a period at the end.