Bug 94479 – Respect runtime flags for Device Orientation and Device Motion

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Hans Wennborg]

Respect runtime flags for Device Orientation and Device Motion

[Hans Wennborg]

Created attachment 159421 [details]
Patch

[Hans Wennborg]

Adam, would you like to take a look?

I guess we want to get rid off these runtime flags eventually, but when we start rolling out Device Motion in Chromium it would be nice if they actually work.

[WebKit Review Bot]

Comment on attachment 159421 [details]
Patch

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

New failing tests:
fast/dom/Window/window-properties-device-orientation.html

[WebKit Review Bot]

Created attachment 159430 [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

[Hans Wennborg]

Created attachment 159438 [details]
Patch

[Peter Beverloo]

Comment on attachment 159438 [details]
Patch

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

Since RuntimeEnabledFeatures defaults to enabling Device Motion, and you now only explicitly disable it for the TestShell (rather than for everything in WebViewImpl) and ENABLE_DEVICE_ORIENTATION is set to 1 for all of Chromium, I guess this would enable Device Motion for Chromium? Or do we disable this somewhere on the Chromium side?

If it does enable Device Motion and that is an intentional effect, please emphasize on that in the ChangeLog.

> Source/WebCore/page/DOMWindow.cpp:1579
> +    else if (eventType == eventNames().devicemotionEvent && RuntimeEnabledFeatures::deviceMotionEnabled()) {

nit: is it correct to implement both device orientation as device motion under the same compile-time flag?

[Hans Wennborg]

(In reply to comment #6)
> (From update of attachment 159438 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159438&action=review
> 
> Since RuntimeEnabledFeatures defaults to enabling Device Motion, and you now only explicitly disable it for the TestShell (rather than for everything in WebViewImpl) and ENABLE_DEVICE_ORIENTATION is set to 1 for all of Chromium, I guess this would enable Device Motion for Chromium? Or do we disable this somewhere on the Chromium side?

We disable it on the Chromium side (render_thread_impl.cc:621) based on a command-line flag:

  WebRuntimeFeatures::enableDeviceMotion(
      command_line.HasSwitch(switches::kEnableDeviceMotion));

> > Source/WebCore/page/DOMWindow.cpp:1579
> > +    else if (eventType == eventNames().devicemotionEvent && RuntimeEnabledFeatures::deviceMotionEnabled()) {
> 
> nit: is it correct to implement both device orientation as device motion under the same compile-time flag?

Yes. Device motion and device orientation share the same flag as they're just two features of the same spec.

[Adam Barth]

Comment on attachment 159438 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:-270
> -    // Keep runtime flag for device motion turned off until it's implemented.
> -    WebRuntimeFeatures::enableDeviceMotion(false);

That's crazy

[WebKit Review Bot]

Comment on attachment 159438 [details]
Patch

Clearing flags on attachment: 159438

Committed r126057: <http://trac.webkit.org/changeset/126057>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hans Wennborg]

(In reply to comment #8)
> (From update of attachment 159438 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159438&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:-270
> > -    // Keep runtime flag for device motion turned off until it's implemented.
> > -    WebRuntimeFeatures::enableDeviceMotion(false);
> 
> That's crazy

Crazy to remove it or crazy that it was there in the first place?

[Adam Barth]

> Crazy to remove it or crazy that it was there in the first place?

Crazy that it was there in the first place.  :)