tools: Add a tool to load pages on a device, and pull the predictor database.

tools: Add a tool to load pages on a device, and pull the predictor database.

BUG=655980
Committed: https://crrev.com/1ad4f365b9917f92886d8e76c8e2789a9cb7e696
Cr-Commit-Position: refs/heads/master@{#431869}

[Benoit L, 2016-11-10 14:56:06]

lizeb@chromium.org changed reviewers:
+ pasko@chromium.org

[pasko, 2016-11-10 16:02:01]

https://codereview.chromium.org/2496563002/diff/1/tools/resource_prefetch_pre...
File tools/resource_prefetch_predictor/generate_database.py (right):

https://codereview.chromium.org/2496563002/diff/1/tools/resource_prefetch_pre...
tools/resource_prefetch_predictor/generate_database.py:31: import device_setup
do we need this dependency?

https://codereview.chromium.org/2496563002/diff/1/tools/resource_prefetch_pre...
tools/resource_prefetch_predictor/generate_database.py:42: # HTTP cache
generation, and introduce noise in loading performance.
this is getting repeated, should some sort of common function in
tools/android/loading return the list?

https://codereview.chromium.org/2496563002/diff/1/tools/resource_prefetch_pre...
tools/resource_prefetch_predictor/generate_database.py:67:
parser.add_option('--repeats', help='Number of times each URL is loaded.',
nit: s/repeats/url-repeat/ sounds more consistent with similar params in
telemetry

also, ambiguity: this seems to be about repeating working on each line in the
file, not about times to load each unique url

https://codereview.chromium.org/2496563002/diff/1/tools/resource_prefetch_pre...
tools/resource_prefetch_predictor/generate_database.py:85: profile_dirs =
['app_chrome/Default', 'cache', 'app_chrome/ShaderCache',
Do you want to avoid dependency on tools/android/loading/controller.py? Why?

https://codereview.chromium.org/2496563002/diff/1/tools/resource_prefetch_pre...
tools/resource_prefetch_predictor/generate_database.py:97: device,
'/data/local/tmp/chrome-command-line')
the controller.py is doing it with /data/local/chrome-command-line. Do they both
work?

https://codereview.chromium.org/2496563002/diff/1/tools/resource_prefetch_pre...
tools/resource_prefetch_predictor/generate_database.py:98:
changer.ReplaceFlags(_CHROME_FLAGS)
I like the FlagReplacer contextmanager, getting the device stuck with some
random flags file because of an exception is .. a thing from the past .. the
past when we loved Telemetry

https://codereview.chromium.org/2496563002/diff/1/tools/resource_prefetch_pre...
tools/resource_prefetch_predictor/generate_database.py:134: logging.error('No
suitable device.')
nit: would be more informative to:

could not find device: options.device

[Benoit L, 2016-11-14 12:49:03]

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

[commit-bot: I haz the power, 2016-11-14 12:49:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Benoit L, 2016-11-14 12:49:25]

Thanks, it is better to re-use the tools/android/loading stuff after all.

PTAL.

https://codereview.chromium.org/2496563002/diff/1/tools/resource_prefetch_pre...
File tools/resource_prefetch_predictor/generate_database.py (right):

https://codereview.chromium.org/2496563002/diff/1/tools/resource_prefetch_pre...
tools/resource_prefetch_predictor/generate_database.py:31: import device_setup
On 2016/11/10 16:02:00, pasko wrote:
> do we need this dependency?

Done.

https://codereview.chromium.org/2496563002/diff/1/tools/resource_prefetch_pre...
tools/resource_prefetch_predictor/generate_database.py:42: # HTTP cache
generation, and introduce noise in loading performance.
On 2016/11/10 16:02:00, pasko wrote:
> this is getting repeated, should some sort of common function in
> tools/android/loading return the list?

Done.

https://codereview.chromium.org/2496563002/diff/1/tools/resource_prefetch_pre...
tools/resource_prefetch_predictor/generate_database.py:67:
parser.add_option('--repeats', help='Number of times each URL is loaded.',
On 2016/11/10 16:02:01, pasko wrote:
> nit: s/repeats/url-repeat/ sounds more consistent with similar params in
> telemetry
> 
> also, ambiguity: this seems to be about repeating working on each line in the
> file, not about times to load each unique url

Done.

https://codereview.chromium.org/2496563002/diff/1/tools/resource_prefetch_pre...
tools/resource_prefetch_predictor/generate_database.py:85: profile_dirs =
['app_chrome/Default', 'cache', 'app_chrome/ShaderCache',
On 2016/11/10 16:02:00, pasko wrote:
> Do you want to avoid dependency on tools/android/loading/controller.py? Why?

Acknowledged.

https://codereview.chromium.org/2496563002/diff/1/tools/resource_prefetch_pre...
tools/resource_prefetch_predictor/generate_database.py:97: device,
'/data/local/tmp/chrome-command-line')
On 2016/11/10 16:02:00, pasko wrote:
> the controller.py is doing it with /data/local/chrome-command-line. Do they
both
> work?

Done.

https://codereview.chromium.org/2496563002/diff/1/tools/resource_prefetch_pre...
tools/resource_prefetch_predictor/generate_database.py:98:
changer.ReplaceFlags(_CHROME_FLAGS)
On 2016/11/10 16:02:00, pasko wrote:
> I like the FlagReplacer contextmanager, getting the device stuck with some
> random flags file because of an exception is .. a thing from the past .. the
> past when we loved Telemetry

Acknowledged.

https://codereview.chromium.org/2496563002/diff/1/tools/resource_prefetch_pre...
tools/resource_prefetch_predictor/generate_database.py:134: logging.error('No
suitable device.')
On 2016/11/10 16:02:00, pasko wrote:
> nit: would be more informative to:
> 
> could not find device: options.device

Done.

[commit-bot: I haz the power, 2016-11-14 13:05:10]

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

[commit-bot: I haz the power, 2016-11-14 13:05:11]

Dry run: This issue passed the CQ dry run.

[pasko, 2016-11-14 13:21:27]

lgtm with a question

thank you!

https://codereview.chromium.org/2496563002/diff/40001/tools/android/loading/d...
File tools/android/loading/devtools_monitor.py (right):

https://codereview.chromium.org/2496563002/diff/40001/tools/android/loading/d...
tools/android/loading/devtools_monitor.py:275: current_time -
self._monitoring_start_timestamp).seconds
was it also assuming seconds before or something more fine-granular?

it it is rounded to seconds now, it might affect hypothetical noise to
hypothetical sandwich runs. WDYT?

[Benoit L, 2016-11-14 13:26:19]

https://codereview.chromium.org/2496563002/diff/40001/tools/android/loading/d...
File tools/android/loading/devtools_monitor.py (right):

https://codereview.chromium.org/2496563002/diff/40001/tools/android/loading/d...
tools/android/loading/devtools_monitor.py:275: current_time -
self._monitoring_start_timestamp).seconds
On 2016/11/14 13:21:27, pasko wrote:
> was it also assuming seconds before or something more fine-granular?
> 
> it it is rounded to seconds now, it might affect hypothetical noise to
> hypothetical sandwich runs. WDYT?

It was seconds as well, but couldn't handle non-integer scaling factors.

[Benoit L, 2016-11-14 13:26:28]

The CQ bit was checked by lizeb@chromium.org

[commit-bot: I haz the power, 2016-11-14 13:26:46]

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, 2016-11-14 13:32:28]

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

[commit-bot: I haz the power, 2016-11-14 13:32:29]

Failed to apply patch for tools/android/loading/controller.py:
While running git apply --index -p1;
  error: patch failed: tools/android/loading/controller.py:339
  error: tools/android/loading/controller.py: patch does not apply

Patch:       tools/android/loading/controller.py
Index: tools/android/loading/controller.py
diff --git a/tools/android/loading/controller.py
b/tools/android/loading/controller.py
index
97f87ba59abc72bc402e95f826e0a03566d7482b..bbd4799ac1470b9e264b4dc3caefd8cf570c215a
100644
--- a/tools/android/loading/controller.py
+++ b/tools/android/loading/controller.py
@@ -339,7 +339,7 @@ class RemoteChromeController(ChromeControllerBase):
       assert self._wpr_attributes.chrome_env_override == {}, \
           'Remote controller doesn\'t support chrome environment variables.'
     package_info = OPTIONS.ChromePackage()
-    command_line_path = '/data/local/chrome-command-line'
+    command_line_path = '/data/local/tmp/chrome-command-line'
     self._device.ForceStop(package_info.package)
     chrome_args = self._GetChromeArguments()
     logging.info('Launching %s with flags: %s' % (package_info.package,

[commit-bot: I haz the power, 2016-11-14 13:33:58]

Description was changed from

==========
tools: Add a tool to load pages on a device, and pull the predictor database.

BUG=655980
==========

to

==========
tools: Add a tool to load pages on a device, and pull the predictor database.

BUG=655980

Committed: https://crrev.com/1ad4f365b9917f92886d8e76c8e2789a9cb7e696
Cr-Commit-Position: refs/heads/master@{#431869}
==========

[commit-bot: I haz the power, 2016-11-14 13:33:59]

Patchset 3 (id:??) landed as
https://crrev.com/1ad4f365b9917f92886d8e76c8e2789a9cb7e696
Cr-Commit-Position: refs/heads/master@{#431869}