Runtime feature switch for device orientation
Created attachment 61526 [details] Patch
Attachment 61526 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3430277
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 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
(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.
(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.
(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?
Created attachment 61531 [details] Patch
Attachment 61531 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3490286
Darin should take a look as well.
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 on attachment 61531 [details] Patch lgtm, other than the qt build problem
> 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.
Attachment 61531 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3527153
Attachment 61531 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3476308
(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.
Created attachment 61640 [details] Patch
Comment on attachment 61640 [details] Patch r=me Not setting r+ yet as this apparently halts pending EWS bots
Comment on attachment 61640 [details] Patch r=me please create subsequent bugs and link to them before landing tho
o...switched back from a r+ for ews bots
Attachment 61640 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3567017
Looks like you might need a prerequisite patch to add RuntimeEnabledFeatures.cpp to all the builds and to fix the guards!
Attachment 61640 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3345726
Attachment 61640 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3559029
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.
Created attachment 62066 [details] Patch
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 on attachment 62066 [details] Patch Clearing flags on attachment: 62066 Committed r63750: <http://trac.webkit.org/changeset/63750>
All reviewed patches have been landed. Closing bug.
Reopening. This had to be rolled out, as it broke the Windows build. Will look into what happened tomorrow.
Uploading new patch that also adds bindings/generic to the WebCore.vcproj/WebCoreCommon.vsprops file.
Created attachment 62159 [details] Patch
Comment on attachment 62159 [details] Patch r=me
Comment on attachment 62159 [details] Patch Clearing flags on attachment: 62159 Committed r63810: <http://trac.webkit.org/changeset/63810>