[tools/android/loading] Helper script running Chrome on device with WPR

[tools/android/loading] Helper script running Chrome on device with WPR

Review-Url: https://codereview.chromium.org/2687803004
Cr-Commit-Position: refs/heads/master@{#453582}
Committed: https://chromium.googlesource.com/chromium/src/+/d8eae749762d9b7f9f0e737b7a6c1ea2bc82aafe

[droger, 2017-02-10 13:53:58]

Patchset #1 (id:1) has been deleted

[droger, 2017-02-10 13:55:30]

droger@chromium.org changed reviewers:
+ alexilin@chromium.org, lizeb@chromium.org

[droger, 2017-02-10 13:55:30]

This is a helper script I wrote, Alex suggested that I commit it as it may be
useful to others.

Let me know if this has some value and if this is the right location for the
script.

[droger, 2017-02-10 14:02:50]

Patchset #1 (id:20001) has been deleted

[alexilin, 2017-02-10 14:37:23]

LGTM, thanks!

https://codereview.chromium.org/2687803004/diff/40001/tools/android/loading/w...
File tools/android/loading/wpr_helper.py (right):

https://codereview.chromium.org/2687803004/diff/40001/tools/android/loading/w...
tools/android/loading/wpr_helper.py:3: # Copyright 2015 The Chromium Authors.
All rights reserved.
nit:
s/2015/2017/

[Benoit L, 2017-02-13 10:13:44]

https://codereview.chromium.org/2687803004/diff/60001/tools/android/loading/w...
File tools/android/loading/wpr_helper.py (right):

https://codereview.chromium.org/2687803004/diff/60001/tools/android/loading/w...
tools/android/loading/wpr_helper.py:28: sys.path.append(os.path.join(_SRC_PATH,
'tools', 'android', 'loading'))
Is this really necessary?
I think that . is in the path by default.

https://codereview.chromium.org/2687803004/diff/60001/tools/android/loading/w...
tools/android/loading/wpr_helper.py:34: # Disable backgound network requests
that may pollute WPR archive, pollute
We are duplicating this in a few places now.
Can you update controller.py in this directory with something like "common
flags", and reference this here?

https://codereview.chromium.org/2687803004/diff/60001/tools/android/loading/w...
tools/android/loading/wpr_helper.py:51: def ResetChromeLocalState(device,
package):
ditto, can this be merged with the matching function in controller.py?

https://codereview.chromium.org/2687803004/diff/60001/tools/android/loading/w...
tools/android/loading/wpr_helper.py:86: print(
incredibly tiny nit: add a space before '(', as print is a statement, not a
function.

Or use logging.info (or warning, up to you).

[droger, 2017-02-17 13:36:57]

Thanks

https://codereview.chromium.org/2687803004/diff/60001/tools/android/loading/w...
File tools/android/loading/wpr_helper.py (right):

https://codereview.chromium.org/2687803004/diff/60001/tools/android/loading/w...
tools/android/loading/wpr_helper.py:28: sys.path.append(os.path.join(_SRC_PATH,
'tools', 'android', 'loading'))
On 2017/02/13 10:13:44, Benoit L wrote:
> Is this really necessary?
> I think that . is in the path by default.

Done.

https://codereview.chromium.org/2687803004/diff/60001/tools/android/loading/w...
tools/android/loading/wpr_helper.py:34: # Disable backgound network requests
that may pollute WPR archive, pollute
On 2017/02/13 10:13:43, Benoit L wrote:
> We are duplicating this in a few places now.
> Can you update controller.py in this directory with something like "common
> flags", and reference this here?

Done.

https://codereview.chromium.org/2687803004/diff/60001/tools/android/loading/w...
tools/android/loading/wpr_helper.py:51: def ResetChromeLocalState(device,
package):
On 2017/02/13 10:13:43, Benoit L wrote:
> ditto, can this be merged with the matching function in controller.py?

Done.

https://codereview.chromium.org/2687803004/diff/60001/tools/android/loading/w...
tools/android/loading/wpr_helper.py:86: print(
On 2017/02/13 10:13:44, Benoit L wrote:
> incredibly tiny nit: add a space before '(', as print is a statement, not a
> function.
> 
> Or use logging.info (or warning, up to you).

Done.

[droger, 2017-02-28 09:59:32]

lizeb: ping
I can also close the CL if this is not useful in the repo.

[Benoit L, 2017-02-28 12:53:37]

On 2017/02/28 09:59:32, droger wrote:
> lizeb: ping
> I can also close the CL if this is not useful in the repo.

lgtm, sorry for the delay.

[droger, 2017-02-28 12:54:47]

On 2017/02/28 12:53:37, Benoit L wrote:
> On 2017/02/28 09:59:32, droger wrote:
> > lizeb: ping
> > I can also close the CL if this is not useful in the repo.
> 
> lgtm, sorry for the delay.

No problem, thanks!

[droger, 2017-02-28 12:54:55]

The CQ bit was checked by droger@chromium.org

[droger, 2017-02-28 12:54:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexilin@chromium.org
Link to the patchset: https://codereview.chromium.org/2687803004/#ps80001
(title: "Share dupliacted code")

[commit-bot: I haz the power, 2017-02-28 12:55:16]

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-02-28 13:05:34]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488286495112920,
"parent_rev": "7164a2cd55c9d685288fc548930608cd641a0b42", "commit_rev":
"d8eae749762d9b7f9f0e737b7a6c1ea2bc82aafe"}

[commit-bot: I haz the power, 2017-02-28 13:06:13]

Description was changed from

==========
[tools/android/loading] Helper script running Chrome on device with WPR
==========

to

==========
[tools/android/loading] Helper script running Chrome on device with WPR

Review-Url: https://codereview.chromium.org/2687803004
Cr-Commit-Position: refs/heads/master@{#453582}
Committed:
https://chromium.googlesource.com/chromium/src/+/d8eae749762d9b7f9f0e737b7a6c...
==========

[commit-bot: I haz the power, 2017-02-28 13:06:14]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/d8eae749762d9b7f9f0e737b7a6c...