Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2543723002:
Add switch to disable WebVR gesture requirement
|
Created:
4 years ago by bsheedy Modified:
3 years, 10 months ago CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, dglazkov+blink, feature-vr-reviews_chromium.org, darin-cc_chromium.org, blink-reviews, kinuko+watch, blink-reviews-api_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd switch 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
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixed DisabledEnabled naming #
Total comments: 9
Patch Set 3 : Addressed naming comments #
Total comments: 4
Patch Set 4 : Addressed ddorwin@'s naming comments #
Total comments: 3
Messages
Total messages: 32 (6 generated)
bsheedy@chromium.org changed reviewers: + bajones@chromium.org, bokan@chromium.org, kinuko@chromium.org
bajones@ for third_party/WebKit/Source/Modules/vr/ OWNER bokan@ for third_party/WebKit/Source/web/WebRuntimeFeatures.cpp OWNER kinuko@chromium.org: for everything else
modules/vr LGTM with a naming nit elsewhere.
Suggest using a better name, but lgtm otherwise https://codereview.chromium.org/2543723002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2543723002/diff/1/third_party/WebKit/Source/m... 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.
webVrGesturesDisabledEnabled -> webVrWithoutUserGesturesEnabled https://codereview.chromium.org/2543723002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2543723002/diff/1/third_party/WebKit/Source/m... 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.
ddorwin@chromium.org changed reviewers: + ddorwin@chromium.org
https://codereview.chromium.org/2543723002/diff/20001/content/public/common/c... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2543723002/diff/20001/content/public/common/c... 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/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/Sour... 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/Sour... 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/publ... File third_party/WebKit/public/web/WebRuntimeFeatures.h (right): https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/publ... 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...
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/Sour... 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/publ... File third_party/WebKit/public/web/WebRuntimeFeatures.h (right): https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/publ... 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/diff/20001/content/public/common/c... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2543723002/diff/20001/content/public/common/c... 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/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/Sour... 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/publ... File third_party/WebKit/public/web/WebRuntimeFeatures.h (right): https://codereview.chromium.org/2543723002/diff/20001/third_party/WebKit/publ... 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.
LGTM with naming suggestions. Thanks. https://codereview.chromium.org/2543723002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2543723002/diff/40001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2543723002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:253: WebVrRequiresUserGestures status=stable nit: not plural, right?
Description was changed from ========== 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 ========== to ========== Add switch 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 ==========
https://codereview.chromium.org/2543723002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2543723002/diff/40001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2543723002/diff/40001/third_party/WebKit/Sour... 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.
LGTM
haraken@chromium.org changed reviewers: + rbyers@chromium.org
https://codereview.chromium.org/2543723002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2543723002/diff/60001/third_party/WebKit/Sour... 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/diff/60001/content/public/common/c... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2543723002/diff/60001/content/public/common/c... 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/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2543723002/diff/60001/third_party/WebKit/Sour... 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.
On 2016/12/01 01:19:17, haraken wrote: > https://codereview.chromium.org/2543723002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): > > https://codereview.chromium.org/2543723002/diff/60001/third_party/WebKit/Sour... > 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.
On 2016/12/01 02:22:03, kinuko wrote: > https://codereview.chromium.org/2543723002/diff/60001/content/public/common/c... > File content/public/common/content_switches.cc (right): > > https://codereview.chromium.org/2543723002/diff/60001/content/public/common/c... > 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/Sour... > File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): > > https://codereview.chromium.org/2543723002/diff/60001/third_party/WebKit/Sour... > 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?
On 2016/12/01 17:21:17, bsheedy wrote: > On 2016/12/01 02:22:03, kinuko wrote: > > > https://codereview.chromium.org/2543723002/diff/60001/content/public/common/c... > > File content/public/common/content_switches.cc (right): > > > > > https://codereview.chromium.org/2543723002/diff/60001/content/public/common/c... > > 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/Sour... > > File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): > > > > > https://codereview.chromium.org/2543723002/diff/60001/third_party/WebKit/Sour... > > 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? +1 to bokan's suggestion. I want to keep RuntimeEnabledFeatures only for web-exposed API things.
On 2016/12/01 23:44:35, haraken wrote: > On 2016/12/01 17:21:17, bsheedy wrote: > > On 2016/12/01 02:22:03, kinuko wrote: > > > > > > https://codereview.chromium.org/2543723002/diff/60001/content/public/common/c... > > > File content/public/common/content_switches.cc (right): > > > > > > > > > https://codereview.chromium.org/2543723002/diff/60001/content/public/common/c... > > > 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/Sour... > > > File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): > > > > > > > > > https://codereview.chromium.org/2543723002/diff/60001/third_party/WebKit/Sour... > > > 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? > > +1 to bokan's suggestion. I want to keep RuntimeEnabledFeatures only for > web-exposed API things. 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.
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?
On 2016/12/02 01:23:25, Rick Byers wrote: > 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? 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.
On 2016/12/02 01:53:34, bsheedy wrote: > On 2016/12/02 01:23:25, Rick Byers wrote: > > 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? > > 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. Isn't webdriver the preferred integration point in that case?
On 2016/12/02 02:28:03, dcheng wrote: > On 2016/12/02 01:53:34, bsheedy wrote: > > On 2016/12/02 01:23:25, Rick Byers wrote: > > > 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? > > > > 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. > > Isn't webdriver the preferred integration point in that case? 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.
On 2016/12/02 16:53:38, Rick Byers wrote: > On 2016/12/02 02:28:03, dcheng wrote: > > On 2016/12/02 01:53:34, bsheedy wrote: > > > On 2016/12/02 01:23:25, Rick Byers wrote: > > > > 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? > > > > > > 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. > > > > Isn't webdriver the preferred integration point in that case? > > 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. 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).
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.
On 2016/12/02 22:31:00, bsheedy wrote: > 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. 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?
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."
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
On 2016/12/02 22:52:52, Lei Lei wrote: > On 2016/12/02 22:31:00, bsheedy wrote: > > 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. > > 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? "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). On 2016/12/03 01:39:58, bsheedy wrote: > 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." 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.
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. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
