Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(118)

Issue 2923163007: Implemented telemetry benchmark that loads page and outputs screenshot. (Closed)

Created:
3 years, 6 months ago by lchoi
Modified:
3 years, 6 months ago
CC:
chromium-reviews, telemetry-reviews_chromium.org, chrishtr
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Implemented telemetry benchmark that loads page and outputs screenshot. Logic for '--disable-javascript' flag not implemented yet. BUG=536285 Review-Url: https://codereview.chromium.org/2923163007 Cr-Commit-Position: refs/heads/master@{#479558} Committed: https://chromium.googlesource.com/chromium/src/+/78f73931528e91c22e91f4b3b8f830c6ed86c9db

Patch Set 1 #

Total comments: 20

Patch Set 2 : Changed name to Screenshot, added logging, error handling, more descriptive comments #

Total comments: 7

Patch Set 3 : Moved files to tools/perf/contrib/cluster_telemetry, more logging, detecting if run on Linux in uniā€¦ #

Total comments: 10

Patch Set 4 : Removed mtime, saved_picture_count, javascript flag, Edited unittest #

Total comments: 3

Patch Set 5 : More detailed logging, unit test now returns if not Linux #

Total comments: 6

Patch Set 6 : Warning level logs, added decorators.enabled() to unit test #

Patch Set 7 : Unit test debug #

Patch Set 8 : Debugging #

Patch Set 9 : Edited gn_isolate_map #

Patch Set 10 : Edited chromium.fyi.json #

Patch Set 11 : Merge branch 'master' of https://chromium.googlesource.com/chromium/src into pixel-diff-screenshot #

Patch Set 12 : Edited chromium.linux.json #

Patch Set 13 : Edited chromium.linux.json #

Patch Set 14 : Changed screenshot_supported logic in inspector_backend #

Patch Set 15 : Rebase #

Patch Set 16 #

Patch Set 17 : Addex pixel assert to unit test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+139 lines, -0 lines) Patch
A tools/perf/contrib/cluster_telemetry/screenshot.py View 1 2 3 4 5 8 1 chunk +56 lines, -0 lines 0 comments Download
A tools/perf/contrib/cluster_telemetry/screenshot_ct.py View 1 2 3 1 chunk +42 lines, -0 lines 0 comments Download
A tools/perf/contrib/cluster_telemetry/screenshot_unittest.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +41 lines, -0 lines 0 comments Download

Messages

Total messages: 81 (55 generated)
lchoi
3 years, 6 months ago (2017-06-07 18:24:55 UTC) #3
wkorman
Exciting! My feedback is all essentially either to enhance comments or improve error handling. We ...
3 years, 6 months ago (2017-06-07 20:15:12 UTC) #4
nednguyen
Some high level comments https://codereview.chromium.org/2923163007/diff/1/tools/perf/contrib/cluster_telemetry/pixel_diff_ct.py File tools/perf/contrib/cluster_telemetry/pixel_diff_ct.py (right): https://codereview.chromium.org/2923163007/diff/1/tools/perf/contrib/cluster_telemetry/pixel_diff_ct.py#newcode13 tools/perf/contrib/cluster_telemetry/pixel_diff_ct.py:13: class PixelDiffCT(perf_benchmark.PerfBenchmark): This should inherit ...
3 years, 6 months ago (2017-06-07 20:32:13 UTC) #5
rmistry
https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pixel_diff.py File tools/perf/measurements/pixel_diff.py (right): https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pixel_diff.py#newcode12 tools/perf/measurements/pixel_diff.py:12: class PixelDiff(legacy_page_test.LegacyPageTest): On 2017/06/07 20:15:11, wkorman wrote: > Add ...
3 years, 6 months ago (2017-06-08 13:08:56 UTC) #7
lchoi
3 years, 6 months ago (2017-06-08 17:20:31 UTC) #8
wkorman
https://codereview.chromium.org/2923163007/diff/1/tools/perf/contrib/cluster_telemetry/pixel_diff_ct.py File tools/perf/contrib/cluster_telemetry/pixel_diff_ct.py (right): https://codereview.chromium.org/2923163007/diff/1/tools/perf/contrib/cluster_telemetry/pixel_diff_ct.py#newcode13 tools/perf/contrib/cluster_telemetry/pixel_diff_ct.py:13: class PixelDiffCT(perf_benchmark.PerfBenchmark): On 2017/06/07 20:32:12, nednguyen wrote: > This ...
3 years, 6 months ago (2017-06-08 17:57:53 UTC) #9
nednguyen
https://codereview.chromium.org/2923163007/diff/20001/tools/perf/measurements/screenshot.py File tools/perf/measurements/screenshot.py (right): https://codereview.chromium.org/2923163007/diff/20001/tools/perf/measurements/screenshot.py#newcode1 tools/perf/measurements/screenshot.py:1: # Copyright 2017 The Chromium Authors. All rights reserved. ...
3 years, 6 months ago (2017-06-08 18:15:19 UTC) #10
lchoi
3 years, 6 months ago (2017-06-08 20:05:13 UTC) #11
nednguyen
https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/cluster_telemetry/screenshot.py File tools/perf/contrib/cluster_telemetry/screenshot.py (right): https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/cluster_telemetry/screenshot.py#newcode21 tools/perf/contrib/cluster_telemetry/screenshot.py:21: # Javascript flag not yet implemented I suggest just ...
3 years, 6 months ago (2017-06-08 20:18:26 UTC) #12
lchoi
https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/cluster_telemetry/screenshot.py File tools/perf/contrib/cluster_telemetry/screenshot.py (right): https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/cluster_telemetry/screenshot.py#newcode64 tools/perf/contrib/cluster_telemetry/screenshot.py:64: if os.path.exists(outpath) and os.path.getmtime(outpath) > last_mod_time: On 2017/06/08 20:18:25, ...
3 years, 6 months ago (2017-06-08 20:38:34 UTC) #13
nednguyen
https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/cluster_telemetry/screenshot.py File tools/perf/contrib/cluster_telemetry/screenshot.py (right): https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/cluster_telemetry/screenshot.py#newcode64 tools/perf/contrib/cluster_telemetry/screenshot.py:64: if os.path.exists(outpath) and os.path.getmtime(outpath) > last_mod_time: On 2017/06/08 20:38:34, ...
3 years, 6 months ago (2017-06-08 21:39:26 UTC) #14
lchoi
3 years, 6 months ago (2017-06-08 23:10:47 UTC) #15
wkorman
https://codereview.chromium.org/2923163007/diff/60001/tools/perf/contrib/cluster_telemetry/screenshot.py File tools/perf/contrib/cluster_telemetry/screenshot.py (right): https://codereview.chromium.org/2923163007/diff/60001/tools/perf/contrib/cluster_telemetry/screenshot.py#newcode52 tools/perf/contrib/cluster_telemetry/screenshot.py:52: logging.info("Writing PNG file to %s", outpath) For nednguyen@: I ...
3 years, 6 months ago (2017-06-08 23:14:41 UTC) #16
nednguyen
https://codereview.chromium.org/2923163007/diff/60001/tools/perf/contrib/cluster_telemetry/screenshot.py File tools/perf/contrib/cluster_telemetry/screenshot.py (right): https://codereview.chromium.org/2923163007/diff/60001/tools/perf/contrib/cluster_telemetry/screenshot.py#newcode52 tools/perf/contrib/cluster_telemetry/screenshot.py:52: logging.info("Writing PNG file to %s", outpath) On 2017/06/08 23:14:41, ...
3 years, 6 months ago (2017-06-09 00:08:23 UTC) #17
lchoi
3 years, 6 months ago (2017-06-09 17:41:57 UTC) #18
wkorman
lgtm Just two log level fixes, otherwise LGTM https://codereview.chromium.org/2923163007/diff/80001/tools/perf/contrib/cluster_telemetry/screenshot.py File tools/perf/contrib/cluster_telemetry/screenshot.py (right): https://codereview.chromium.org/2923163007/diff/80001/tools/perf/contrib/cluster_telemetry/screenshot.py#newcode29 tools/perf/contrib/cluster_telemetry/screenshot.py:29: logging.info("WaitForDocumentReadyStateToBeComplete() ...
3 years, 6 months ago (2017-06-09 17:59:54 UTC) #19
nednguyen
lgtm with some nits. However, Ravis is the directory OWNER and you want to wait ...
3 years, 6 months ago (2017-06-09 18:11:09 UTC) #20
lchoi
https://codereview.chromium.org/2923163007/diff/80001/tools/perf/contrib/cluster_telemetry/screenshot_unittest.py File tools/perf/contrib/cluster_telemetry/screenshot_unittest.py (right): https://codereview.chromium.org/2923163007/diff/80001/tools/perf/contrib/cluster_telemetry/screenshot_unittest.py#newcode36 tools/perf/contrib/cluster_telemetry/screenshot_unittest.py:36: self.assertTrue(os.access(path, os.R_OK)) On 2017/06/09 18:11:09, nednguyen wrote: > In ...
3 years, 6 months ago (2017-06-09 18:16:51 UTC) #21
nednguyen
https://codereview.chromium.org/2923163007/diff/80001/tools/perf/contrib/cluster_telemetry/screenshot_unittest.py File tools/perf/contrib/cluster_telemetry/screenshot_unittest.py (right): https://codereview.chromium.org/2923163007/diff/80001/tools/perf/contrib/cluster_telemetry/screenshot_unittest.py#newcode36 tools/perf/contrib/cluster_telemetry/screenshot_unittest.py:36: self.assertTrue(os.access(path, os.R_OK)) On 2017/06/09 18:16:51, lchoi wrote: > On ...
3 years, 6 months ago (2017-06-09 18:22:59 UTC) #22
rmistry
lgtm
3 years, 6 months ago (2017-06-09 18:40:10 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2923163007/100001
3 years, 6 months ago (2017-06-09 18:57:00 UTC) #26
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/475684)
3 years, 6 months ago (2017-06-09 20:43:59 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2923163007/120001
3 years, 6 months ago (2017-06-10 00:09:10 UTC) #31
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/404153)
3 years, 6 months ago (2017-06-10 01:29:52 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2923163007/310001
3 years, 6 months ago (2017-06-15 00:22:50 UTC) #77
commit-bot: I haz the power
3 years, 6 months ago (2017-06-15 00:41:13 UTC) #80
Message was sent while issue was closed.
Committed patchset #17 (id:310001) as
https://chromium.googlesource.com/chromium/src/+/78f73931528e91c22e91f4b3b8f8...

Powered by Google App Engine
This is Rietveld 408576698