Adding media browsing benchmark.

Adding media browsing benchmark.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq

[Hannes Payer (out of office), 2016-07-05 13:51:04]

Description was changed from

==========
lines


lines


lines


lines


lines


ulan change


remove print


Adding media browsing benchmark.

BUG=
==========

to

==========
lines


lines


lines


lines


lines


ulan change


remove print


Adding media browsing benchmark.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

[Hannes Payer (out of office), 2016-07-05 13:51:39]

Description was changed from

==========
lines


lines


lines


lines


lines


ulan change


remove print


Adding media browsing benchmark.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

to

==========
Adding media browsing benchmark.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

[Hannes Payer (out of office), 2016-07-05 13:58:03]

hpayer@chromium.org changed reviewers:
+ petrcermak@chromium.org, primiano@chromium.org, ulan@chromium.org

[Hannes Payer (out of office), 2016-07-05 13:58:04]

petrcermak@, primiano@, similar to the news reading workflow
(https://codereview.chromium.org/2118293002).

Interesting for system health?

[sullivan, 2016-07-06 12:17:50]

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

[sullivan, 2016-07-06 12:17:50]

+charliea whose team is adding a lot of media system health benchmarks for power

[petrcermak, 2016-07-07 15:04:44]

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

[petrcermak, 2016-07-07 15:04:45]

My comments are exactly the same as in
https://codereview.chromium.org/2118293002/ (including inline comments):

Looks good overall. As I explained in my inline comments (in
https://codereview.chromium.org/2118293002/), I think that the stories should be
added to the system_health folder straightaway.

Please add a bug to the patch description. If you don't have one, please use
589726. In fact, even if you have your own bug, please add 589726 to the BUG=
line.

sullivan: Will the infrastructure be able to handle the extra benchmark?

+nednguyen

Thanks,
Petr

https://codereview.chromium.org/2124823002/diff/40001/tools/perf/benchmarks/b...
File tools/perf/benchmarks/browse_media.py (right):

https://codereview.chromium.org/2124823002/diff/40001/tools/perf/benchmarks/b...
tools/perf/benchmarks/browse_media.py:2: # Use of this source code is governed
by a BSD-style license that can be
This file is pretty much the same as news.py in
https://codereview.chromium.org/2118293002/. Could you please avoid code
duplication by re-using the code across the two benchmarks (perhaps they could
even be defined in the same file given how similar they are)?

https://codereview.chromium.org/2124823002/diff/40001/tools/perf/benchmarks/b...
tools/perf/benchmarks/browse_media.py:79: class
MobileBrowseMediaBenchmark(_BrowseMediaBenchmark):
nednguyen: I can see many benchmarks having to do this (define a mobile- and
desktop-specific subclass). Is there any way we could simplify things?

idea: if it were possible to figure out the current platform in CreateStorySet,
all of this would be unnecessary:

class BrowseMediaBenchmark(perf_benchmark.PerfBenchmack):

  ...

  def CreateStorySet(self, finder_options):
    if finder_options.somehow.figure.out.the.platform.GetDeviceTypeName() ==
'Desktop':
      return page_sets.DesktopBrowseMediaStorySet()
    else:
      return page_sets.MobileBrowseMediaStorySet()

  ...

WDYT?

[sullivan, 2016-07-08 20:53:23]

On 2016/07/07 15:04:45, petrcermak wrote:
> My comments are exactly the same as in
> https://codereview.chromium.org/2118293002/ (including inline comments):
> 
> Looks good overall. As I explained in my inline comments (in
> https://codereview.chromium.org/2118293002/), I think that the stories should
be
> added to the system_health folder straightaway.
> 
> Please add a bug to the patch description. If you don't have one, please use
> 589726. In fact, even if you have your own bug, please add 589726 to the BUG=
> line.
> 
> sullivan: Will the infrastructure be able to handle the extra benchmark?

Our infra can handle the extra benchmark, but I'd prefer that:

1) There is some central place for all the system health user stories listed, so
they're shareable between different metrics
2) Specifically for this benchmark, we understand the overlap between this and
the large set of media cases the power system health benchmarks that charliea is
working on.

> +nednguyen
> 
> Thanks,
> Petr
> 
>
https://codereview.chromium.org/2124823002/diff/40001/tools/perf/benchmarks/b...
> File tools/perf/benchmarks/browse_media.py (right):
> 
>
https://codereview.chromium.org/2124823002/diff/40001/tools/perf/benchmarks/b...
> tools/perf/benchmarks/browse_media.py:2: # Use of this source code is governed
> by a BSD-style license that can be
> This file is pretty much the same as news.py in
> https://codereview.chromium.org/2118293002/. Could you please avoid code
> duplication by re-using the code across the two benchmarks (perhaps they could
> even be defined in the same file given how similar they are)?
> 
>
https://codereview.chromium.org/2124823002/diff/40001/tools/perf/benchmarks/b...
> tools/perf/benchmarks/browse_media.py:79: class
> MobileBrowseMediaBenchmark(_BrowseMediaBenchmark):
> nednguyen: I can see many benchmarks having to do this (define a mobile- and
> desktop-specific subclass). Is there any way we could simplify things?
> 
> idea: if it were possible to figure out the current platform in
CreateStorySet,
> all of this would be unnecessary:
> 
> class BrowseMediaBenchmark(perf_benchmark.PerfBenchmack):
> 
>   ...
> 
>   def CreateStorySet(self, finder_options):
>     if finder_options.somehow.figure.out.the.platform.GetDeviceTypeName() ==
> 'Desktop':
>       return page_sets.DesktopBrowseMediaStorySet()
>     else:
>       return page_sets.MobileBrowseMediaStorySet()
> 
>   ...
> 
> WDYT?

[petrcermak, 2016-07-11 09:22:40]

On 2016/07/08 20:53:23, sullivan wrote:
> On 2016/07/07 15:04:45, petrcermak wrote:
> > My comments are exactly the same as in
> > https://codereview.chromium.org/2118293002/ (including inline comments):
> > 
> > Looks good overall. As I explained in my inline comments (in
> > https://codereview.chromium.org/2118293002/), I think that the stories
should
> be
> > added to the system_health folder straightaway.
> > 
> > Please add a bug to the patch description. If you don't have one, please use
> > 589726. In fact, even if you have your own bug, please add 589726 to the
BUG=
> > line.
> > 
> > sullivan: Will the infrastructure be able to handle the extra benchmark?
> 
> Our infra can handle the extra benchmark, but I'd prefer that:
> 
> 1) There is some central place for all the system health user stories listed,
so
> they're shareable between different metrics

This is being addressed by ulan@. The stories are moving to the system_health
folder (https://codereview.chromium.org/2118293002/).

> 2) Specifically for this benchmark, we understand the overlap between this and
> the large set of media cases the power system health benchmarks that charliea
is
> working on.

I defer this to Charlie.

[petrcermak, 2016-07-19 09:38:49]

ping

[Hannes Payer (out of office), 2016-07-21 16:40:16]

On 2016/07/19 09:38:49, petrcermak wrote:
> ping

This CL moved to https://codereview.chromium.org/2168743004/ after re-basing
with https://codereview.chromium.org/2118293002.