Bug 91008 - Web Inspector: Override the DeviceOrientation
Summary: Web Inspector: Override the DeviceOrientation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Konrad Piascik
URL:
Keywords:
Depends on:
Blocks: 93552
  Show dependency treegraph
 
Reported: 2012-07-11 13:17 PDT by Konrad Piascik
Modified: 2012-08-10 08:35 PDT (History)
13 users (show)

See Also:


Attachments
Patch (23.25 KB, patch)
2012-07-12 07:10 PDT, Konrad Piascik
no flags Details | Formatted Diff | Diff
Screenshot (66.50 KB, image/png)
2012-07-12 12:43 PDT, Konrad Piascik
no flags Details
Patch (26.72 KB, patch)
2012-07-12 13:22 PDT, Konrad Piascik
no flags Details | Formatted Diff | Diff
Patch (28.57 KB, patch)
2012-07-24 08:28 PDT, Konrad Piascik
no flags Details | Formatted Diff | Diff
Patch (28.55 KB, patch)
2012-07-24 11:27 PDT, Konrad Piascik
no flags Details | Formatted Diff | Diff
Patch (28.57 KB, patch)
2012-07-24 13:25 PDT, Konrad Piascik
no flags Details | Formatted Diff | Diff
Patch (28.57 KB, patch)
2012-07-24 13:41 PDT, Konrad Piascik
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (997.07 KB, application/zip)
2012-07-24 14:05 PDT, WebKit Review Bot
no flags Details
Patch (34.30 KB, patch)
2012-07-26 15:33 PDT, Konrad Piascik
no flags Details | Formatted Diff | Diff
Patch (34.37 KB, patch)
2012-07-27 06:13 PDT, Konrad Piascik
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-06 (307.05 KB, application/zip)
2012-07-27 07:50 PDT, WebKit Review Bot
no flags Details
Patch (34.39 KB, patch)
2012-07-27 08:43 PDT, Konrad Piascik
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (498.80 KB, application/zip)
2012-07-27 09:56 PDT, WebKit Review Bot
no flags Details
Patch (35.45 KB, patch)
2012-07-30 15:10 PDT, Konrad Piascik
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-03 (362.40 KB, application/zip)
2012-07-30 16:30 PDT, WebKit Review Bot
no flags Details
Patch (35.94 KB, patch)
2012-07-31 10:58 PDT, Konrad Piascik
no flags Details | Formatted Diff | Diff
Patch (35.77 KB, patch)
2012-08-01 06:16 PDT, Konrad Piascik
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Konrad Piascik 2012-07-11 13:17:45 PDT
Allow web inspector to override the values recieved by deviceOrientation events.
Comment 1 Konrad Piascik 2012-07-12 07:10:49 PDT
Created attachment 151944 [details]
Patch
Comment 2 Rob Buis 2012-07-12 07:31:33 PDT
Comment on attachment 151944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151944&action=review

Looks good, some stuff to fix.

> Source/WebCore/inspector/InspectorInstrumentation.h:262
> +    static DeviceOrientation* overridenDeviceOrientation(Page*, DeviceOrientation*);

Have you considered wrapping the whole function in #if ENABLE(INSPECTOR)?

> Source/WebCore/inspector/InspectorInstrumentation.h:1369
> +inline DeviceOrientation* InspectorInstrumentation::overridenDeviceOrientation(Page* page, DeviceOrientation* deviceOrientation)

You probably mean overrideDeviceOrientation, or overriddenDeviceOrientation?

> LayoutTests/ChangeLog:10
> +        * inspector/device-orientation-success.html: Added.

No expected result?

> LayoutTests/inspector/device-orientation-success.html:26
> +        function callbackComplete()

Indenting!
Comment 3 Konrad Piascik 2012-07-12 07:42:02 PDT
Comment on attachment 151944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151944&action=review

>> Source/WebCore/inspector/InspectorInstrumentation.h:262
>> +    static DeviceOrientation* overridenDeviceOrientation(Page*, DeviceOrientation*);
> 
> Have you considered wrapping the whole function in #if ENABLE(INSPECTOR)?

This is not the convention of this class.  The Impl functions are wrapped in this enable but the others are not.

>> Source/WebCore/inspector/InspectorInstrumentation.h:1369
>> +inline DeviceOrientation* InspectorInstrumentation::overridenDeviceOrientation(Page* page, DeviceOrientation* deviceOrientation)
> 
> You probably mean overrideDeviceOrientation, or overriddenDeviceOrientation?

will fix.

>> LayoutTests/ChangeLog:10
>> +        * inspector/device-orientation-success.html: Added.
> 
> No expected result?

nope I don't have a way to generate them.

>> LayoutTests/inspector/device-orientation-success.html:26
>> +        function callbackComplete()
> 
> Indenting!

fixing.
Comment 4 Pavel Feldman 2012-07-12 08:31:30 PDT
Comment on attachment 151944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151944&action=review

Looks good overall, couple of comments inline.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1165
> +    return deviceOrientation;

Who owns this object?

>>> Source/WebCore/inspector/InspectorInstrumentation.h:1369
>>> +inline DeviceOrientation* InspectorInstrumentation::overridenDeviceOrientation(Page* page, DeviceOrientation* deviceOrientation)
>> 
>> You probably mean overrideDeviceOrientation, or overriddenDeviceOrientation?
> 
> will fix.

I'd prefer overrideDeviceOrientation

> Source/WebCore/inspector/front-end/SettingsScreen.js:693
> +    _createDeviceOrientationOverrideControl: function()

A screenshot please

> Source/WebCore/inspector/front-end/SettingsScreen.js:695
> +        const deviceOrientationSetting = WebInspector.settings.deviceOrientationOverride.get();

var

> Source/WebCore/inspector/front-end/SettingsScreen.js:727
> +            this._deviceOrientationSectionElement.addStyleClass("hidden");

Please don't hide control - disable them instead.

> Source/WebCore/inspector/front-end/SettingsScreen.js:728
> +            PageAgent.clearDeviceOrientationData();

Should be done via UA

>>> LayoutTests/ChangeLog:10
>>> +        * inspector/device-orientation-success.html: Added.
>> 
>> No expected result?
> 
> nope I don't have a way to generate them.

What do you mean you can't generate them?
Comment 5 Konrad Piascik 2012-07-12 08:44:12 PDT
Comment on attachment 151944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151944&action=review

>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1165
>> +    return deviceOrientation;
> 
> Who owns this object?

This is passed in from DeviceOrientationController::didChangeDeviceOrientation()

>> Source/WebCore/inspector/front-end/SettingsScreen.js:693
>> +    _createDeviceOrientationOverrideControl: function()
> 
> A screenshot please

will attach

>> Source/WebCore/inspector/front-end/SettingsScreen.js:695
>> +        const deviceOrientationSetting = WebInspector.settings.deviceOrientationOverride.get();
> 
> var

ok

>> Source/WebCore/inspector/front-end/SettingsScreen.js:727
>> +            this._deviceOrientationSectionElement.addStyleClass("hidden");
> 
> Please don't hide control - disable them instead.

will disable instead
Comment 6 Pavel Feldman 2012-07-12 08:52:36 PDT
(In reply to comment #5)
> (From update of attachment 151944 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151944&action=review
> 
> >> Source/WebCore/inspector/InspectorInstrumentation.cpp:1165
> >> +    return deviceOrientation;
> > 
> > Who owns this object?
> 
> This is passed in from DeviceOrientationController::didChangeDeviceOrientation()
> 

My point is that it is not clear whether this object is ref counted from the method signature and if it is not, this is likely a memory problem.
Comment 7 Konrad Piascik 2012-07-12 08:55:49 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 151944 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=151944&action=review
> > 
> > >> Source/WebCore/inspector/InspectorInstrumentation.cpp:1165
> > >> +    return deviceOrientation;
> > > 
> > > Who owns this object?
> > 
> > This is passed in from DeviceOrientationController::didChangeDeviceOrientation()
> > 
> 
> My point is that it is not clear whether this object is ref counted from the method signature and if it is not, this is likely a memory problem.

Yes it is refCounted.
Comment 8 Konrad Piascik 2012-07-12 10:58:49 PDT
Comment on attachment 151944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151944&action=review

>>>> LayoutTests/ChangeLog:10
>>>> +        * inspector/device-orientation-success.html: Added.
>>> 
>>> No expected result?
>> 
>> nope I don't have a way to generate them.
> 
> What do you mean you can't generate them?

I'm having difficulty setting up chromium on linux and blackberry doesn't currently support running inspector tests.  Qt doesn't support deviceOrientation and neither does mac build.  I can try setting up chrome on mac and see if that works.
Comment 9 Konrad Piascik 2012-07-12 12:43:44 PDT
Created attachment 152026 [details]
Screenshot
Comment 10 Konrad Piascik 2012-07-12 13:22:48 PDT
Created attachment 152044 [details]
Patch
Comment 11 Pavel Feldman 2012-07-12 14:45:50 PDT
Comment on attachment 152044 [details]
Patch

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.
Comment 12 Konrad Piascik 2012-07-24 08:28:54 PDT
Created attachment 154071 [details]
Patch
Comment 13 Konrad Piascik 2012-07-24 11:27:36 PDT
Created attachment 154113 [details]
Patch
Comment 14 Konrad Piascik 2012-07-24 13:25:23 PDT
Created attachment 154129 [details]
Patch
Comment 15 Early Warning System Bot 2012-07-24 13:31:57 PDT
Comment on attachment 154129 [details]
Patch

Attachment 154129 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13338361
Comment 16 WebKit Review Bot 2012-07-24 13:34:13 PDT
Comment on attachment 154129 [details]
Patch

Attachment 154129 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13331460
Comment 17 Konrad Piascik 2012-07-24 13:41:29 PDT
Created attachment 154132 [details]
Patch
Comment 18 WebKit Review Bot 2012-07-24 14:05:48 PDT
Comment on attachment 154132 [details]
Patch

Attachment 154132 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13335415

New failing tests:
fast/forms/range/slider-onchange-event.html
fast/forms/range/slider-mouse-events.html
fast/forms/range/slider-delete-while-dragging-thumb.html
Comment 19 WebKit Review Bot 2012-07-24 14:05:53 PDT
Created attachment 154137 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 20 Konrad Piascik 2012-07-24 14:30:17 PDT
(In reply to comment #18)
> (From update of attachment 154132 [details])
> Attachment 154132 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13335415
> 
> New failing tests:
> fast/forms/range/slider-onchange-event.html
> fast/forms/range/slider-mouse-events.html
> fast/forms/range/slider-delete-while-dragging-thumb.html

These failures are not related to any code that I've touched and I don't think they're caused by my changes.
Comment 21 Pavel Feldman 2012-07-25 05:43:41 PDT
Comment on attachment 154132 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154132&action=review

Overall looks good.

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

setDeviceOrientationOverride

> Source/WebCore/inspector/Inspector.json:365
> +                "name": "clearDeviceOrientationData",

clearDeviceOrientationOverride

> Source/WebCore/inspector/front-end/SettingsScreen.js:352
> +    if (Capabilities.canOverrideDeviceOrientation)

Lets put it behind experiment and check for WebInspector.experimentsSettings.deviceLocationAndOrientation.isEnabled() (same applies for device location change).

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

It looks very similar to the device location / metrics. Could we extract this checkbox construction into a method?

> Source/WebCore/inspector/front-end/SettingsScreen.js:737
> +            var element = parentElement.createChild("input");

Please use CSS for styling.

> Source/WebCore/inspector/front-end/UserAgentSupport.js:180
> +        return (this._alpha || this._alpha == 0) && (this._beta || this._beta == 0) && (this._gamma || this._gamma == 0) ? this._alpha + "," + this._beta + "," + this._gamma : "";

Why not JSON.parse / JSON.stringify ?

> Source/WebCore/inspector/front-end/UserAgentSupport.js:206
> +        return /^[-]?[0-9]*[.]?[0-9]*$/.test(value);

nit: It is better to prohibit non-digit input in the text field.

> LayoutTests/inspector/device-orientation-success.html:32
> +    InspectorTest.evaluateInPage("var handler = function(evt) {console.log('alpha: ' + evt.alpha + 'beta: ' + evt.beta + 'gamma: ' + evt.gamma);}, false);", setupHandler);

This function can be declared in the inline script above.
Comment 22 Konrad Piascik 2012-07-26 15:33:22 PDT
Created attachment 154771 [details]
Patch
Comment 23 Konrad Piascik 2012-07-26 15:34:04 PDT
Comment on attachment 154132 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154132&action=review

>> Source/WebCore/inspector/front-end/UserAgentSupport.js:206
>> +        return /^[-]?[0-9]*[.]?[0-9]*$/.test(value);
> 
> nit: It is better to prohibit non-digit input in the text field.

How do you want to do that?  Is there an example in the code already?
Comment 24 Konrad Piascik 2012-07-27 06:13:49 PDT
Created attachment 154923 [details]
Patch
Comment 25 Build Bot 2012-07-27 07:30:14 PDT
Comment on attachment 154923 [details]
Patch

Attachment 154923 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13374395
Comment 26 WebKit Review Bot 2012-07-27 07:50:15 PDT
Comment on attachment 154923 [details]
Patch

Attachment 154923 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13362717

New failing tests:
inspector/device-orientation-success.html
Comment 27 WebKit Review Bot 2012-07-27 07:50:22 PDT
Created attachment 154949 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 28 Konrad Piascik 2012-07-27 08:43:40 PDT
(In reply to comment #26)
> (From update of attachment 154923 [details])
> Attachment 154923 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13362717
> 
> New failing tests:
> inspector/device-orientation-success.html

Error in my test re-submitting.
Comment 29 Konrad Piascik 2012-07-27 08:43:51 PDT
Created attachment 154961 [details]
Patch
Comment 30 WebKit Review Bot 2012-07-27 09:56:01 PDT
Comment on attachment 154961 [details]
Patch

Attachment 154961 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13367723

New failing tests:
inspector/device-orientation-success.html
Comment 31 WebKit Review Bot 2012-07-27 09:56:06 PDT
Created attachment 154977 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 32 Pavel Feldman 2012-07-27 11:52:28 PDT
Comment on attachment 154961 [details]
Patch

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.
Comment 33 Konrad Piascik 2012-07-27 12:18:51 PDT
Comment on attachment 154961 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154961&action=review

I understand your desire for refactor and I agree, but I would prefer to land this then refactor all the code at once.  From what I see there are 3 changes that I could lump into a single refactor. 1) CSS for the inputs 2) checkbox for enabling the feature/disabling input 3) parse user input.  I can do all of this, but would rather it be a separate bug.

>> Source/WebCore/inspector/InspectorPageAgent.cpp:1040
>> +        return;
> 
> Please report an error here.

will do

>> Source/WebCore/inspector/front-end/SettingsScreen.js:512
>> +    _createInput: function(parentElement, id, defaultText, eventListener)
> 
> Please annotate new methods with closure.

ok

>> 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.

will change const to var where appropriate.

>> 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.

they are public _ does not denote that a variable is private.
Comment 34 Pavel Feldman 2012-07-27 13:22:24 PDT
> they are public _ does not denote that a variable is private.

But it makes JSON.stringify of it look ugly and that's the value that is stored in the settings. We should not save internal representations, we should make sure that persistence is nice. Declaring them as private is not that important here.
Comment 35 Pavel Feldman 2012-07-27 13:23:29 PDT
> But it makes JSON.stringify of it look ugly and that's the value that is stored in the settings. We should not save internal representations, we should make sure that persistence is nice. Declaring them as private is not that important here.

... i.e. whoever edits the code after you feels free to rename _foo to _bar and he does not realize that it breaks the persistence compatibility.
Comment 36 Konrad Piascik 2012-07-30 15:10:51 PDT
Created attachment 155378 [details]
Patch
Comment 37 WebKit Review Bot 2012-07-30 16:30:03 PDT
Comment on attachment 155378 [details]
Patch

Attachment 155378 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13385686

New failing tests:
inspector/device-orientation-success.html
Comment 38 WebKit Review Bot 2012-07-30 16:30:08 PDT
Created attachment 155394 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 39 Konrad Piascik 2012-07-31 10:58:02 PDT
Created attachment 155584 [details]
Patch
Comment 40 Alexander Pavlov (apavlov) 2012-08-01 02:57:13 PDT
Comment on attachment 155584 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155584&action=review

It would be awesome if this feature could play together with the "Device Metrics Emulation". Meaning, if the device gets rotated from portrait to landscape (or vice versa), this would also get reflected in the @media (orientation: ...) evaluation results.

> Source/WebCore/inspector/Inspector.json:388
> +                "name": "clearDeviceOrientationOverride",

I seem to have seen a similar discussion elsewhere (perhaps, on the Geolocation mocking patch), mentioning that we usually clear the override by providing out-of-domain values to a respective set...Override() method (e.g. a screen resolution of 0x0). What was the consensus?

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

The usual way to override something by Instrumentation is to modify an in-out parameter (see InspectorInstrumentation::apply[ScreenWidth|ScreenHeight|UserAgent]Override() and the agent methods they invoke).

> Source/WebCore/inspector/InspectorPageAgent.cpp:1041
> +        *error = "Internal error: unable to override device orientaiton.";

Typo: "orientaITon" -> "orientation"

> Source/WebCore/inspector/InspectorPageAgent.cpp:1046
> +    clearDeviceOrientationOverride(&clearError);

No need to clear the current override first. Since m_deviceOrientation is a RefPtr, its contents will get deref'ed (and possibly destroyed) just fine when a new value is assigned to it.

> Source/WebCore/inspector/InspectorPageAgent.cpp:1068
> +    if (m_deviceOrientation)

The same idea about the apply...Override() approach holds here.

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

Too many blank lines

> Source/WebCore/inspector/front-end/SettingsScreen.js:850
> +        cellElement.appendChild(document.createTextNode("\u03B1: "));

I have no good ideas about this, but it would be nice to have some kind of a (graphical?) hint next to the fields, for the user to see which of the angles is alpha, beta, and gamma. If have something to say, please speak up! :) At the very least, we can implement it later.

> LayoutTests/inspector/device-orientation-success.html:25
> +        InspectorTest.evaluateInPage("window.removeEventListener('deviceorientation', handler, false);", callbackComplete);

You don't seem to need to remove the listener, as it will not impact the further test flow (and there will be a new window object for the next test, right?). Just invoke completeTest() at this point.

> LayoutTests/inspector/device-orientation-success.html:27
> +        function callbackComplete()

Bad indentation. This also needs a blank line above
Comment 41 Pavel Feldman 2012-08-01 05:36:47 PDT
Comment on attachment 155584 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155584&action=review

Alexander, I think it would make sense for me to complete this review. No need to jump into the process on the 10th patch revision.

>> Source/WebCore/inspector/Inspector.json:388
>> +                "name": "clearDeviceOrientationOverride",
> 
> I seem to have seen a similar discussion elsewhere (perhaps, on the Geolocation mocking patch), mentioning that we usually clear the override by providing out-of-domain values to a respective set...Override() method (e.g. a screen resolution of 0x0). What was the consensus?

The consensus was that we do explicit clear method.

>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1221
>> +DeviceOrientationData* InspectorInstrumentation::overrideDeviceOrientationImpl(InstrumentingAgents* instrumentingAgents, DeviceOrientationData* deviceOrientation)
> 
> The usual way to override something by Instrumentation is to modify an in-out parameter (see InspectorInstrumentation::apply[ScreenWidth|ScreenHeight|UserAgent]Override() and the agent methods they invoke).

I disagree, I like it this way. If you have multiple values to override, you do in-out parameters.

>> Source/WebCore/inspector/InspectorPageAgent.cpp:1068
>> +    if (m_deviceOrientation)
> 
> The same idea about the apply...Override() approach holds here.

@apavlov, I think "apply" is superfluous
Comment 42 Pavel Feldman 2012-08-01 06:05:33 PDT
Comment on attachment 155584 [details]
Patch

Restoring r? so that I saw it at the dashboard.
Comment 43 Konrad Piascik 2012-08-01 06:12:26 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=155584&action=review

>> Source/WebCore/inspector/front-end/SettingsScreen.js:845
>> +
> 
> Too many blank lines

will remove.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:850
>> +        cellElement.appendChild(document.createTextNode("\u03B1: "));
> 
> I have no good ideas about this, but it would be nice to have some kind of a (graphical?) hint next to the fields, for the user to see which of the angles is alpha, beta, and gamma. If have something to say, please speak up! :) At the very least, we can implement it later.

I do have a really good idea of how to implement this.  Take a look at the Ripple emulator and how they do device orientation.  It's an open source project that I want to take code from and re-use in Web Inspector.  I was planning on doing this as a follow up patch.
Comment 44 Konrad Piascik 2012-08-01 06:16:42 PDT
Created attachment 155795 [details]
Patch

Fixed type, removed blank line and updated test to use fewer callbacks.
Comment 45 WebKit Review Bot 2012-08-02 12:29:43 PDT
Comment on attachment 155795 [details]
Patch

Clearing flags on attachment: 155795

Committed r124484: <http://trac.webkit.org/changeset/124484>
Comment 46 WebKit Review Bot 2012-08-02 12:29:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 47 Ryosuke Niwa 2012-08-04 18:33:31 PDT
The test added by this patch has been failing on Mac:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=inspector%2Fdevice-orientation-success.html


--- /Volumes/Data/slave/lion-debug-tests-wk1/build/layout-test-results/inspector/device-orientation-success-expected.txt
+++ /Volumes/Data/slave/lion-debug-tests-wk1/build/layout-test-results/inspector/device-orientation-success-actual.txt
@@ -1,3 +1,1 @@
-CONSOLE MESSAGE: line 17: alpha: 20 beta: 30 gamma: 40
-CONSOLE MESSAGE: line 17: alpha: 1.1 beta: 2.2 gamma: 3.3
Comment 48 Pavel Feldman 2012-08-09 23:57:59 PDT
@Konrad Piascik: are you following up on the test failure? We might need to roll this change out in case it fails tests consistently.
Comment 49 Konrad Piascik 2012-08-10 08:35:45 PDT
(In reply to comment #48)
> @Konrad Piascik: are you following up on the test failure? We might need to roll this change out in case it fails tests consistently.

I can't investigate this until tuesday, but think that it should be skipped, if this is urgent.