Bug 42265 - Runtime feature switch for device orientation
: Runtime feature switch for device orientation
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
:
Depends on: 42380 42648
Blocks: 41616
  Show dependency treegraph
 
Reported: 2010-07-14 08:31 PDT by Hans Wennborg
Modified: 2010-07-21 03:42 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.28 KB, patch)
2010-07-14 08:37 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (9.00 KB, patch)
2010-07-14 10:14 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (11.29 KB, patch)
2010-07-15 04:31 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (9.00 KB, patch)
2010-07-20 06:37 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (15.12 KB, patch)
2010-07-21 02:25 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 2010-07-14 08:31:03 PDT
Runtime feature switch for device orientation
Comment 1 Hans Wennborg 2010-07-14 08:37:54 PDT
Created attachment 61526 [details]
Patch
Comment 2 Early Warning System Bot 2010-07-14 08:52:17 PDT
Attachment 61526 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3430277
Comment 3 Jeremy Orlow 2010-07-14 08:55:09 PDT
Comment on attachment 61526 [details]
Patch

WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:50
 +  bool RuntimeEnabledFeatures::isDeviceOrientationEventEnabled = true;
I think you can omit "Event" from all of these names.


WebCore/page/DOMWindow.cpp:1416
 +      else if (eventType == eventNames().deviceorientationEvent && frame() && frame()->page() && RuntimeEnabledFeatures::deviceOrientationEventEnabled())
I'd slightly prefer putting this logic into the deviceOrientation() method and having that return 0 if it's turned off at runtime.  (And thus check for it being null here.)

At very least, you should add a double check in that method to make sure it only gets instantiated when it's enabled.


Please add a test in chromium to verify this stuff can't be seen when the flag is off.
Comment 4 Steve Block 2010-07-14 09:03:16 PDT
Comment on attachment 61526 [details]
Patch

WebCore/bindings/generic/RuntimeEnabledFeatures.h:116
 +  #if ENABLE(DEVICE_ORIENTATION)
It looks like some features choose to guard these blocks, others don't. I think we shouldn't use guards unless we need to, it just clutters the code.

WebCore/bindings/generic/RuntimeEnabledFeatures.h:117
 +      static void setDeviceOrientationEventEnabled(bool isEnabled) { isDeviceOrientationEventEnabled = isEnabled; }
I think the flag should just be named for the feature, ie 'DeviceOrientation', in keeping with the other features.

WebCore/page/DOMWindow.idl:308
 +          attribute [Conditional=DEVICE_ORIENTATION,EnabledAtRuntime] EventListener ondeviceorientation;
There should probably be a space here, though I notice the rest of the file is inconsistent
Comment 5 Hans Wennborg 2010-07-14 10:11:34 PDT
(In reply to comment #3)
> (From update of attachment 61526 [details])
> WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:50
>  +  bool RuntimeEnabledFeatures::isDeviceOrientationEventEnabled = true;
> I think you can omit "Event" from all of these names.
Done. (Except deviceOrientationEventEnabled() which is needed for the bindings)

> 
> 
> WebCore/page/DOMWindow.cpp:1416
>  +      else if (eventType == eventNames().deviceorientationEvent && frame() && frame()->page() && RuntimeEnabledFeatures::deviceOrientationEventEnabled())
> I'd slightly prefer putting this logic into the deviceOrientation() method and having that return 0 if it's turned off at runtime.  (And thus check for it being null here.)
Instantiation is not done in deviceOrientation(), but in the initializer list of the Page constructor, so checking the flag there.

> 
> 
> Please add a test in chromium to verify this stuff can't be seen when the flag is off.
Will do.
Comment 6 Hans Wennborg 2010-07-14 10:12:05 PDT
(In reply to comment #4)
> (From update of attachment 61526 [details])
> WebCore/bindings/generic/RuntimeEnabledFeatures.h:116
>  +  #if ENABLE(DEVICE_ORIENTATION)
> It looks like some features choose to guard these blocks, others don't. I think we shouldn't use guards unless we need to, it just clutters the code.
Done. Removing the gurads.

> 
> WebCore/bindings/generic/RuntimeEnabledFeatures.h:117
>  +      static void setDeviceOrientationEventEnabled(bool isEnabled) { isDeviceOrientationEventEnabled = isEnabled; }
> I think the flag should just be named for the feature, ie 'DeviceOrientation', in keeping with the other features.
Done.

> 
> WebCore/page/DOMWindow.idl:308
>  +          attribute [Conditional=DEVICE_ORIENTATION,EnabledAtRuntime] EventListener ondeviceorientation;
> There should probably be a space here, though I notice the rest of the file is inconsistent
Done.
Comment 7 Hans Wennborg 2010-07-14 10:13:49 PDT
(In reply to comment #2)
> Attachment 61526 [details] did not build on qt:
> Build output: http://webkit-commit-queue.appspot.com/results/3430277

Not sure what to do about this. Seems the problem is that WebKit/WebCore/bindings/generic isn't on the search path for include files (only WebCore/bindings and WebCore/bindings/js is).

Do you think this is an error in my code (i.e. one is not allowed to use that header from where I use it), or something that should be fixed in the Qt makefiles?
Comment 8 Hans Wennborg 2010-07-14 10:14:40 PDT
Created attachment 61531 [details]
Patch
Comment 9 Early Warning System Bot 2010-07-14 10:25:55 PDT
Attachment 61531 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3490286
Comment 10 Jeremy Orlow 2010-07-14 11:34:23 PDT
Darin should take a look as well.
Comment 11 Jeremy Orlow 2010-07-14 11:34:47 PDT
Comment on attachment 61531 [details]
Patch

WebCore/page/Page.cpp:143
 +      , m_deviceOrientationController(RuntimeEnabledFeatures::deviceOrientationEnabled() ? new DeviceOrientationController(this, deviceOrientationClient) : 0)
Should we be creating this lazily on first use?  I see that at least the geolocation one isn't, but maybe that should be changed too?  (It's a shame to slow down Page creation for features that may or may never be accessed.  Doing this lazily is generally the webkit way.)  It's ok to do this in another patch, but I think it should be done unless there's a good reason not to.

WebKit/chromium/src/WebRuntimeFeatures.cpp:231
 +  #if ENABLE(DEVICE_ORIENTATION)
Do we need these guards?  It seems like not.


WebKit/chromium/src/WebRuntimeFeatures.cpp:236
 +  bool WebRuntimeFeatures::isDeviceOrientationEnabled()
Interesting.  I was going to say the same here, but I realized that we don't want it to say true when support isn't even compiled in.  But then I realized I could say the same about what's in WebCore as well.  I almost wonder if it should default to false if the feature isn't compiled in.  But in that case, we should probably do the same for the other features.

What do you think?
Comment 12 Steve Block 2010-07-14 11:43:32 PDT
Comment on attachment 61531 [details]
Patch

lgtm, other than the qt build problem
Comment 13 Steve Block 2010-07-14 11:46:15 PDT
> Should we be creating this lazily on first use?
I don't think this is a big deal. The controller is a very thin shim. If we're to change this, let's change them all in a separate patch.

> WebKit/chromium/src/WebRuntimeFeatures.cpp:236
>  +  bool WebRuntimeFeatures::isDeviceOrientationEnabled()
> Interesting.  I was going to say the same here, but I realized that we don't
> want it to say true when support isn't even compiled in.  But then I realized I
> could say the same about what's in WebCore as well.  I almost wonder if it
> should default to false if the feature isn't compiled in.  But in that case, we
> should probably do the same for the other features.
I think that in general, we should use as few guards as possible. I think it's fine for the runtime feature to default to true, even if it's never used because the calling code is behind an enable guard.
Comment 14 WebKit Review Bot 2010-07-14 11:49:59 PDT
Attachment 61531 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3527153
Comment 15 WebKit Review Bot 2010-07-14 11:54:03 PDT
Attachment 61531 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3476308
Comment 16 Hans Wennborg 2010-07-15 04:31:13 PDT
(In reply to comment #11)
> (From update of attachment 61531 [details])
> WebCore/page/Page.cpp:143
>  +      , m_deviceOrientationController(RuntimeEnabledFeatures::deviceOrientationEnabled() ? new DeviceOrientationController(this, deviceOrientationClient) : 0)
> Should we be creating this lazily on first use?  I see that at least the geolocation one isn't, but maybe that should be changed too?  (It's a shame to slow down Page creation for features that may or may never be accessed.  Doing this lazily is generally the webkit way.)  It's ok to do this in another patch, but I think it should be done unless there's a good reason not to.
Let's aim for doing it in another patch, then.

> 
> WebKit/chromium/src/WebRuntimeFeatures.cpp:231
>  +  #if ENABLE(DEVICE_ORIENTATION)
> Do we need these guards?  It seems like not.
Right, let's remove it.

> 
> 
> WebKit/chromium/src/WebRuntimeFeatures.cpp:236
>  +  bool WebRuntimeFeatures::isDeviceOrientationEnabled()
> Interesting.  I was going to say the same here, but I realized that we don't want it to say true when support isn't even compiled in.  But then I realized I could say the same about what's in WebCore as well.  I almost wonder if it should default to false if the feature isn't compiled in.  But in that case, we should probably do the same for the other features.
> 
> What do you think?
I don't think it matters as much what the flag in WebCore is when the feature is not compiled in, as calling code in WebCore would be behind compile-time enable guard anyway.

In Chromium, we're going to flip the compile-time flag anyway (and expect it to be on, as some code will break without it), so we might as well drop the guards here as well. And I agree with Steve that the fever guards we need, the better.


Uploading new patch with changes to the makefiles to hopefully make the ews bots happy. We should watch this closely when it lands eventually to make sure nothing else breaks. Please chime in if I have missed any obvious build file.
Comment 17 Hans Wennborg 2010-07-15 04:31:43 PDT
Created attachment 61640 [details]
Patch
Comment 18 Steve Block 2010-07-15 04:35:39 PDT
Comment on attachment 61640 [details]
Patch

r=me

Not setting r+ yet as this apparently halts pending EWS bots
Comment 19 Jeremy Orlow 2010-07-15 04:36:54 PDT
Comment on attachment 61640 [details]
Patch

r=me

please create subsequent bugs and link to them before landing tho
Comment 20 Jeremy Orlow 2010-07-15 04:37:38 PDT
o...switched back from a r+ for ews bots
Comment 21 Early Warning System Bot 2010-07-15 04:40:35 PDT
Attachment 61640 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3567017
Comment 22 Steve Block 2010-07-15 04:52:58 PDT
Looks like you might need a prerequisite patch to add RuntimeEnabledFeatures.cpp to all the builds and to fix the guards!
Comment 23 WebKit Review Bot 2010-07-15 05:06:02 PDT
Attachment 61640 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3345726
Comment 24 WebKit Review Bot 2010-07-15 05:16:30 PDT
Attachment 61640 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3559029
Comment 25 Hans Wennborg 2010-07-20 06:37:23 PDT
Sorry for the delay.

Filed bug for discussion of lazily initializing Page members: Bug 42635

Since the patch from Bug 42380 has landed, the EWS bots should hopefully be happy with this patch now.
Comment 26 Hans Wennborg 2010-07-20 06:37:46 PDT
Created attachment 62066 [details]
Patch
Comment 27 Steve Block 2010-07-20 07:56:17 PDT
Comment on attachment 62066 [details]
Patch

r=me

Looks like the Windows EWS bot is dead, so go ahead and submit and watch the build bots.
Comment 28 WebKit Commit Bot 2010-07-20 10:08:41 PDT
Comment on attachment 62066 [details]
Patch

Clearing flags on attachment: 62066

Committed r63750: <http://trac.webkit.org/changeset/63750>
Comment 29 WebKit Commit Bot 2010-07-20 10:08:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Hans Wennborg 2010-07-20 11:18:51 PDT
Reopening. This had to be rolled out, as it broke the Windows build.

Will look into what happened tomorrow.
Comment 31 Hans Wennborg 2010-07-21 02:25:05 PDT
Uploading new patch that also adds bindings/generic to the WebCore.vcproj/WebCoreCommon.vsprops file.
Comment 32 Hans Wennborg 2010-07-21 02:25:38 PDT
Created attachment 62159 [details]
Patch
Comment 33 Steve Block 2010-07-21 03:11:39 PDT
Comment on attachment 62159 [details]
Patch

r=me
Comment 34 WebKit Commit Bot 2010-07-21 03:42:45 PDT
Comment on attachment 62159 [details]
Patch

Clearing flags on attachment: 62159

Committed r63810: <http://trac.webkit.org/changeset/63810>
Comment 35 WebKit Commit Bot 2010-07-21 03:42:52 PDT
All reviewed patches have been landed.  Closing bug.