Bug 42265 - Runtime feature switch for device orientation
: Runtime feature switch for device orientation
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
: 42380 42648
: 41616
  Show dependency treegraph
 
Reported: 2010-07-14 08:31 PST by
Modified: 2010-07-21 03:42 PST (History)


Attachments
Patch (8.28 KB, patch)
2010-07-14 08:37 PST, Hans Wennborg
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.00 KB, patch)
2010-07-14 10:14 PST, Hans Wennborg
no flags Review Patch | Details | Formatted Diff | Diff
Patch (11.29 KB, patch)
2010-07-15 04:31 PST, Hans Wennborg
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.00 KB, patch)
2010-07-20 06:37 PST, Hans Wennborg
no flags Review Patch | Details | Formatted Diff | Diff
Patch (15.12 KB, patch)
2010-07-21 02:25 PST, Hans Wennborg
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-07-14 08:31:03 PST
Runtime feature switch for device orientation
------- Comment #1 From 2010-07-14 08:37:54 PST -------
Created an attachment (id=61526) [details]
Patch
------- Comment #2 From 2010-07-14 08:52:17 PST -------
Attachment 61526 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3430277
------- Comment #3 From 2010-07-14 08:55:09 PST -------
(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.


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 From 2010-07-14 09:03:16 PST -------
(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.

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 From 2010-07-14 10:11:34 PST -------
(In reply to comment #3)
> (From update of attachment 61526 [details] [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 From 2010-07-14 10:12:05 PST -------
(In reply to comment #4)
> (From update of attachment 61526 [details] [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 From 2010-07-14 10:13:49 PST -------
(In reply to comment #2)
> Attachment 61526 [details] [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 From 2010-07-14 10:14:40 PST -------
Created an attachment (id=61531) [details]
Patch
------- Comment #9 From 2010-07-14 10:25:55 PST -------
Attachment 61531 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3490286
------- Comment #10 From 2010-07-14 11:34:23 PST -------
Darin should take a look as well.
------- Comment #11 From 2010-07-14 11:34:47 PST -------
(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.

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 From 2010-07-14 11:43:32 PST -------
(From update of attachment 61531 [details])
lgtm, other than the qt build problem
------- Comment #13 From 2010-07-14 11:46:15 PST -------
> 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 From 2010-07-14 11:49:59 PST -------
Attachment 61531 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3527153
------- Comment #15 From 2010-07-14 11:54:03 PST -------
Attachment 61531 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3476308
------- Comment #16 From 2010-07-15 04:31:13 PST -------
(In reply to comment #11)
> (From update of attachment 61531 [details] [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 From 2010-07-15 04:31:43 PST -------
Created an attachment (id=61640) [details]
Patch
------- Comment #18 From 2010-07-15 04:35:39 PST -------
(From update of attachment 61640 [details])
r=me

Not setting r+ yet as this apparently halts pending EWS bots
------- Comment #19 From 2010-07-15 04:36:54 PST -------
(From update of attachment 61640 [details])
r=me

please create subsequent bugs and link to them before landing tho
------- Comment #20 From 2010-07-15 04:37:38 PST -------
o...switched back from a r+ for ews bots
------- Comment #21 From 2010-07-15 04:40:35 PST -------
Attachment 61640 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3567017
------- Comment #22 From 2010-07-15 04:52:58 PST -------
Looks like you might need a prerequisite patch to add RuntimeEnabledFeatures.cpp to all the builds and to fix the guards!
------- Comment #23 From 2010-07-15 05:06:02 PST -------
Attachment 61640 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3345726
------- Comment #24 From 2010-07-15 05:16:30 PST -------
Attachment 61640 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3559029
------- Comment #25 From 2010-07-20 06:37:23 PST -------
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 From 2010-07-20 06:37:46 PST -------
Created an attachment (id=62066) [details]
Patch
------- Comment #27 From 2010-07-20 07:56:17 PST -------
(From update of attachment 62066 [details])
r=me

Looks like the Windows EWS bot is dead, so go ahead and submit and watch the build bots.
------- Comment #28 From 2010-07-20 10:08:41 PST -------
(From update of attachment 62066 [details])
Clearing flags on attachment: 62066

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

Will look into what happened tomorrow.
------- Comment #31 From 2010-07-21 02:25:05 PST -------
Uploading new patch that also adds bindings/generic to the WebCore.vcproj/WebCoreCommon.vsprops file.
------- Comment #32 From 2010-07-21 02:25:38 PST -------
Created an attachment (id=62159) [details]
Patch
------- Comment #33 From 2010-07-21 03:11:39 PST -------
(From update of attachment 62159 [details])
r=me
------- Comment #34 From 2010-07-21 03:42:45 PST -------
(From update of attachment 62159 [details])
Clearing flags on attachment: 62159

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