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

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


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

setDeviceOrientationOverride

Not all backends will support such capability. You should add
canOverrideDeviceOrientation capability (see the device metrics example).

> Source/WebCore/inspector/Inspector.json:389
> +		   ]

This method should be marked as hidden.

> Source/WebCore/inspector/Inspector.json:393
> +		   "description": "clears the overridden DeviceOrientation."

clearDeviceOrientationOverride, also should be hidden.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1202
> +DeviceOrientationData*
InspectorInstrumentation::overrideDeviceOrientationImpl(InstrumentingAgents*
instrumentingAgents, DeviceOrientationData* deviceOrientationData)

Could you declare orientation data as PassRefPtr?

> Source/WebCore/inspector/InspectorInstrumentation.h:1413
> +inline DeviceOrientationData*
InspectorInstrumentation::overrideDeviceOrientation(Page* page,
DeviceOrientationData* deviceOrientationData)

PassRefPtr ?

> Source/WebCore/inspector/front-end/SettingsScreen.js:855
> +	       element.addEventListener("blur",
this._applyDeviceOrientationUserInput.bind(this), false);

You want to align input value to the right.