Bug 42265 – Runtime feature switch for device orientation

• Component: New Bugs
• Status: RESOLVED FIXED

[Hans Wennborg]

Runtime feature switch for device orientation

[Hans Wennborg]

Created attachment 61526 [details]
Patch

[Early Warning System Bot]

Attachment 61526 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3430277

[Jeremy Orlow]

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.

[Steve Block]

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

[Hans Wennborg]

(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.

[Hans Wennborg]

(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.

[Hans Wennborg]

(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?

[Hans Wennborg]

Created attachment 61531 [details]
Patch

[Early Warning System Bot]

Attachment 61531 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3490286

[Jeremy Orlow]

Darin should take a look as well.

[Jeremy Orlow]

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?

[Steve Block]

Comment on attachment 61531 [details]
Patch

lgtm, other than the qt build problem

[Steve Block]

> 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.

[WebKit Review Bot]

Attachment 61531 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3527153

[WebKit Review Bot]

Attachment 61531 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3476308

[Hans Wennborg]

(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.

[Hans Wennborg]

Created attachment 61640 [details]
Patch

[Steve Block]

Comment on attachment 61640 [details]
Patch

r=me

Not setting r+ yet as this apparently halts pending EWS bots

[Jeremy Orlow]

Comment on attachment 61640 [details]
Patch

r=me

please create subsequent bugs and link to them before landing tho

[Jeremy Orlow]

o...switched back from a r+ for ews bots

[Early Warning System Bot]

Attachment 61640 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3567017

[Steve Block]

Looks like you might need a prerequisite patch to add RuntimeEnabledFeatures.cpp to all the builds and to fix the guards!

[WebKit Review Bot]

Attachment 61640 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3345726

[WebKit Review Bot]

Attachment 61640 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3559029

[Hans Wennborg]

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.

[Hans Wennborg]

Created attachment 62066 [details]
Patch

[Steve Block]

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.

[WebKit Commit Bot]

Comment on attachment 62066 [details]
Patch

Clearing flags on attachment: 62066

Committed r63750: <http://trac.webkit.org/changeset/63750>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Hans Wennborg]

Reopening. This had to be rolled out, as it broke the Windows build.

Will look into what happened tomorrow.

[Hans Wennborg]

Uploading new patch that also adds bindings/generic to the WebCore.vcproj/WebCoreCommon.vsprops file.

[Hans Wennborg]

Created attachment 62159 [details]
Patch

[Steve Block]

Comment on attachment 62159 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 62159 [details]
Patch

Clearing flags on attachment: 62159

Committed r63810: <http://trac.webkit.org/changeset/63810>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.