[Telemetry] Add --user-data-dir to android_browser_backend.py

[Telemetry] Add --user-data-dir to android_browser_backend.py

This CL adds --user-data-dir to android_browser_backend.py. 
--user-data-dir is a prerequisite for --ignore-certificate-errors-spki-list
which allows Chrome to selectively bypass cert errors while avoiding the 
downsides of --ignore-certificate-errors  (e.g. re-establishing socket
connections and skipping disk cache). 

BUG=chromium:753948
Review-Url: https://codereview.chromium.org/3002243002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/b0b51eab27da63357fb9fdf016c087289021cd29

[xunjieli, 2017-08-24 19:08:17]

xunjieli@chromium.org changed reviewers:
+ jbudorick@chromium.org, sullivan@chromium.org

[xunjieli, 2017-08-24 19:08:18]

Annie and John, PTAL. Do you see any problem with this?
Note that for desktop, Chrome is always invoked with --user-data-dir. I'd like
to do the same for Android.

[xunjieli, 2017-08-24 19:08:23]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-24 19:08:32]

Dry run: 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-08-24 20:21:45]

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

[commit-bot: I haz the power, 2017-08-24 20:21:46]

Dry run: This issue passed the CQ dry run.

[sullivan, 2017-08-24 20:24:17]

sullivan@chromium.org changed reviewers:
+ simonhatch@chromium.org

[sullivan, 2017-08-24 20:24:17]

Is it possible to wait for Ned to get in to review? He's back on the 30th.

In the meantime, it would be great if John could weigh in with any known
problems that might crop up.

And Simon, how can a perf try job be run to test the impact of a catapult CL?

[xunjieli, 2017-08-24 20:28:43]

Description was changed from

==========
[Telemetry] Add --user-data-dir to android_browser_backend.py

We would like to use --ignore-certificate-errors-spki-list instead of
--ignore-certificate-errors to bypass certificate errors. More on motivation,
see linked bug below.

--ignore-certificate-errors-spki-list depends on --user-data-dir flag being
present. This CL adds a dummy --user-data-dir flag to android_browser_backend
so --ignore-certificate-errors-spki-list can work.


BUG=chromium:753948
==========

to

==========
[Telemetry] Add --user-data-dir to android_browser_backend.py

We would like to use --ignore-certificate-errors-spki-list instead of
--ignore-certificate-errors to bypass certificate errors. More on motivation,
see linked bug below.

--ignore-certificate-errors-spki-list depends on --user-data-dir flag being
present. This CL adds a dummy --user-data-dir flag to android_browser_backend
so --ignore-certificate-errors-spki-list can work.


BUG=chromium:753948
==========

[xunjieli, 2017-08-24 20:29:26]

On 2017/08/24 20:24:17, sullivan wrote:
> Is it possible to wait for Ned to get in to review? He's back on the 30th.

Sounds good. I will put the other CL on hold as well and wait until Ned gets
back.

[jbudorick, 2017-08-24 20:33:15]

On 2017/08/24 20:24:17, sullivan wrote:
> Is it possible to wait for Ned to get in to review? He's back on the 30th.
> 
> In the meantime, it would be great if John could weigh in with any known
> problems that might crop up.

I haven't used --user-data-dir on Android before. Playing around with it
locally, though, passing "dummy" seems to have no effect, as intended.

This is fine with me, but I'd agree -- wait for Ned.

> 
> And Simon, how can a perf try job be run to test the impact of a catapult CL?

[shatch, 2017-08-24 20:35:12]

On 2017/08/24 20:29:26, xunjieli wrote:
> On 2017/08/24 20:24:17, sullivan wrote:
> > Is it possible to wait for Ned to get in to review? He's back on the 30th.
> 
> Sounds good. I will put the other CL on hold as well and wait until Ned gets
> back.

You should be able to :

./tools/perf/run_benchmark try ... --repo_path=third_party/catapult

The caveat is that you can't have unpinned your catapult checkout:
https://github.com/catapult-project/catapult/issues/3779

[xunjieli, 2017-08-25 14:00:41]

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

[nednguyen, 2017-08-30 13:06:09]

On 2017/08/24 20:35:12, shatch wrote:
> On 2017/08/24 20:29:26, xunjieli wrote:
> > On 2017/08/24 20:24:17, sullivan wrote:
> > > Is it possible to wait for Ned to get in to review? He's back on the 30th.
> > 
> > Sounds good. I will put the other CL on hold as well and wait until Ned gets
> > back.
> 
> You should be able to :
> 
> ./tools/perf/run_benchmark try ... --repo_path=third_party/catapult
> 
> The caveat is that you can't have unpinned your catapult checkout:
> https://github.com/catapult-project/catapult/issues/3779


Have you been able to verify that this makes the tests in
https://codereview.chromium.org/3003143002/ passing? If so, I am supportive of
this change as it makes Chrome behavior more realistic as well.

The process for making this change would then be sending an announcement to
telemetry-announce@ & land this after 3 days if there is no push back.

+Juan who owns android memory benchmarks & Pasko who owns Android startup
benchmarks.

[nednguyen, 2017-08-30 13:06:31]

nednguyen@google.com changed reviewers:
+ pasko@chromium.org, perezju@chromium.org

[xunjieli, 2017-08-30 13:16:15]

On 2017/08/30 13:06:09, nednguyen wrote:
> On 2017/08/24 20:35:12, shatch wrote:
> > On 2017/08/24 20:29:26, xunjieli wrote:
> > > On 2017/08/24 20:24:17, sullivan wrote:
> > > > Is it possible to wait for Ned to get in to review? He's back on the
30th.
> > > 
> > > Sounds good. I will put the other CL on hold as well and wait until Ned
gets
> > > back.
> > 
> > You should be able to :
> > 
> > ./tools/perf/run_benchmark try ... --repo_path=third_party/catapult
> > 
> > The caveat is that you can't have unpinned your catapult checkout:
> > https://github.com/catapult-project/catapult/issues/3779
> 
> 
> Have you been able to verify that this makes the tests in
> https://codereview.chromium.org/3003143002/ passing? If so, I am supportive of
> this change as it makes Chrome behavior more realistic as well.
> 

Welcome back!
Yes, Patch #7 of that CL depends on this
(https://codereview.chromium.org/3003143002/#ps180001). You can see that the
Android Tryserver is green.
In PS #6 of that same CL, the Android Tryserver is not. I verified locally as
well.

> The process for making this change would then be sending an announcement to
> telemetry-announce@ & land this after 3 days if there is no push back.
> 
> +Juan who owns android memory benchmarks & Pasko who owns Android startup
> benchmarks.

SG. I will send an email to telemetry-announce@.

[nednguyen, 2017-08-30 13:20:17]

lgtm

[xunjieli, 2017-08-30 13:48:44]

Description was changed from

==========
[Telemetry] Add --user-data-dir to android_browser_backend.py

We would like to use --ignore-certificate-errors-spki-list instead of
--ignore-certificate-errors to bypass certificate errors. More on motivation,
see linked bug below.

--ignore-certificate-errors-spki-list depends on --user-data-dir flag being
present. This CL adds a dummy --user-data-dir flag to android_browser_backend
so --ignore-certificate-errors-spki-list can work.


BUG=chromium:753948
==========

to

==========
[Telemetry] Add --user-data-dir to android_browser_backend.py

This CL adds --user-data-dir to android_browser_backend.py. 
--user-data-dir is a prerequisite for  --ignore-certificate-errors-spki-list
which allows Chrome to selectively bypass cert errors. More on motivation,
see linked bug below.

BUG=chromium:753948
==========

[xunjieli, 2017-08-30 13:48:59]

Description was changed from

==========
[Telemetry] Add --user-data-dir to android_browser_backend.py

This CL adds --user-data-dir to android_browser_backend.py. 
--user-data-dir is a prerequisite for  --ignore-certificate-errors-spki-list
which allows Chrome to selectively bypass cert errors. More on motivation,
see linked bug below.

BUG=chromium:753948
==========

to

==========
[Telemetry] Add --user-data-dir to android_browser_backend.py

This CL adds --user-data-dir to android_browser_backend.py. 
--user-data-dir is a prerequisite for --ignore-certificate-errors-spki-list
which allows Chrome to selectively bypass cert errors. More on motivation,
see linked bug below.

BUG=chromium:753948
==========

[perezju, 2017-08-30 13:58:14]

On 2017/08/30 13:20:17, nednguyen wrote:
> lgtm

Is this flag related to Chrome's profile dir? How is this going to interact with
e.g. RemoveProfile [1] or PushProfile [2]?

[1]:
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
[2]:
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...

[xunjieli, 2017-08-30 14:00:55]

On 2017/08/30 13:58:14, perezju wrote:
> On 2017/08/30 13:20:17, nednguyen wrote:
> > lgtm
> 
> Is this flag related to Chrome's profile dir? How is this going to interact
with
> e.g. RemoveProfile [1] or PushProfile [2]?
> 
> [1]:
>
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
> [2]:
>
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...

Since we are using a dummy directory here (which is invalid), Chrome defaults to
the default directory.  There shouldn't be any impact on PremoveProfile and
PushProfile. They should work fine.

[perezju, 2017-08-30 14:02:57]

On 2017/08/30 14:00:55, xunjieli wrote:
> On 2017/08/30 13:58:14, perezju wrote:
> > On 2017/08/30 13:20:17, nednguyen wrote:
> > > lgtm
> > 
> > Is this flag related to Chrome's profile dir? How is this going to interact
> with
> > e.g. RemoveProfile [1] or PushProfile [2]?
> > 
> > [1]:
> >
>
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
> > [2]:
> >
>
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
> 
> Since we are using a dummy directory here (which is invalid), Chrome defaults
to
> the default directory.  There shouldn't be any impact on PremoveProfile and
> PushProfile. They should work fine.

that sgtm, thanks for the explanation. Could you add a small comment explaining
that next to where you add the flag?

lgtm

[pasko, 2017-08-30 14:22:45]

https://codereview.chromium.org/3002243002/diff/1/telemetry/telemetry/interna...
File telemetry/telemetry/internal/backends/chrome/android_browser_backend.py
(right):

https://codereview.chromium.org/3002243002/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:204:
args.append('--user-data-dir=dummy')
is this a relative path? is it going to be cleaned up after benchmark run? If it
can be accidentally reused from previous runs, will it make a performance
difference?

I would vote for an absolute path here and some initialization/removal steps in
Telemetry for it, but I am not sure how to make it best.

Also, since this redefines the user data dir to be potentially on another
partition, this may affect our perf graphs with unrelated tests, which is
undesirable.

Please enhance the issue description with details of why --user-data-dir is
needed. Referring to the bug is not convenient, as many details may be added to
the bug later and it will be difficult to find the bits you intended to
reference here.

In particular, I do not understand why the default user data dir location does
not work for us. We are running the benchmark as root, so we can put anything to
this directory.

[xunjieli, 2017-08-30 14:25:00]

On 2017/08/30 14:22:45, pasko wrote:
>
https://codereview.chromium.org/3002243002/diff/1/telemetry/telemetry/interna...
> File telemetry/telemetry/internal/backends/chrome/android_browser_backend.py
> (right):
> 
>
https://codereview.chromium.org/3002243002/diff/1/telemetry/telemetry/interna...
> telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:204:
> args.append('--user-data-dir=dummy')
> is this a relative path? is it going to be cleaned up after benchmark run? If
it
> can be accidentally reused from previous runs, will it make a performance
> difference?
> 
> I would vote for an absolute path here and some initialization/removal steps
in
> Telemetry for it, but I am not sure how to make it best.
> 
> Also, since this redefines the user data dir to be potentially on another
> partition, this may affect our perf graphs with unrelated tests, which is
> undesirable.
> 
> Please enhance the issue description with details of why --user-data-dir is
> needed. Referring to the bug is not convenient, as many details may be added
to
> the bug later and it will be difficult to find the bits you intended to
> reference here.
> 
> In particular, I do not understand why the default user data dir location does
> not work for us. We are running the benchmark as root, so we can put anything
to
> this directory.

Thank you both. I think default user data dir might work. Let me try that and
also add more comments to the CL and in the CL description.

[xunjieli, 2017-08-30 14:27:25]

On 2017/08/30 14:25:00, xunjieli wrote:
> On 2017/08/30 14:22:45, pasko wrote:
> >
>
https://codereview.chromium.org/3002243002/diff/1/telemetry/telemetry/interna...
> > File telemetry/telemetry/internal/backends/chrome/android_browser_backend.py
> > (right):
> > 
> >
>
https://codereview.chromium.org/3002243002/diff/1/telemetry/telemetry/interna...
> > telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:204:
> > args.append('--user-data-dir=dummy')
> > is this a relative path? is it going to be cleaned up after benchmark run?
If
> it
> > can be accidentally reused from previous runs, will it make a performance
> > difference?
> > 
> > I would vote for an absolute path here and some initialization/removal steps
> in
> > Telemetry for it, but I am not sure how to make it best.
> > 
> > Also, since this redefines the user data dir to be potentially on another
> > partition, this may affect our perf graphs with unrelated tests, which is
> > undesirable.
> > 
> > Please enhance the issue description with details of why --user-data-dir is
> > needed. Referring to the bug is not convenient, as many details may be added
> to
> > the bug later and it will be difficult to find the bits you intended to
> > reference here.
> > 
> > In particular, I do not understand why the default user data dir location
does
> > not work for us. We are running the benchmark as root, so we can put
anything
> to
> > this directory.
> 
> Thank you both. I think default user data dir might work. Let me try that and
> also add more comments to the CL and in the CL description.

The Telemetry-announce email is at
https://groups.google.com/a/chromium.org/forum/#!topic/telemetry-announce/i8Q...

[xunjieli, 2017-08-30 14:42:51]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-30 14:42:57]

Dry run: CQ is trying da patch.

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

[pasko, 2017-08-30 14:47:02]

On 2017/08/30 14:27:25, xunjieli wrote:
> On 2017/08/30 14:25:00, xunjieli wrote:
> > On 2017/08/30 14:22:45, pasko wrote:
> > >
> >
>
https://codereview.chromium.org/3002243002/diff/1/telemetry/telemetry/interna...
> > > File
telemetry/telemetry/internal/backends/chrome/android_browser_backend.py
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/3002243002/diff/1/telemetry/telemetry/interna...
> > >
telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:204:
> > > args.append('--user-data-dir=dummy')
> > > is this a relative path? is it going to be cleaned up after benchmark run?
> If
> > it
> > > can be accidentally reused from previous runs, will it make a performance
> > > difference?
> > > 
> > > I would vote for an absolute path here and some initialization/removal
steps
> > in
> > > Telemetry for it, but I am not sure how to make it best.
> > > 
> > > Also, since this redefines the user data dir to be potentially on another
> > > partition, this may affect our perf graphs with unrelated tests, which is
> > > undesirable.
> > > 
> > > Please enhance the issue description with details of why --user-data-dir
is
> > > needed. Referring to the bug is not convenient, as many details may be
added
> > to
> > > the bug later and it will be difficult to find the bits you intended to
> > > reference here.
> > > 
> > > In particular, I do not understand why the default user data dir location
> does
> > > not work for us. We are running the benchmark as root, so we can put
> anything
> > to
> > > this directory.
> > 
> > Thank you both. I think default user data dir might work. Let me try that
and
> > also add more comments to the CL and in the CL description.
> 
> The Telemetry-announce email is at
>
https://groups.google.com/a/chromium.org/forum/#!topic/telemetry-announce/i8Q...

Thank you for the explanation. Please inline the summary of it into a comment in
the code. Given a description of what is happening in comments, the reference to
the bug is probably not necessary.

[xunjieli, 2017-08-30 15:02:11]

Description was changed from

==========
[Telemetry] Add --user-data-dir to android_browser_backend.py

This CL adds --user-data-dir to android_browser_backend.py. 
--user-data-dir is a prerequisite for --ignore-certificate-errors-spki-list
which allows Chrome to selectively bypass cert errors. More on motivation,
see linked bug below.

BUG=chromium:753948
==========

to

==========
[Telemetry] Add --user-data-dir to android_browser_backend.py

This CL adds --user-data-dir to android_browser_backend.py. 
--user-data-dir is a prerequisite for --ignore-certificate-errors-spki-list
which allows Chrome to selectively bypass cert errors while avoiding the
downsides of --ignore-certificate-errors  (e.g. re-establishing socket
connections and skipping disk cache). More on motivation,
see linked bug below.

BUG=chromium:753948
==========

[xunjieli, 2017-08-30 15:02:26]

Description was changed from

==========
[Telemetry] Add --user-data-dir to android_browser_backend.py

This CL adds --user-data-dir to android_browser_backend.py. 
--user-data-dir is a prerequisite for --ignore-certificate-errors-spki-list
which allows Chrome to selectively bypass cert errors while avoiding the
downsides of --ignore-certificate-errors  (e.g. re-establishing socket
connections and skipping disk cache). More on motivation,
see linked bug below.

BUG=chromium:753948
==========

to

==========
[Telemetry] Add --user-data-dir to android_browser_backend.py

This CL adds --user-data-dir to android_browser_backend.py. 
--user-data-dir is a prerequisite for --ignore-certificate-errors-spki-list
which allows Chrome to selectively bypass cert errors while avoiding the 
downsides of --ignore-certificate-errors  (e.g. re-establishing socket
connections and skipping disk cache). More on motivation,
see linked bug below.

BUG=chromium:753948
==========

[commit-bot: I haz the power, 2017-08-30 16:43:08]

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

[commit-bot: I haz the power, 2017-08-30 16:43:09]

Dry run: Try jobs failed on following builders:
  Catapult Android Tryserver on master.tryserver.client.catapult (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[xunjieli, 2017-08-30 17:21:47]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-30 17:21:57]

Dry run: 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-08-30 18:03:50]

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

[commit-bot: I haz the power, 2017-08-30 18:03:51]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2017-08-30 21:37:14]

Description was changed from

==========
[Telemetry] Add --user-data-dir to android_browser_backend.py

This CL adds --user-data-dir to android_browser_backend.py. 
--user-data-dir is a prerequisite for --ignore-certificate-errors-spki-list
which allows Chrome to selectively bypass cert errors while avoiding the 
downsides of --ignore-certificate-errors  (e.g. re-establishing socket
connections and skipping disk cache). More on motivation,
see linked bug below.

BUG=chromium:753948
==========

to

==========
[Telemetry] Add --user-data-dir to android_browser_backend.py

This CL adds --user-data-dir to android_browser_backend.py. 
--user-data-dir is a prerequisite for --ignore-certificate-errors-spki-list
which allows Chrome to selectively bypass cert errors while avoiding the 
downsides of --ignore-certificate-errors  (e.g. re-establishing socket
connections and skipping disk cache). 

BUG=chromium:753948
==========

[nednguyen, 2017-08-30 21:39:20]

On 2017/08/30 14:47:02, pasko wrote:
> On 2017/08/30 14:27:25, xunjieli wrote:
> > On 2017/08/30 14:25:00, xunjieli wrote:
> > > On 2017/08/30 14:22:45, pasko wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/3002243002/diff/1/telemetry/telemetry/interna...
> > > > File
> telemetry/telemetry/internal/backends/chrome/android_browser_backend.py
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/3002243002/diff/1/telemetry/telemetry/interna...
> > > >
> telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:204:
> > > > args.append('--user-data-dir=dummy')
> > > > is this a relative path? is it going to be cleaned up after benchmark
run?
> > If
> > > it
> > > > can be accidentally reused from previous runs, will it make a
performance
> > > > difference?
> > > > 
> > > > I would vote for an absolute path here and some initialization/removal
> steps
> > > in
> > > > Telemetry for it, but I am not sure how to make it best.
> > > > 
> > > > Also, since this redefines the user data dir to be potentially on
another
> > > > partition, this may affect our perf graphs with unrelated tests, which
is
> > > > undesirable.
> > > > 
> > > > Please enhance the issue description with details of why --user-data-dir
> is
> > > > needed. Referring to the bug is not convenient, as many details may be
> added
> > > to
> > > > the bug later and it will be difficult to find the bits you intended to
> > > > reference here.
> > > > 
> > > > In particular, I do not understand why the default user data dir
location
> > does
> > > > not work for us. We are running the benchmark as root, so we can put
> > anything
> > > to
> > > > this directory.
> > > 
> > > Thank you both. I think default user data dir might work. Let me try that
> and
> > > also add more comments to the CL and in the CL description.
> > 
> > The Telemetry-announce email is at
> >
>
https://groups.google.com/a/chromium.org/forum/#!topic/telemetry-announce/i8Q...
> 
> Thank you for the explanation. Please inline the summary of it into a comment
in
> the code. Given a description of what is happening in comments, the reference
to
> the bug is probably not necessary.

I think with this, we may need to make sure that Telemetry clean up the profile
upon startup to avoid one test run affect the next test run.  Helen: can you do
a few run locally & check if the user directory size grows?

[xunjieli, 2017-08-30 21:40:10]

> > > Thank you both. I think default user data dir might work. Let me try that
> and
> > > also add more comments to the CL and in the CL description.
> > 
> > The Telemetry-announce email is at
> >
>
https://groups.google.com/a/chromium.org/forum/#!topic/telemetry-announce/i8Q...
> 
> Thank you for the explanation. Please inline the summary of it into a comment
in
> the code. Given a description of what is happening in comments, the reference
to
> the bug is probably not necessary.

Done. Thank you! Using default user profile directory works. 
perezju, pasko@: Could you take another look? I tried to summarize in a comment
above the flag. Let me know if I can make it clearer.

[xunjieli, 2017-08-30 21:46:21]

> I think with this, we may need to make sure that Telemetry clean up the
profile
> upon startup to avoid one test run affect the next test run.  Helen: can you
do
> a few run locally & check if the user directory size grows?

SG. I will do that now.

[xunjieli, 2017-08-30 22:08:28]

On 2017/08/30 21:46:21, xunjieli wrote:
> > I think with this, we may need to make sure that Telemetry clean up the
> profile
> > upon startup to avoid one test run affect the next test run.  Helen: can you
> do
> > a few run locally & check if the user directory size grows?
> 
> SG. I will do that now.

Not sure if this is sufficient.

I ran this three times:
$ telemetry/bin/run_tests
telemetry.page.page_run_end_to_end_unittest.ActualPageRunEndToEndTests.testWebPageReplay

which outputs that 
"
  Browser started (pid=24874).
  OS: android M
  Model: Nexus 5X
  Browser command line: _ --no-default-browser-check --metrics-recording-only
--proxy-server=socks://localhost:41814
--user-data-dir=/data/data/com.google.android.apps.chrome/
--enable-remote-debugging --disable-external-intent-requests
--ignore-certificate-errors --disable-component-extensions-with-background-pages
--disable-fre --enable-gpu-benchmarking --disable-default-apps
--enable-net-benchmarking --disable-search-geolocation-disclosure --no-first-run
--disable-background-networking --use-mobile-user-agent
--top-controls-show-threshold=0.5 --top-controls-hide-threshold=0.5
--use-mobile-user-agent --enable-pinch --enable-viewport
--validate-input-event-stream --enable-longpress-drag-selection
--touch-selection-strategy=direction --disable-gpu-process-crash-limit
--main-frame-resizes-are-orientation-changes --disable-composited-antialiasing
--ui-prioritize-in-gpu-process --profiler-timing=0
--prerender-from-omnibox=enabled --enable-dom-distiller --flag-switches-begin
--flag-switches-end
  GPU device 0: VENDOR = 0x0 (Qualcomm), DEVICE = 0x0 (Adreno (TM) 418)
"

and the size of /data/data/com.google.android.apps.chrome/ didn't change.

adb shell du -sh /data/data/com.google.android.apps.chrome/
1.0M    /data/data/com.google.android.apps.chrome/

[xunjieli, 2017-08-30 22:21:53]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-30 22:21:57]

Dry run: CQ is trying da patch.

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

[nednguyen, 2017-08-30 22:32:40]

On 2017/08/30 22:08:28, xunjieli wrote:
> On 2017/08/30 21:46:21, xunjieli wrote:
> > > I think with this, we may need to make sure that Telemetry clean up the
> > profile
> > > upon startup to avoid one test run affect the next test run.  Helen: can
you
> > do
> > > a few run locally & check if the user directory size grows?
> > 
> > SG. I will do that now.
> 
> Not sure if this is sufficient.
> 
> I ran this three times:
> $ telemetry/bin/run_tests
>
telemetry.page.page_run_end_to_end_unittest.ActualPageRunEndToEndTests.testWebPageReplay
> 
> which outputs that 
> "
>   Browser started (pid=24874).
>   OS: android M
>   Model: Nexus 5X
>   Browser command line: _ --no-default-browser-check --metrics-recording-only
> --proxy-server=socks://localhost:41814
> --user-data-dir=/data/data/com.google.android.apps.chrome/
> --enable-remote-debugging --disable-external-intent-requests
> --ignore-certificate-errors
--disable-component-extensions-with-background-pages
> --disable-fre --enable-gpu-benchmarking --disable-default-apps
> --enable-net-benchmarking --disable-search-geolocation-disclosure
--no-first-run
> --disable-background-networking --use-mobile-user-agent
> --top-controls-show-threshold=0.5 --top-controls-hide-threshold=0.5
> --use-mobile-user-agent --enable-pinch --enable-viewport
> --validate-input-event-stream --enable-longpress-drag-selection
> --touch-selection-strategy=direction --disable-gpu-process-crash-limit
> --main-frame-resizes-are-orientation-changes --disable-composited-antialiasing
> --ui-prioritize-in-gpu-process --profiler-timing=0
> --prerender-from-omnibox=enabled --enable-dom-distiller --flag-switches-begin
> --flag-switches-end
>   GPU device 0: VENDOR = 0x0 (Qualcomm), DEVICE = 0x0 (Adreno (TM) 418)
> "
> 
> and the size of /data/data/com.google.android.apps.chrome/ didn't change.
> 
> adb shell du -sh /data/data/com.google.android.apps.chrome/
> 1.0M    /data/data/com.google.android.apps.chrome/

SGTM. There is existing logic to clean up the profile
(https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...).
I just want to make sure that they're triggered. 
Actually can you add some exception there & see if that code path is hit?

[commit-bot: I haz the power, 2017-08-30 22:48:48]

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

[commit-bot: I haz the power, 2017-08-30 22:48:50]

Dry run: This issue passed the CQ dry run.

[perezju, 2017-08-31 07:55:35]

lgtm

On 2017/08/30 22:32:40, nednguyen wrote:
> SGTM. There is existing logic to clean up the profile
>
(https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...).
> I just want to make sure that they're triggered. 
> Actually can you add some exception there & see if that code path is hit?

I'm pretty sure that code path should be triggered. But it would be nice to
double check.

[pasko, 2017-08-31 11:24:36]

lgtm, thank you

[xunjieli, 2017-08-31 14:49:36]

On 2017/08/30 22:32:40, nednguyen wrote:
> On 2017/08/30 22:08:28, xunjieli wrote:
> > On 2017/08/30 21:46:21, xunjieli wrote:
> > > > I think with this, we may need to make sure that Telemetry clean up the
> > > profile
> > > > upon startup to avoid one test run affect the next test run.  Helen: can
> you
> > > do
> > > > a few run locally & check if the user directory size grows?
> > > 
> > > SG. I will do that now.
> > 
> > Not sure if this is sufficient.
> > 
> > I ran this three times:
> > $ telemetry/bin/run_tests
> >
>
telemetry.page.page_run_end_to_end_unittest.ActualPageRunEndToEndTests.testWebPageReplay
> > 
> > which outputs that 
> > "
> >   Browser started (pid=24874).
> >   OS: android M
> >   Model: Nexus 5X
> >   Browser command line: _ --no-default-browser-check
--metrics-recording-only
> > --proxy-server=socks://localhost:41814
> > --user-data-dir=/data/data/com.google.android.apps.chrome/
> > --enable-remote-debugging --disable-external-intent-requests
> > --ignore-certificate-errors
> --disable-component-extensions-with-background-pages
> > --disable-fre --enable-gpu-benchmarking --disable-default-apps
> > --enable-net-benchmarking --disable-search-geolocation-disclosure
> --no-first-run
> > --disable-background-networking --use-mobile-user-agent
> > --top-controls-show-threshold=0.5 --top-controls-hide-threshold=0.5
> > --use-mobile-user-agent --enable-pinch --enable-viewport
> > --validate-input-event-stream --enable-longpress-drag-selection
> > --touch-selection-strategy=direction --disable-gpu-process-crash-limit
> > --main-frame-resizes-are-orientation-changes
--disable-composited-antialiasing
> > --ui-prioritize-in-gpu-process --profiler-timing=0
> > --prerender-from-omnibox=enabled --enable-dom-distiller
--flag-switches-begin
> > --flag-switches-end
> >   GPU device 0: VENDOR = 0x0 (Qualcomm), DEVICE = 0x0 (Adreno (TM) 418)
> > "
> > 
> > and the size of /data/data/com.google.android.apps.chrome/ didn't change.
> > 
> > adb shell du -sh /data/data/com.google.android.apps.chrome/
> > 1.0M    /data/data/com.google.android.apps.chrome/
> 
> SGTM. There is existing logic to clean up the profile
>
(https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...).
> I just want to make sure that they're triggered. 
> Actually can you add some exception there & see if that code path is hit?

I added an assert and the RemoveProfile() is triggered upon starting telemetry.
I talked to Ned offline. Since we are using the default dir here, this change
isn't as risky as using a dummy dir. I will CQ this now and keep an eye on the
bots. If this causes any problem, I will revert it.

[xunjieli, 2017-08-31 14:49:48]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-08-31 14:49:49]

The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/3002243002/#ps40001
(title: "summary in comment")

[commit-bot: I haz the power, 2017-08-31 14:49:55]

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-08-31 14:51:50]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1504190988708590,
"parent_rev": "8a17d643192760e1d8e747a6ce5548c660fa8a91", "commit_rev":
"b0b51eab27da63357fb9fdf016c087289021cd29"}

[commit-bot: I haz the power, 2017-08-31 14:51:56]

Description was changed from

==========
[Telemetry] Add --user-data-dir to android_browser_backend.py

This CL adds --user-data-dir to android_browser_backend.py. 
--user-data-dir is a prerequisite for --ignore-certificate-errors-spki-list
which allows Chrome to selectively bypass cert errors while avoiding the 
downsides of --ignore-certificate-errors  (e.g. re-establishing socket
connections and skipping disk cache). 

BUG=chromium:753948
==========

to

==========
[Telemetry] Add --user-data-dir to android_browser_backend.py

This CL adds --user-data-dir to android_browser_backend.py. 
--user-data-dir is a prerequisite for --ignore-certificate-errors-spki-list
which allows Chrome to selectively bypass cert errors while avoiding the 
downsides of --ignore-certificate-errors  (e.g. re-establishing socket
connections and skipping disk cache). 

BUG=chromium:753948

Review-Url: https://codereview.chromium.org/3002243002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-08-31 14:51:57]

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