WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
91008
Web Inspector: Override the DeviceOrientation
https://bugs.webkit.org/show_bug.cgi?id=91008
Summary
Web Inspector: Override the DeviceOrientation
Konrad Piascik
Reported
2012-07-11 13:17:45 PDT
Allow web inspector to override the values recieved by deviceOrientation events.
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
Show Obsolete
(11)
View All
Add attachment
proposed patch, testcase, etc.
Konrad Piascik
Comment 1
2012-07-12 07:10:49 PDT
Created
attachment 151944
[details]
Patch
Rob Buis
Comment 2
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!
Konrad Piascik
Comment 3
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.
Pavel Feldman
Comment 4
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?
Konrad Piascik
Comment 5
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
Pavel Feldman
Comment 6
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.
Konrad Piascik
Comment 7
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.
Konrad Piascik
Comment 8
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.
Konrad Piascik
Comment 9
2012-07-12 12:43:44 PDT
Created
attachment 152026
[details]
Screenshot
Konrad Piascik
Comment 10
2012-07-12 13:22:48 PDT
Created
attachment 152044
[details]
Patch
Pavel Feldman
Comment 11
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.
Konrad Piascik
Comment 12
2012-07-24 08:28:54 PDT
Created
attachment 154071
[details]
Patch
Konrad Piascik
Comment 13
2012-07-24 11:27:36 PDT
Created
attachment 154113
[details]
Patch
Konrad Piascik
Comment 14
2012-07-24 13:25:23 PDT
Created
attachment 154129
[details]
Patch
Early Warning System Bot
Comment 15
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
WebKit Review Bot
Comment 16
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
Konrad Piascik
Comment 17
2012-07-24 13:41:29 PDT
Created
attachment 154132
[details]
Patch
WebKit Review Bot
Comment 18
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
WebKit Review Bot
Comment 19
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
Konrad Piascik
Comment 20
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.
Pavel Feldman
Comment 21
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.
Konrad Piascik
Comment 22
2012-07-26 15:33:22 PDT
Created
attachment 154771
[details]
Patch
Konrad Piascik
Comment 23
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?
Konrad Piascik
Comment 24
2012-07-27 06:13:49 PDT
Created
attachment 154923
[details]
Patch
Build Bot
Comment 25
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
WebKit Review Bot
Comment 26
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
WebKit Review Bot
Comment 27
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
Konrad Piascik
Comment 28
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.
Konrad Piascik
Comment 29
2012-07-27 08:43:51 PDT
Created
attachment 154961
[details]
Patch
WebKit Review Bot
Comment 30
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
WebKit Review Bot
Comment 31
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
Pavel Feldman
Comment 32
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.
Konrad Piascik
Comment 33
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.
Pavel Feldman
Comment 34
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.
Pavel Feldman
Comment 35
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.
Konrad Piascik
Comment 36
2012-07-30 15:10:51 PDT
Created
attachment 155378
[details]
Patch
WebKit Review Bot
Comment 37
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
WebKit Review Bot
Comment 38
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
Konrad Piascik
Comment 39
2012-07-31 10:58:02 PDT
Created
attachment 155584
[details]
Patch
Alexander Pavlov (apavlov)
Comment 40
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
Pavel Feldman
Comment 41
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
Pavel Feldman
Comment 42
2012-08-01 06:05:33 PDT
Comment on
attachment 155584
[details]
Patch Restoring r? so that I saw it at the dashboard.
Konrad Piascik
Comment 43
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.
Konrad Piascik
Comment 44
2012-08-01 06:16:42 PDT
Created
attachment 155795
[details]
Patch Fixed type, removed blank line and updated test to use fewer callbacks.
WebKit Review Bot
Comment 45
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
>
WebKit Review Bot
Comment 46
2012-08-02 12:29:50 PDT
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 47
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
Pavel Feldman
Comment 48
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.
Konrad Piascik
Comment 49
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug