Bug 94479 - Respect runtime flags for Device Orientation and Device Motion
Summary: Respect runtime flags for Device Orientation and Device Motion
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hans Wennborg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-20 07:20 PDT by Hans Wennborg
Modified: 2012-08-21 07:40 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.69 KB, patch)
2012-08-20 07:25 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (330.19 KB, application/zip)
2012-08-20 08:02 PDT, WebKit Review Bot
no flags Details
Patch (4.33 KB, patch)
2012-08-20 08:43 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Wennborg 2012-08-20 07:20:40 PDT
Respect runtime flags for Device Orientation and Device Motion
Comment 1 Hans Wennborg 2012-08-20 07:25:23 PDT
Created attachment 159421 [details]
Patch
Comment 2 Hans Wennborg 2012-08-20 07:27:28 PDT
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.
Comment 3 WebKit Review Bot 2012-08-20 08:02:18 PDT
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
Comment 4 WebKit Review Bot 2012-08-20 08:02:21 PDT
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
Comment 5 Hans Wennborg 2012-08-20 08:43:26 PDT
Created attachment 159438 [details]
Patch
Comment 6 Peter Beverloo 2012-08-20 08:52:21 PDT
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?
Comment 7 Hans Wennborg 2012-08-20 09:00:39 PDT
(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.
Comment 8 Adam Barth 2012-08-20 11:59:11 PDT
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
Comment 9 WebKit Review Bot 2012-08-20 12:54:00 PDT
Comment on attachment 159438 [details]
Patch

Clearing flags on attachment: 159438

Committed r126057: <http://trac.webkit.org/changeset/126057>
Comment 10 WebKit Review Bot 2012-08-20 12:54:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Hans Wennborg 2012-08-21 00:59:40 PDT
(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?
Comment 12 Adam Barth 2012-08-21 07:40:18 PDT
> Crazy to remove it or crazy that it was there in the first place?

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