Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2866703004:
Merge tough_video_cases_extra into tough_video_cases. (Closed)
|
Created:
3 years, 7 months ago by CalebRouleau Modified:
3 years, 7 months ago CC:
chromium-reviews, posciak+watch_chromium.org, telemetry-reviews_chromium.org, martiniss Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMerge tough_video_cases_extra into tough_video_cases.
Fix duplicate display name conflict by adding &seek to end of every url
for a seeking Page.
Delete ChromeOS-specific benchmarks as well since they are not being
used anymore (crbug/709161). I would recommend CrOS team to simply use
our normal media.tough_video_cases benchmark in the future instead of
creating their own. This is more reasonable now because of the huge
reduction in runtime of tough_video_cases (crbug/710253).
I also re-organized benchmarks/media.py to put related stuff close together.
BUG=709161, 713335, 565180
Patch Set 1 #Patch Set 2 : Add tag for normal_play. #
Total comments: 2
Patch Set 3 : respond to feedback. #Patch Set 4 : respond to feedback. #Patch Set 5 : merge #
Created: 3 years, 7 months ago
(Patch set is too large to download)
Messages
Total messages: 31 (12 generated)
Description was changed from ========== Merge tough_video_cases_extra into tough_video_cases. Fix duplicate display name conflict by adding &seek to end of every url for a seeking Page. Delete ChromeOS-specific benchmarks as well since they are not being used anymore (crbug/709161). I would recommend CrOS team to simply use our normal media.tough_video_cases benchmark in the future instead of creating their own. This is more reasonable now because of the huge reduction in runtime of tough_video_cases (crbug/710253). BUG=709161,713335,565180 ========== to ========== Merge tough_video_cases_extra into tough_video_cases. Fix duplicate display name conflict by adding &seek to end of every url for a seeking Page. Delete ChromeOS-specific benchmarks as well since they are not being used anymore (crbug/709161). I would recommend CrOS team to simply use our normal media.tough_video_cases benchmark in the future instead of creating their own. This is more reasonable now because of the huge reduction in runtime of tough_video_cases (crbug/710253). I also re-organized benchmarks/media.py to put related stuff close together. BUG=709161,713335,565180 ==========
crouleau@chromium.org changed reviewers: + johnchen@chromium.org, laszio@chromium.org, nednguyen@google.com
PTAL
lgtm but can you block landing this Cl on https://chromium-review.googlesource.com/c/486267/? Once https://chromium-review.googlesource.com/c/486267/, we can manually reshard the benchmark to avoid that massive change to testing/buildbot/chromium.perf.json https://codereview.chromium.org/2866703004/diff/20001/tools/perf/benchmarks/m... File tools/perf/benchmarks/media.py (right): https://codereview.chromium.org/2866703004/diff/20001/tools/perf/benchmarks/m... tools/perf/benchmarks/media.py:68: if possible_browser.platform.GetOSName() != "android": can you refactor 68 & 69 to "@benchmark.Disabled('android')". This is because if it's specified through decorator, we have some logic to not even scheduled this on the desktop platform.
lgtm
On 2017/05/05 at 21:38:50, nednguyen wrote: > lgtm but can you block landing this Cl on https://chromium-review.googlesource.com/c/486267/? > > Once https://chromium-review.googlesource.com/c/486267/, we can manually reshard the benchmark to avoid that massive change to testing/buildbot/chromium.perf.json This CL has landed. Can you re-run //tools/perf/generate_perf_data and upload the CL then? There should be much smaller changes to the perf json files. > > https://codereview.chromium.org/2866703004/diff/20001/tools/perf/benchmarks/m... > File tools/perf/benchmarks/media.py (right): > > https://codereview.chromium.org/2866703004/diff/20001/tools/perf/benchmarks/m... > tools/perf/benchmarks/media.py:68: if possible_browser.platform.GetOSName() != "android": > can you refactor 68 & 69 to "@benchmark.Disabled('android')". This is because if it's specified through decorator, we have some logic to not even scheduled this on the desktop platform.
lgtm
On 2017/05/05 23:52:25, martiniss wrote: > On 2017/05/05 at 21:38:50, nednguyen wrote: > > lgtm but can you block landing this Cl on > https://chromium-review.googlesource.com/c/486267/ > > > > Once https://chromium-review.googlesource.com/c/486267/, we can manually > reshard the benchmark to avoid that massive change to > testing/buildbot/chromium.perf.json > > This CL has landed. Can you re-run //tools/perf/generate_perf_data and upload > the CL then? There should be much smaller changes to the perf json files. > > > > > https://codereview.chromium.org/2866703004/diff/20001/tools/perf/benchmarks/m... > > File tools/perf/benchmarks/media.py (right): > > > > > https://codereview.chromium.org/2866703004/diff/20001/tools/perf/benchmarks/m... > > tools/perf/benchmarks/media.py:68: if possible_browser.platform.GetOSName() != > "android": > > can you refactor 68 & 69 to https://mail.google.com/mail/?view=cm&fs=1&tf=1&to="@benchmark.Disabled(. This is because > if it's specified through decorator, we have some logic to not even scheduled > this on the desktop platform. Not sure whether I improve it or not...
https://codereview.chromium.org/2866703004/diff/20001/tools/perf/benchmarks/m... File tools/perf/benchmarks/media.py (right): https://codereview.chromium.org/2866703004/diff/20001/tools/perf/benchmarks/m... tools/perf/benchmarks/media.py:68: if possible_browser.platform.GetOSName() != "android": On 2017/05/05 21:38:50, nednguyen wrote: > can you refactor 68 & 69 to https://mail.google.com/mail/?view=cm&fs=1&tf=1&to="@benchmark.Disabled(. This is because if > it's specified through decorator, we have some logic to not even scheduled this > on the desktop platform. Done.
The CQ bit was checked by crouleau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from johnchen@chromium.org, nednguyen@google.com, laszio@google.com Link to the patchset: https://codereview.chromium.org/2866703004/#ps40001 (title: "respond to feedback.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by crouleau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from johnchen@chromium.org, nednguyen@google.com, laszio@google.com Link to the patchset: https://codereview.chromium.org/2866703004/#ps50001 (title: "respond to feedback.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/06 15:30:26, commit-bot: I haz the power wrote: > CQ is trying da patch. > > Follow status at: > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by crouleau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from johnchen@chromium.org, nednguyen@google.com, laszio@google.com Link to the patchset: https://codereview.chromium.org/2866703004/#ps60001 (title: "merge")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/05/09 01:39:20, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) You need to re-upload this CL using --gerrit, I think.
On 2017/05/09 13:06:43, nednguyen wrote: > On 2017/05/09 01:39:20, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > You need to re-upload this CL using --gerrit, I think. Dirk: does it makes sense to tell user to use --gerrit when rietveld cannot deal with the size of the patch?
nednguyen@google.com changed reviewers: + dpranke@chromium.org
On 2017/05/09 13:07:14, nednguyen wrote: > On 2017/05/09 13:06:43, nednguyen wrote: > > On 2017/05/09 01:39:20, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > > > You need to re-upload this CL using --gerrit, I think. > > Dirk: does it makes sense to tell user to use --gerrit when rietveld cannot deal > with the size of the patch? Okay, trying: https://chromium-review.googlesource.com/c/499989/ please LGTM so I can try it.
On 2017/05/09 16:31:24, CalebRouleau wrote: > On 2017/05/09 13:07:14, nednguyen wrote: > > On 2017/05/09 13:06:43, nednguyen wrote: > > > On 2017/05/09 01:39:20, commit-bot: I haz the power wrote: > > > > Try jobs failed on following builders: > > > > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > > > > > You need to re-upload this CL using --gerrit, I think. > > > > Dirk: does it makes sense to tell user to use --gerrit when rietveld cannot > deal > > with the size of the patch? Sure, if that's the problem.
On 2017/05/09 19:55:29, Dirk Pranke wrote: > On 2017/05/09 16:31:24, CalebRouleau wrote: > > On 2017/05/09 13:07:14, nednguyen wrote: > > > On 2017/05/09 13:06:43, nednguyen wrote: > > > > On 2017/05/09 01:39:20, commit-bot: I haz the power wrote: > > > > > Try jobs failed on following builders: > > > > > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > > > > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > > > > > > > You need to re-upload this CL using --gerrit, I think. > > > > > > Dirk: does it makes sense to tell user to use --gerrit when rietveld cannot > > deal > > > with the size of the patch? > > Sure, if that's the problem. Thanks, I filed https://bugs.chromium.org/p/chromium/issues/detail?id=720156 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
