Add feature to disable WebVR gesture requirement (issue 2543723002 by bsheedy@chromium.org)

[bsh...@chromium.org]

Reviewers: kinuko, bajones, bokan
CL: https://codereview.chromium.org/2543723002/

Message:
bajones@ for third_party/WebKit/Source/Modules/vr/ OWNER

bokan@ for third_party/WebKit/Source/web/WebRuntimeFeatures.cpp OWNER

kin...@chromium.org: for everything else

Description:
Add feature to disable WebVR gesture requirement

Adds a way to disable WebVR requiring a user gesture to present to allow
automated tests to present.

Feature can only be enabled via a the command line flag
"--disable-webvr-gestures-for-tests"
BUG=667520

Affected files (+22, -2 lines):
M content/browser/renderer_host/render_process_host_impl.cc
M content/child/runtime_features.cc
M content/public/common/content_switches.h
M content/public/common/content_switches.cc
M third_party/WebKit/Source/modules/vr/VRDisplay.cpp
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
M third_party/WebKit/Source/web/WebRuntimeFeatures.cpp
M third_party/WebKit/public/web/WebRuntimeFeatures.h


Index: content/browser/renderer_host/render_process_host_impl.cc
diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc
index 597e977a184e46bb5ce9597fa8a5e3160a2c6c61..cd510759b35a8c814b01a21a873294f0c784fa5e 100644
--- a/content/browser/renderer_host/render_process_host_impl.cc
+++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -1670,6 +1670,7 @@ void RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer(
switches::kDisableTouchDragDrop,
switches::kDisableV8IdleTasks,
switches::kDisableWebGLImageChromium,
+ switches::kDisableWebVrGesturesForTests,
switches::kDomAutomationController,
switches::kEnableBlinkFeatures,
switches::kEnableBrowserSideNavigation,
Index: content/child/runtime_features.cc
diff --git a/content/child/runtime_features.cc b/content/child/runtime_features.cc
index 956aacac7360a5bba9e66cbfd9d9707ef99aeb74..2c0e22fb0f3a42e1c18cf7bcdbfe0433285c9ca9 100644
--- a/content/child/runtime_features.cc
+++ b/content/child/runtime_features.cc
@@ -199,6 +199,9 @@ void SetRuntimeFeaturesDefaultsAndUpdateFromArgs(
if (command_line.HasSwitch(switches::kEnableWebVR))
WebRuntimeFeatures::enableWebVR(true);

+ if (command_line.HasSwitch(switches::kDisableWebVrGesturesForTests))
+ WebRuntimeFeatures::disableWebVrGestures(true);
+
if (command_line.HasSwitch(switches::kDisablePresentationAPI))
WebRuntimeFeatures::enablePresentationAPI(false);

Index: content/public/common/content_switches.cc
diff --git a/content/public/common/content_switches.cc b/content/public/common/content_switches.cc
index 1668c5a7816c9fd57bad58df9277e38d3a04fefc..ce33bef418b4b443795496a03a8a1055611c799b 100644
--- a/content/public/common/content_switches.cc
+++ b/content/public/common/content_switches.cc
@@ -304,6 +304,11 @@ const char kDisableWebGLImageChromium[] = "disable-webgl-image-chromium";
// Don't enforce the same-origin policy. (Used by people testing their sites.)
const char kDisableWebSecurity[] = "disable-web-security";

+// Disables WebVR's user gesture requirement in order to present so that
+// automated tests can be run in cases where we cannot mint gesture tokens
+const char kDisableWebVrGesturesForTests[] =
+ "disable-webvr-gestures-for-tests";
+
// Disables Blink's XSSAuditor. The XSSAuditor mitigates reflective XSS.
const char kDisableXSSAuditor[] = "disable-xss-auditor";

Index: content/public/common/content_switches.h
diff --git a/content/public/common/content_switches.h b/content/public/common/content_switches.h
index e721b9abdd9c058fbdb1fc5e9ed3812765316ff7..c5c937acc2872f4116dfc71844b88423e6385f07 100644
--- a/content/public/common/content_switches.h
+++ b/content/public/common/content_switches.h
@@ -101,6 +101,7 @@ CONTENT_EXPORT extern const char kDisableThreadedScrolling[];
extern const char kDisableV8IdleTasks[];
CONTENT_EXPORT extern const char kDisableWebGLImageChromium[];
CONTENT_EXPORT extern const char kDisableWebSecurity[];
+extern const char kDisableWebVrGesturesForTests[];
extern const char kDisableXSSAuditor[];
CONTENT_EXPORT extern const char kDisableZeroCopy[];
CONTENT_EXPORT extern const char kDisableZeroCopyDxgiVideo[];
Index: third_party/WebKit/Source/modules/vr/VRDisplay.cpp
diff --git a/third_party/WebKit/Source/modules/vr/VRDisplay.cpp b/third_party/WebKit/Source/modules/vr/VRDisplay.cpp
index a266f43d9b194bc884955d27c1c2187d10041e79..fdab47832ff4e63080185115ad6ffd7259786861 100644
--- a/third_party/WebKit/Source/modules/vr/VRDisplay.cpp
+++ b/third_party/WebKit/Source/modules/vr/VRDisplay.cpp
@@ -23,6 +23,7 @@
#include "modules/vr/VRStageParameters.h"
#include "modules/webgl/WebGLRenderingContextBase.h"
#include "platform/Histogram.h"
+#include "platform/RuntimeEnabledFeatures.h"
#include "platform/UserGestureIndicator.h"
#include "public/platform/Platform.h"
#include "wtf/AutoReset.h"
@@ -274,12 +275,15 @@ ScriptPromise VRDisplay::requestPresent(ScriptState* scriptState,
}

bool firstPresent = !m_isPresenting;
+ bool gesturesDisabled =
+ RuntimeEnabledFeatures::webVrGesturesDisabledEnabled();

// Initiating VR presentation is only allowed in response to a user gesture.
// If the VRDisplay is already presenting, however, repeated calls are
// allowed outside a user gesture so that the presented content may be
// updated.
- if (firstPresent && !UserGestureIndicator::utilizeUserGesture()) {
+ if (firstPresent && !UserGestureIndicator::utilizeUserGesture() &&
+ !gesturesDisabled) {
DOMException* exception = DOMException::create(
InvalidStateError, "API can only be initiated by a user gesture.");
resolver->reject(exception);
Index: third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
diff --git a/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in b/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
index a1a27b1ca3a268e5cd8b0c9b2d53072913fbbc50..762dfd4a7af74f820dd60a9131eb6bf05ae19c07 100644
--- a/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
+++ b/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
@@ -250,6 +250,7 @@ WebGLDraftExtensions status=experimental
WebGLImageChromium
WebUSB status=experimental, origin_trial_feature_name=WebUSB
WebVR origin_trial_feature_name=WebVR
+WebVrGesturesDisabled
WebVTTRegions status=experimental
V8BasedStructuredClone status=stable
V8IdleTasks
@@ -270,4 +271,4 @@ LazyParseCSS status=experimental
ParseHTMLOnMainThread status=test
SendBeaconThrowForBlobWithNonSimpleType status=experimental
PerformanceNavigationTiming2 status=test
-BackgroundVideoTrackOptimization status=stable
\ No newline at end of file
+BackgroundVideoTrackOptimization status=stable
Index: third_party/WebKit/Source/web/WebRuntimeFeatures.cpp
diff --git a/third_party/WebKit/Source/web/WebRuntimeFeatures.cpp b/third_party/WebKit/Source/web/WebRuntimeFeatures.cpp
index 97ecac1a41e3f562b5086696267049b4124e991f..dda594340560966b431b236f5a013a12ca5a7f94 100644
--- a/third_party/WebKit/Source/web/WebRuntimeFeatures.cpp
+++ b/third_party/WebKit/Source/web/WebRuntimeFeatures.cpp
@@ -296,6 +296,10 @@ void WebRuntimeFeatures::enableWebVR(bool enable) {
RuntimeEnabledFeatures::setWebVREnabled(enable);
}

+void WebRuntimeFeatures::disableWebVrGestures(bool disable) {
+ RuntimeEnabledFeatures::setWebVrGesturesDisabledEnabled(disable);
+}
+
void WebRuntimeFeatures::enablePresentationAPI(bool enable) {
RuntimeEnabledFeatures::setPresentationEnabled(enable);
}
Index: third_party/WebKit/public/web/WebRuntimeFeatures.h
diff --git a/third_party/WebKit/public/web/WebRuntimeFeatures.h b/third_party/WebKit/public/web/WebRuntimeFeatures.h
index 0eb3e5b336086bc922016546608f62ecc2aa6743..6ef71a95d73fa95037c95f2cb476ca911814825b 100644
--- a/third_party/WebKit/public/web/WebRuntimeFeatures.h
+++ b/third_party/WebKit/public/web/WebRuntimeFeatures.h
@@ -128,6 +128,7 @@ class WebRuntimeFeatures {
BLINK_EXPORT static void enableWebGLImageChromium(bool);
BLINK_EXPORT static void enableWebUsb(bool);
BLINK_EXPORT static void enableWebVR(bool);
+ BLINK_EXPORT static void disableWebVrGestures(bool);
BLINK_EXPORT static void enableXSLT(bool);
BLINK_EXPORT static void forceOverlayFullscreenVideo(bool);
BLINK_EXPORT static void enableAutoplayMutedVideos(bool);

[baj...@chromium.org]

modules/vr LGTM with a naming nit elsewhere.

https://codereview.chromium.org/2543723002/

[bo...@chromium.org]

Suggest using a better name, but lgtm otherwise


https://codereview.chromium.org/2543723002/diff/1/third_party/WebKit/Source/modules/vr/VRDisplay.cpp
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2543723002/diff/1/third_party/WebKit/Source/modules/vr/VRDisplay.cpp#newcode279
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:279:
RuntimeEnabledFeatures::webVrGesturesDisabledEnabled();
Perhaps a better name might be webVrWithoutUserGestures? Or flip the
meaning and webVrNeedsUserGestures? IMO having DisabledEnabled is
awkward.

https://codereview.chromium.org/2543723002/

[bsh...@chromium.org]

webVrGesturesDisabledEnabled -> webVrWithoutUserGesturesEnabled

https://codereview.chromium.org/2543723002/diff/1/third_party/WebKit/Source/modules/vr/VRDisplay.cpp
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2543723002/diff/1/third_party/WebKit/Source/modules/vr/VRDisplay.cpp#newcode279
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:279:
RuntimeEnabledFeatures::webVrGesturesDisabledEnabled();

On 2016/11/30 22:29:11, bokan wrote:
> Perhaps a better name might be webVrWithoutUserGestures? Or flip the
meaning and
> webVrNeedsUserGestures? IMO having DisabledEnabled is awkward.

Done.

https://codereview.chromium.org/2543723002/

[ddo...@chromium.org]

https://codereview.chromium.org/2543723002/diff/20001/content/public/common/content_switches.cc
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/2543723002/diff/20001/content/public/common/content_switches.cc#newcode310
content/public/common/content_switches.cc:310:
"disable-webvr-gestures-for-tests";
"webvr gestures" sounds like a feature. Should we follow existing
examples like disable-gesture-requirement-for-media-playback?
Something like disable-webvr-gesture-requirement-for-test.

Similar for the variable name.

https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
(right):

https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in#newcode253
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:253:
WebVrWithoutUserGestures
This seems like the wrong place to control this. These are mostly
features in development and will eventually be removed from here.

Most for-test values are probably checked in Chromium. The problem for
WebVR is that the gesture requirement is checked in Blink. Still, maybe
there is a better way to do this.

I'd be curious what Blink people have to say.

https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in#newcode253
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:253:
WebVrWithoutUserGestures
"Without" is still a negative. These are feature names, so they can be
independent of the switch name. (In the past, we toggled between
enabled/disabled in the switch names, but this remained the same.)
It seems that Requires/Needs would be better. Then we need to set it
true (maybe =stable would cause that if we keep it here) by default.

https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/public/web/WebRuntimeFeatures.h
File third_party/WebKit/public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/public/web/WebRuntimeFeatures.h#newcode131
third_party/WebKit/public/web/WebRuntimeFeatures.h:131: BLINK_EXPORT
static void disableWebVrGestures(bool);
Similarly, "disable" with a bool is odd and the rest are enable...

https://codereview.chromium.org/2543723002/

[har...@chromium.org]

https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
(right):

https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in#newcode253
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:253:
WebVrWithoutUserGestures

On 2016/11/30 23:33:34, ddorwin wrote:
> This seems like the wrong place to control this. These are mostly
features in
> development and will eventually be removed from here.
>
> Most for-test values are probably checked in Chromium. The problem for
WebVR is
> that the gesture requirement is checked in Blink. Still, maybe there
is a better
> way to do this.
>
> I'd be curious what Blink people have to say.

If you need to control a runtime-enabled feature in Blink, this is a
right place to do that.

https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/public/web/WebRuntimeFeatures.h
File third_party/WebKit/public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/public/web/WebRuntimeFeatures.h#newcode131
third_party/WebKit/public/web/WebRuntimeFeatures.h:131: BLINK_EXPORT
static void disableWebVrGestures(bool);

On 2016/11/30 23:33:34, ddorwin wrote:
> Similarly, "disable" with a bool is odd and the rest are enable...

Yeah, everything else is "enable". So let's use "enable" here as well.

https://codereview.chromium.org/2543723002/

[bsh...@chromium.org]

https://codereview.chromium.org/2543723002/diff/20001/content/public/common/content_switches.cc
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/2543723002/diff/20001/content/public/common/content_switches.cc#newcode310
content/public/common/content_switches.cc:310:
"disable-webvr-gestures-for-tests";

On 2016/11/30 23:33:34, ddorwin wrote:

> "webvr gestures" sounds like a feature. Should we follow existing
examples like
> disable-gesture-requirement-for-media-playback?
> Something like disable-webvr-gesture-requirement-for-test.
>
> Similar for the variable name.

Done.

https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
(right):

https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in#newcode253
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:253:
WebVrWithoutUserGestures

On 2016/11/30 23:41:28, haraken wrote:
> On 2016/11/30 23:33:34, ddorwin wrote:
> > This seems like the wrong place to control this. These are mostly
features in
> > development and will eventually be removed from here.
> >
> > Most for-test values are probably checked in Chromium. The problem
for WebVR
> is
> > that the gesture requirement is checked in Blink. Still, maybe there
is a
> better
> > way to do this.
> >
> > I'd be curious what Blink people have to say.
>
> If you need to control a runtime-enabled feature in Blink, this is a
right place
> to do that.

Done.

https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/public/web/WebRuntimeFeatures.h
File third_party/WebKit/public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/public/web/WebRuntimeFeatures.h#newcode131
third_party/WebKit/public/web/WebRuntimeFeatures.h:131: BLINK_EXPORT
static void disableWebVrGestures(bool);

On 2016/11/30 23:41:28, haraken wrote:
> On 2016/11/30 23:33:34, ddorwin wrote:
> > Similarly, "disable" with a bool is odd and the rest are enable...
>
> Yeah, everything else is "enable". So let's use "enable" here as well.

Done.

https://codereview.chromium.org/2543723002/

[ddo...@chromium.org]

LGTM with naming suggestions. Thanks.


https://codereview.chromium.org/2543723002/diff/40001/third_party/WebKit/Source/modules/vr/VRDisplay.cpp
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2543723002/diff/40001/third_party/WebKit/Source/modules/vr/VRDisplay.cpp#newcode278
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:278: bool
gesturesEnabled =
gestureRequired would be better.

https://codereview.chromium.org/2543723002/diff/40001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
(right):

https://codereview.chromium.org/2543723002/diff/40001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in#newcode253
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:253:
WebVrRequiresUserGestures status=stable
nit: not plural, right?

https://codereview.chromium.org/2543723002/

[bsh...@chromium.org]

https://codereview.chromium.org/2543723002/diff/40001/third_party/WebKit/Source/modules/vr/VRDisplay.cpp
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2543723002/diff/40001/third_party/WebKit/Source/modules/vr/VRDisplay.cpp#newcode278
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:278: bool
gesturesEnabled =

On 2016/12/01 00:03:10, ddorwin wrote:
> gestureRequired would be better.

Done.

https://codereview.chromium.org/2543723002/diff/40001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
(right):

https://codereview.chromium.org/2543723002/diff/40001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in#newcode253
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:253:
WebVrRequiresUserGestures status=stable

On 2016/12/01 00:03:10, ddorwin wrote:
> nit: not plural, right?

Done.

https://codereview.chromium.org/2543723002/

[har...@chromium.org]

LGTM


https://codereview.chromium.org/2543723002/

[har...@chromium.org]

https://codereview.chromium.org/2543723002/diff/60001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
(right):

https://codereview.chromium.org/2543723002/diff/60001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in#newcode253
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:253:
WebVrRequiresUserGesture status=stable

Oh, you need to add "status=stable" to make your CL work... Hmm.

Feature flags with "status=stable" are expected to be removed once the
features are fully stabilized. In other words, we shouldn't keep the
feature flags just for testing purposes.

Maybe we should ask API owners how to handle this situation.

+Rick

https://codereview.chromium.org/2543723002/

[kin...@chromium.org]

https://codereview.chromium.org/2543723002/diff/60001/content/public/common/content_switches.cc
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/2543723002/diff/60001/content/public/common/content_switches.cc#newcode308
content/public/common/content_switches.cc:308: // automated tests can be

run in cases where we cannot mint gesture tokens

nit: finish the sentence with a period

https://codereview.chromium.org/2543723002/diff/60001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
(right):

https://codereview.chromium.org/2543723002/diff/60001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in#newcode253
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:253:
WebVrRequiresUserGesture status=stable

If the flag is not about feature stability but for testing could we use
an inverted flag name (e.g. WebVrAllowedWithoutUserGesture) with no
status values to make the intention / expected default behavior clearer?
Would be also nice to have a short comment to note that the flag is for
testing purpose.

https://codereview.chromium.org/2543723002/

[bo...@chromium.org]

On 2016/12/01 01:19:17, haraken wrote:
>
https://codereview.chromium.org/2543723002/diff/60001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
> File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):
>
>
https://codereview.chromium.org/2543723002/diff/60001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in#newcode253
> third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:253:
> WebVrRequiresUserGesture status=stable
>

> Oh, you need to add "status=stable" to make your CL work... Hmm.
>
> Feature flags with "status=stable" are expected to be removed once the
features
> are fully stabilized. In other words, we shouldn't keep the feature flags just
> for testing purposes.
>
> Maybe we should ask API owners how to handle this situation.
>
> +Rick

Maybe Settings.in/WebSettings is a more appropriate place for this? We've
already got testing related switches in there for things like
mockGestureTapHighlightsEnabled, mockScrollbarsEnabled, and
threadedScrollingEnabled. Though there are ancient FIXMEs littered throughout
that these don't belong in here, I'm not sure they still apply. I don't think we
have a good, set place for switches needed for testing so they're sort of
scattered throughout REF, Settings and elsewhere. We should either clarify they
belong in Settings or create an appropriate place for them.

https://codereview.chromium.org/2543723002/

[bsh...@chromium.org]

On 2016/12/01 02:22:03, kinuko wrote:
>
https://codereview.chromium.org/2543723002/diff/60001/content/public/common/content_switches.cc
> File content/public/common/content_switches.cc (right):
>
>
https://codereview.chromium.org/2543723002/diff/60001/content/public/common/content_switches.cc#newcode308
> content/public/common/content_switches.cc:308: // automated tests can be run
in
> cases where we cannot mint gesture tokens
> nit: finish the sentence with a period
>
>

https://codereview.chromium.org/2543723002/diff/60001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
> File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):
>
>
https://codereview.chromium.org/2543723002/diff/60001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in#newcode253
> third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:253:
> WebVrRequiresUserGesture status=stable

> If the flag is not about feature stability but for testing could we use an
> inverted flag name (e.g. WebVrAllowedWithoutUserGesture) with no status values
> to make the intention / expected default behavior clearer? Would be also nice
> to have a short comment to note that the flag is for testing purpose.

That's what we were doing earlier, but changed for naming purposes.

What about bokan@'s suggestion of putting them in Settings.in or similar?

https://codereview.chromium.org/2543723002/

[har...@chromium.org]

+1 to bokan's suggestion. I want to keep RuntimeEnabledFeatures only for
web-exposed API things.



https://codereview.chromium.org/2543723002/

[kin...@chromium.org]

One of the criteria we often use to put Settings.in or REF is if it needs to
change during the lifetime of renderer process, in that sense REF seems to make
sense. That's said it's also true that REF is designed more for feature-related
things and I'm fine with us having this in Settings.in.

In either way it'd be definitely good to have a comment to note about why we
have the flag there.

https://codereview.chromium.org/2543723002/

[rby...@chromium.org]

I don't have a strong objection to this, but I am curious why it's really
necessary. LOTS of web platform features require a user gesture (fullscreen,
trusted input events, permissions, etc etc....). AFAIK we don't have a setting
to disable that for any of them. Instead all our (internal and 3rd party)
automated testing systems have ways of simulating input, including the ability
to generate a user gesture (i.e. webdriver uses the chrome remote debug protocol
synthetic input APIs, LayoutTests use the eventSender API, browser tests
directly inject fake input events). What makes WebVR different from all these
cases?

https://codereview.chromium.org/2543723002/

[bsh...@chromium.org]

This might be possible for most things in the long term (there are currently a
number of issues with trying to use layout tests for WebVR testing that are
either currently being worked on or will certainly be worked on in the future).
However, there is at least one case where I think this functionality is
necessary.

The Daydream team has agreed to run some WebVR tests on their internal test
setup (since the native WebVR implementation uses the Daydream/GVR NDK), which
largely involves opening up an app, letting it run for several hours, and
recording various metrics. They're planning on just using Chrome Canary for the
tests, and I doubt they'd be willing to take the time to try to get everything
working with something like browser tests. Thus, they need some way to
automatically presenting in a release APK, which this provides.

https://codereview.chromium.org/2543723002/

[dch...@chromium.org]

Isn't webdriver the preferred integration point in that case?

https://codereview.chromium.org/2543723002/

[rby...@chromium.org]

Yeah in practice most people testing Chrome from the outside like this need SOME
way to automate chrome anyway (navigate to pages, click on links, etc.) and in
practice people use Selenium (WebDriver) which is implemented on the Chrome
DevTools remote debugging protocol.

But if, for some reason, Daydream tests don't already need to automate the
browser and so aren't already using WebDriver (or something like it), then
adding a flag like this to make things easier for them is fine with me.

https://codereview.chromium.org/2543723002/

[dch...@chromium.org]

I'd rather not add more config options. Each option has an incremental and
continual maintenance cost. I don't see how adding this flag will significantly
reduce the setup cost for the internal tester (which still needs to start Chrome
Canary, load up some page, and then capture metrics).

https://codereview.chromium.org/2543723002/

[bsh...@chromium.org]

On 2016/12/02 18:12:04, dcheng wrote:
> I'd rather not add more config options. Each option has an incremental and
> continual maintenance cost. I don't see how adding this flag will
significantly
> reduce the setup cost for the internal tester (which still needs to start
Chrome
> Canary, load up some page, and then capture metrics).

I've talked with the Daydream team, and they're going to try out ChromeDriver to
see how easy it would be to include it in their testing setup. If they find it's
simple enough, they're fine with using it, but otherwise would like the flag
added.

I also found that this flag was requested externally a couple months ago. See
https://mail.mozilla.org/pipermail/web-vr-discuss/2016-September/001636.html.

https://codereview.chromium.org/2543723002/

[lei...@chromium.org]

I am still thinking flag is better than Chromedriver even daydream team is
willing to use Chromedriver. Since Daydream team already use 'adb' command to
run tests for other VR apps, it will be nice to use same way for Chrome VR, so
they don't need extra work to support Chrome VR. And this flag will be also
helpful for our developers, they can use one 'adb' command to launch chrome,
open url and run VR websites for a while to collect metrics.
adb shell am start \
-a android.intent.action.VIEW \
-n com.chrome.canary/com.google.android.apps.chrome.Main \
--es activeUrl "https://webvr.info/samples/03-vr-presentation.html" \
--esa commandLineArgs --disable-gesture-requirement-for-webvr

Since we already have several flags to disable gesture check for different
features/APIs.
1.
disable-gesture-requirement-for-media-playback(https://cs.chromium.org/chromium/src/content/public/common/content_switches.cc?q=disable-gesture-requirement-for-media-playback&sq=package:chromium&dr=C&l=137)
2.
disable-gesture-requirement-for-presentation(https://cs.chromium.org/chromium/src/content/public/common/content_switches.cc?q=disable-gesture-requirement-for-media-playback&sq=package:chromium&dr=C&l=141)
We just need to add one more for Chrome VR.

Does it make sense？WDYT?


https://codereview.chromium.org/2543723002/

[bsh...@chromium.org]

I heard back from one of the Daydream members, and they still would prefer the
command line flag:

"I think going to generic solution will be better. Put it this way, we are
building the same automation that we plan to use for our device certification
too. (E.g. Ensure device performs well with 2.5 hours of YouTube or chrome use).
So, if we have to rely on a private driver, it adds complexity to our possible
external release."

https://codereview.chromium.org/2543723002/

[dch...@chromium.org]

"Just one more flag" isn't sustainable, as it doesn't scale to have so many
flags just to toggle individual behaviors off for tests. It seems like both of
these shouldn't have been checked in either (the media playback one is a bit
trickier), and we should actually be trying to remove them (I'll file bugs on
Monday).

While I agree that this might be simpler for Daydream, it's not just about
Daydream; Chrome has a lot of other things in it, and the list of flags is
already extremely long:
http://peter.sh/experiments/chromium-command-line-switches/

If web driver doesn't work for some other reason, then a flag might be
reasonable. But we should at least try it first.

https://codereview.chromium.org/2543723002/

[bsh...@chromium.org]

We've found a solution that works for the Daydream team without ChromeDriver or
a command line flag, so this isn't necessary for our testing anymore. I still
think it's worth adding this though since external developers have requested a
way to bypass the gesture requirement.

https://codereview.chromium.org/2543723002/