Fix breakage when blink_perf benchmarks are run without --browser flag

Fix breakage when blink_perf benchmarks are run without --browser flag

Before this, running blink_perf benchmarks through Telemetry will raise exception
if "--browser" flag isn't specified.

This is because when browser_type isn't set, Telemetry will set a default value using
the latest browser build, but this happens after CustomizeBrowserOptions method call.
Therefore the check "if 'content-shell' in 
options.browser_type:"raises an exception because options.browser_type is still None at this point.

This CL fixes it by moving the logic of augmenting '--expose-internals-for-testing' 
to SetOptions hooks, which is invoked after the default value of browser_type
is already set.

Review-Url: https://codereview.chromium.org/2868183002
Cr-Commit-Position: refs/heads/master@{#470617}
Committed: https://chromium.googlesource.com/chromium/src/+/5acc8b0fec7834edf48f612f22274220f32669c3

[nednguyen, 2017-05-10 00:46:16]

Description was changed from

==========
Default blink_perf benchmarks to use content-shell browser
==========

to

==========
Fix breakage when blink_perf benchmarks are run without --browser flag

Before this, running blink_perf benchmarks through Telemetry will raise
exception.

This is because when browser_type isn't set, Telemetry will set a default value
using
the latest browser build, but this happens after CustomizeBrowserOptions method
call.
Therefore the breakage happens at line "if 'content-shell' in
options.browser_type:" --> exception thrown because options.browser_type is
still None at this point.

This CL fixes it by moving the logic of augmenting
'--expose-internals-for-testing' 
to SetOptions hooks, which is invoked after the default value of browser_type
is already set.
==========

[nednguyen, 2017-05-10 00:46:16]

nednguyen@google.com changed reviewers:
+ wangxianzhu@chromium.org

[nednguyen, 2017-05-10 00:46:29]

Description was changed from

==========
Fix breakage when blink_perf benchmarks are run without --browser flag

Before this, running blink_perf benchmarks through Telemetry will raise
exception.

This is because when browser_type isn't set, Telemetry will set a default value
using
the latest browser build, but this happens after CustomizeBrowserOptions method
call.
Therefore the breakage happens at line "if 'content-shell' in
options.browser_type:" --> exception thrown because options.browser_type is
still None at this point.

This CL fixes it by moving the logic of augmenting
'--expose-internals-for-testing' 
to SetOptions hooks, which is invoked after the default value of browser_type
is already set.
==========

to

==========
Fix breakage when blink_perf benchmarks are run without --browser flag

Before this, running blink_perf benchmarks through Telemetry will raise
exception.

This is because when browser_type isn't set, Telemetry will set a default value
using
the latest browser build, but this happens after CustomizeBrowserOptions method
call.
Therefore the breakage happens at line "if 'content-shell' in 
options.browser_type:" --> exception thrown because options.browser_type is
still None at this point.

This CL fixes it by moving the logic of augmenting
'--expose-internals-for-testing' 
to SetOptions hooks, which is invoked after the default value of browser_type
is already set.
==========

[nednguyen, 2017-05-10 00:47:03]

Description was changed from

==========
Fix breakage when blink_perf benchmarks are run without --browser flag

Before this, running blink_perf benchmarks through Telemetry will raise
exception.

This is because when browser_type isn't set, Telemetry will set a default value
using
the latest browser build, but this happens after CustomizeBrowserOptions method
call.
Therefore the breakage happens at line "if 'content-shell' in 
options.browser_type:" --> exception thrown because options.browser_type is
still None at this point.

This CL fixes it by moving the logic of augmenting
'--expose-internals-for-testing' 
to SetOptions hooks, which is invoked after the default value of browser_type
is already set.
==========

to

==========
Fix breakage when blink_perf benchmarks are run without --browser flag

Before this, running blink_perf benchmarks through Telemetry will raise
exception.

This is because when browser_type isn't set, Telemetry will set a default value
using
the latest browser build, but this happens after CustomizeBrowserOptions method
call.
Therefore the check "if 'content-shell' in 
options.browser_type:"raises an exception because options.browser_type is still
None at this point.

This CL fixes it by moving the logic of augmenting
'--expose-internals-for-testing' 
to SetOptions hooks, which is invoked after the default value of browser_type
is already set.
==========

[nednguyen, 2017-05-10 00:48:30]

+Elliot: after this CL, running blink_perf without "--browser" flag will pick up
the latest browser build binary on your client, which hopefully makes it less
verbose for people who build content-shell & iterate.

[nednguyen, 2017-05-10 00:49:56]

Description was changed from

==========
Fix breakage when blink_perf benchmarks are run without --browser flag

Before this, running blink_perf benchmarks through Telemetry will raise
exception.

This is because when browser_type isn't set, Telemetry will set a default value
using
the latest browser build, but this happens after CustomizeBrowserOptions method
call.
Therefore the check "if 'content-shell' in 
options.browser_type:"raises an exception because options.browser_type is still
None at this point.

This CL fixes it by moving the logic of augmenting
'--expose-internals-for-testing' 
to SetOptions hooks, which is invoked after the default value of browser_type
is already set.
==========

to

==========
Fix breakage when blink_perf benchmarks are run without --browser flag

Before this, running blink_perf benchmarks through Telemetry will raise
exception
if "--browser" flag isn't specified.

This is because when browser_type isn't set, Telemetry will set a default value
using
the latest browser build, but this happens after CustomizeBrowserOptions method
call.
Therefore the check "if 'content-shell' in 
options.browser_type:"raises an exception because options.browser_type is still
None at this point.

This CL fixes it by moving the logic of augmenting
'--expose-internals-for-testing' 
to SetOptions hooks, which is invoked after the default value of browser_type
is already set.
==========

[nednguyen, 2017-05-10 00:50:02]

The CQ bit was checked by nednguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 00:51:11]

Dry run: CQ is trying da patch.

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

[Xianzhu, 2017-05-10 02:36:01]

Do we still have tests requiring content_shell and internals api? If not I'd
suggest to remove the logic.

[commit-bot: I haz the power, 2017-05-10 02:53:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 02:53:39]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has
not started yet; builder either lacks capacity or does not exist (misspelled?))
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[nednguyen, 2017-05-10 09:54:47]

https://codereview.chromium.org/2868183002/diff/20001/tools/perf/benchmarks/b...
File tools/perf/benchmarks/blink_perf.py (right):

https://codereview.chromium.org/2868183002/diff/20001/tools/perf/benchmarks/b...
tools/perf/benchmarks/blink_perf.py:171: super(_BlinkPerfMeasurement,
self).SetOptions(options)
The logic of adding "--expose-internals-for-testing" when browser is
content_shell is moved to here to avoid breakage. You can verify this by running
the benchmark with "-v", the log shows '--expose-internals-for-testing' is still
there

[Xianzhu, 2017-05-10 15:48:34]

https://codereview.chromium.org/2868183002/diff/20001/tools/perf/benchmarks/b...
File tools/perf/benchmarks/blink_perf.py (right):

https://codereview.chromium.org/2868183002/diff/20001/tools/perf/benchmarks/b...
tools/perf/benchmarks/blink_perf.py:171: super(_BlinkPerfMeasurement,
self).SetOptions(options)
On 2017/05/10 09:54:47, nednguyen wrote:
> The logic of adding "--expose-internals-for-testing" when browser is
> content_shell is moved to here to avoid breakage. You can verify this by
running
> the benchmark with "-v", the log shows '--expose-internals-for-testing' is
still
> there

I see that the blink_gc test suite is using this configuration. I thought if no
one using we should just remove it.

lgtm.

[nednguyen, 2017-05-10 16:03:08]

On 2017/05/10 15:48:34, Xianzhu wrote:
>
https://codereview.chromium.org/2868183002/diff/20001/tools/perf/benchmarks/b...
> File tools/perf/benchmarks/blink_perf.py (right):
> 
>
https://codereview.chromium.org/2868183002/diff/20001/tools/perf/benchmarks/b...
> tools/perf/benchmarks/blink_perf.py:171: super(_BlinkPerfMeasurement,
> self).SetOptions(options)
> On 2017/05/10 09:54:47, nednguyen wrote:
> > The logic of adding "--expose-internals-for-testing" when browser is
> > content_shell is moved to here to avoid breakage. You can verify this by
> running
> > the benchmark with "-v", the log shows '--expose-internals-for-testing' is
> still
> > there
> 
> I see that the blink_gc test suite is using this configuration. I thought if
no
> one using we should just remove it.
> 
> lgtm.

Oh, I misread your statement. You're right that we should remove this
configuration if it's not needed but that seems to be another CL.

[nednguyen, 2017-05-10 16:03:13]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2017-05-10 16:04:07]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-10 17:02:03]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494432193907010,
"parent_rev": "5cf5aec5302e3f6372e52cf963e21eb8125b3c91", "commit_rev":
"5acc8b0fec7834edf48f612f22274220f32669c3"}

[commit-bot: I haz the power, 2017-05-10 17:02:10]

Description was changed from

==========
Fix breakage when blink_perf benchmarks are run without --browser flag

Before this, running blink_perf benchmarks through Telemetry will raise
exception
if "--browser" flag isn't specified.

This is because when browser_type isn't set, Telemetry will set a default value
using
the latest browser build, but this happens after CustomizeBrowserOptions method
call.
Therefore the check "if 'content-shell' in 
options.browser_type:"raises an exception because options.browser_type is still
None at this point.

This CL fixes it by moving the logic of augmenting
'--expose-internals-for-testing' 
to SetOptions hooks, which is invoked after the default value of browser_type
is already set.
==========

to

==========
Fix breakage when blink_perf benchmarks are run without --browser flag

Before this, running blink_perf benchmarks through Telemetry will raise
exception
if "--browser" flag isn't specified.

This is because when browser_type isn't set, Telemetry will set a default value
using
the latest browser build, but this happens after CustomizeBrowserOptions method
call.
Therefore the check "if 'content-shell' in 
options.browser_type:"raises an exception because options.browser_type is still
None at this point.

This CL fixes it by moving the logic of augmenting
'--expose-internals-for-testing' 
to SetOptions hooks, which is invoked after the default value of browser_type
is already set.

Review-Url: https://codereview.chromium.org/2868183002
Cr-Commit-Position: refs/heads/master@{#470617}
Committed:
https://chromium.googlesource.com/chromium/src/+/5acc8b0fec7834edf48f612f2227...
==========

[commit-bot: I haz the power, 2017-05-10 17:02:11]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/5acc8b0fec7834edf48f612f2227...