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
Screenshot (66.50 KB, image/png)
2012-07-12 12:43 PDT, Konrad Piascik
no flags
Patch (26.72 KB, patch)
2012-07-12 13:22 PDT, Konrad Piascik
no flags
Patch (28.57 KB, patch)
2012-07-24 08:28 PDT, Konrad Piascik
no flags
Patch (28.55 KB, patch)
2012-07-24 11:27 PDT, Konrad Piascik
no flags
Patch (28.57 KB, patch)
2012-07-24 13:25 PDT, Konrad Piascik
no flags
Patch (28.57 KB, patch)
2012-07-24 13:41 PDT, Konrad Piascik
no flags
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
Patch (34.30 KB, patch)
2012-07-26 15:33 PDT, Konrad Piascik
no flags
Patch (34.37 KB, patch)
2012-07-27 06:13 PDT, Konrad Piascik
no flags
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
Patch (34.39 KB, patch)
2012-07-27 08:43 PDT, Konrad Piascik
no flags
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
Patch (35.45 KB, patch)
2012-07-30 15:10 PDT, Konrad Piascik
no flags
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
Patch (35.94 KB, patch)
2012-07-31 10:58 PDT, Konrad Piascik
no flags
Patch (35.77 KB, patch)
2012-08-01 06:16 PDT, Konrad Piascik
no flags
Konrad Piascik
Comment 1 2012-07-12 07:10:49 PDT
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
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
Konrad Piascik
Comment 13 2012-07-24 11:27:36 PDT
Konrad Piascik
Comment 14 2012-07-24 13:25:23 PDT
Early Warning System Bot
Comment 15 2012-07-24 13:31:57 PDT
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
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
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
Build Bot
Comment 25 2012-07-27 07:30:14 PDT
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
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
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
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.