Allow pages to set custom options.

Allow pages to set custom options.


BUG=catapult:#3863

Review-Url: https://chromiumcodereview.appspot.com/3011293002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/4ee9a49ff94565eeece6078540df8c11a347751d

[CalebRouleau, 2017-09-15 00:45:52]

Description was changed from

==========
Allow pages to set custom options.

This code works for me, but I'm wondering if it will break anyone else.
In particular, I'm wondering if other teams use different
SharedPageState classes and for them this method will have no effect.

BUG=catapult:#
==========

to

==========
Allow pages to set custom options.

This code works for me, but I'm wondering if it will break anyone else.
In particular, I'm wondering if other teams use different
SharedPageState classes and for them this method will have no effect.

BUG=catapult:#3863
==========

[CalebRouleau, 2017-09-15 00:45:54]

crouleau@chromium.org changed reviewers:
+ dmazzoni@chromium.org, nednguyen@google.com

[nednguyen, 2017-09-15 00:52:01]

On 2017/09/15 00:46:08, CalebRouleau wrote:

The only gotcha here is we add yet-another-place to configure browser options.
So with this, browser options can now be configured through:
1) Benchmark commandline option
2) Benchmark code (through Benchmark.CustomizeBrowserOptions method)
3) Telemetry's page.

Probably this is unavoidable, but I wonder if we can make sure that the test
will fail hard if there are conflicting options. Does any of you know whether
Chrome will crash upon startup if the command line flag options are conflicting?

[CalebRouleau, 2017-09-15 17:49:24]

On 2017/09/15 00:52:01, nednguyen wrote:
> On 2017/09/15 00:46:08, CalebRouleau wrote:
> 
> The only gotcha here is we add yet-another-place to configure browser options.
> So with this, browser options can now be configured through:
> 1) Benchmark commandline option
> 2) Benchmark code (through Benchmark.CustomizeBrowserOptions method)
> 3) Telemetry's page.
> 
> Probably this is unavoidable, but I wonder if we can make sure that the test
> will fail hard if there are conflicting options. Does any of you know whether
> Chrome will crash upon startup if the command line flag options are
conflicting?

Chrome will not crash at startup for that. I tried setting both
"--enable-media-suspend" and "--disable-media-suspend" and Chrome seemed to
ignore the second flag.

I could make AppendExtraBrowserArgs check for conflicts. The conflicts I can
think of are --enableX conflicts with --disableX. Are there any others?

[nednguyen, 2017-09-15 18:09:36]

On 2017/09/15 17:49:24, CalebRouleau wrote:
> On 2017/09/15 00:52:01, nednguyen wrote:
> > On 2017/09/15 00:46:08, CalebRouleau wrote:
> > 
> > The only gotcha here is we add yet-another-place to configure browser
options.
> > So with this, browser options can now be configured through:
> > 1) Benchmark commandline option
> > 2) Benchmark code (through Benchmark.CustomizeBrowserOptions method)
> > 3) Telemetry's page.
> > 
> > Probably this is unavoidable, but I wonder if we can make sure that the test
> > will fail hard if there are conflicting options. Does any of you know
whether
> > Chrome will crash upon startup if the command line flag options are
> conflicting?
> 
> Chrome will not crash at startup for that. I tried setting both
> "--enable-media-suspend" and "--disable-media-suspend" and Chrome seemed to
> ignore the second flag.
> 
> I could make AppendExtraBrowserArgs check for conflicts.

This seems like a good idea. Though I would suggest check for conflict right
after GetBrowserStartupArgs() is invoked so we can be absolutely sure
(https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...).
I think we can achieve this with:

1) Add GetValidatedBrowserStartupArgs method that call GetBrowserStartupArgs() &
then call validation code
2) Update callsites of GetBrowserStartupArgs() to use
GetValidatedBrowserStartupArgs() instead.

> The conflicts I can
> think of are --enableX conflicts with --disableX. Are there any others?

Other one would be specifying a same flag multiple times, e.g: ./<path to chrome
binary> --window-size==1280,1024 --window-size==1080,960

[CalebRouleau, 2017-09-21 22:14:26]

On 2017/09/15 18:09:36, nednguyen wrote:
> On 2017/09/15 17:49:24, CalebRouleau wrote:
> > On 2017/09/15 00:52:01, nednguyen wrote:
> > > On 2017/09/15 00:46:08, CalebRouleau wrote:
> > > 
> > > The only gotcha here is we add yet-another-place to configure browser
> options.
> > > So with this, browser options can now be configured through:
> > > 1) Benchmark commandline option
> > > 2) Benchmark code (through Benchmark.CustomizeBrowserOptions method)
> > > 3) Telemetry's page.
> > > 
> > > Probably this is unavoidable, but I wonder if we can make sure that the
test
> > > will fail hard if there are conflicting options. Does any of you know
> whether
> > > Chrome will crash upon startup if the command line flag options are
> > conflicting?
> > 
> > Chrome will not crash at startup for that. I tried setting both
> > "--enable-media-suspend" and "--disable-media-suspend" and Chrome seemed to
> > ignore the second flag.
> > 
> > I could make AppendExtraBrowserArgs check for conflicts.
> 
> This seems like a good idea. Though I would suggest check for conflict right
> after GetBrowserStartupArgs() is invoked so we can be absolutely sure
>
(https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...).
> I think we can achieve this with:
> 
> 1) Add GetValidatedBrowserStartupArgs method that call GetBrowserStartupArgs()
&
> then call validation code
> 2) Update callsites of GetBrowserStartupArgs() to use
> GetValidatedBrowserStartupArgs() instead.
> 
> > The conflicts I can
> > think of are --enableX conflicts with --disableX. Are there any others?
> 
> Other one would be specifying a same flag multiple times, e.g: ./<path to
chrome
> binary> --window-size==1280,1024 --window-size==1080,960

I wrote
https://docs.google.com/document/d/1I33_j_9y7D0mzAUUuRoFyJS2JVZX02wCoBeNkGRxw...
, and I will follow through with it soon. In the mean time, we decided offline
that we can move forward with this change and do the validation later on.

[CalebRouleau, 2017-09-21 22:15:44]

crouleau@chromium.org changed reviewers:
+ perezju@chromium.org

[nednguyen, 2017-09-21 22:45:18]

Chatted offline, let just add extra_browser_args as an arg to Page's constructor
for now. 

The rationale is to make the API less powerful, thus blocking people from
writing complex logic for setting extra browser args. In the future, if we find
this is too restricted, it should be easy to extend the API to be more powerful
(whereas the reverse is harder)

[dmazzoni, 2017-09-21 22:46:06]

lgtm

verified this works for me

[CalebRouleau, 2017-09-21 23:12:39]

On 2017/09/21 22:46:06, dmazzoni wrote:
> lgtm
> 
> verified this works for me

I made more changes though based on Ned's suggestions. Please check again.

[CalebRouleau, 2017-09-21 23:12:40]

On 2017/09/21 22:46:06, dmazzoni wrote:
> lgtm
> 
> verified this works for me

I made more changes though based on Ned's suggestions. Please check again.

[nednguyen, 2017-09-21 23:14:08]

lgtm

[CalebRouleau, 2017-09-21 23:24:04]

The CQ bit was checked by crouleau@chromium.org

[CalebRouleau, 2017-09-21 23:24:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/3011293002/#ps40001
(title: "Add as argument to constructor instead.")

[commit-bot: I haz the power, 2017-09-21 23:24:11]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[CalebRouleau, 2017-09-21 23:43:06]

Description was changed from

==========
Allow pages to set custom options.

This code works for me, but I'm wondering if it will break anyone else.
In particular, I'm wondering if other teams use different
SharedPageState classes and for them this method will have no effect.

BUG=catapult:#3863
==========

to

==========
Allow pages to set custom options.


BUG=catapult:#3863
==========

[commit-bot: I haz the power, 2017-09-21 23:56:03]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1506036244045690,
"parent_rev": "703485470a88380b828a6ec8f75e0fe11e371e17", "commit_rev":
"4ee9a49ff94565eeece6078540df8c11a347751d"}

[commit-bot: I haz the power, 2017-09-21 23:56:07]

Description was changed from

==========
Allow pages to set custom options.


BUG=catapult:#3863
==========

to

==========
Allow pages to set custom options.


BUG=catapult:#3863

Review-Url: https://chromiumcodereview.appspot.com/3011293002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-09-21 23:56:08]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...