[Telemetry] Allows concatenating multiple Disabled & Enabled decorators

[Telemetry] Allows concatenating multiple Disabled & Enabled decorators.

For this to work, we also abandon the "@Disabled" &
"@Disabled()" syntax, which used to mean unconditionally
disable. The places that use such syntax are switched
to @Disabled('all').
The reason for this is to simplify the handling logic of
combining multiple decorators & make the unconditional
@Disabled(..) clearer.

BUG=552477
Committed: https://crrev.com/f83d36c71ece47e23fac753b42437148090636c1
Cr-Commit-Position: refs/heads/master@{#358457}

[nednguyen, 2015-11-06 17:28:52]

nednguyen@google.com changed reviewers:
+ aiolos@chromium.org, sullivan@chromium.org

[eakuefner, 2015-11-06 17:32:42]

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

[eakuefner, 2015-11-06 17:32:43]

https://codereview.chromium.org/1428133005/diff/1/tools/telemetry/telemetry/d...
File tools/telemetry/telemetry/decorators.py (right):

https://codereview.chromium.org/1428133005/diff/1/tools/telemetry/telemetry/d...
tools/telemetry/telemetry/decorators.py:114: if not hasattr(func,
'_disabled_strings'):
Can you find a way to share this update algorithm between the decorators?

[nednguyen, 2015-11-06 18:23:50]

https://codereview.chromium.org/1428133005/diff/1/tools/telemetry/telemetry/d...
File tools/telemetry/telemetry/decorators.py (right):

https://codereview.chromium.org/1428133005/diff/1/tools/telemetry/telemetry/d...
tools/telemetry/telemetry/decorators.py:114: if not hasattr(func,
'_disabled_strings'):
On 2015/11/06 17:32:43, eakuefner wrote:
> Can you find a way to share this update algorithm between the decorators?

Code sharing these make it pretty messy because of the "hasattr" & "setattr". So
in this particular case, code sharing does make it harder to maintain the code.

[aiolos (Not reviewing), 2015-11-06 18:37:39]

https://codereview.chromium.org/1428133005/diff/20001/tools/perf/benchmarks/b...
File tools/perf/benchmarks/benchmark_smoke_unittest.py (right):

https://codereview.chromium.org/1428133005/diff/20001/tools/perf/benchmarks/b...
tools/perf/benchmarks/benchmark_smoke_unittest.py:146: if (getattr(method,
attribute, None) or
Shouldn't this be 

if (not getattr(...) or
    not getattr(...)):

[nednguyen, 2015-11-06 18:47:25]

https://codereview.chromium.org/1428133005/diff/20001/tools/perf/benchmarks/b...
File tools/perf/benchmarks/benchmark_smoke_unittest.py (right):

https://codereview.chromium.org/1428133005/diff/20001/tools/perf/benchmarks/b...
tools/perf/benchmarks/benchmark_smoke_unittest.py:146: if (getattr(method,
attribute, None) or
On 2015/11/06 18:37:39, aiolos wrote:
> Shouldn't this be 
> 
> if (not getattr(...) or
>     not getattr(...)):

Done. Good catch

[nednguyen, 2015-11-06 19:02:49]

The CQ bit was checked by nednguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-06 19:05:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428133005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428133005/40001

[aiolos (Not reviewing), 2015-11-06 19:10:12]

lgtm

[eakuefner, 2015-11-06 19:22:38]

lgtm

[nednguyen, 2015-11-06 19:48:00]

The CQ bit was unchecked by nednguyen@google.com

[nednguyen, 2015-11-06 19:48:01]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2015-11-06 19:55:35]

Description was changed from

==========
[Telemetry] Allows concatenating multiple Disabled & Enabled decorators

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect
==========

to

==========
[Telemetry] Allows concatenating multiple Disabled & Enabled decorators

BUG=552477
==========

[nednguyen, 2015-11-06 19:55:40]

The CQ bit was unchecked by nednguyen@google.com

[nednguyen, 2015-11-06 19:55:46]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2015-11-06 19:59:38]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2015-11-06 19:59:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
eakuefner@chromium.org, aiolos@chromium.org
Link to the patchset: https://codereview.chromium.org/1428133005/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2015-11-06 20:22:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428133005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428133005/60001

[commit-bot: I haz the power, 2015-11-06 21:22:26]

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

[commit-bot: I haz the power, 2015-11-06 21:22:27]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[nednguyen, 2015-11-06 22:01:29]

Description was changed from

==========
[Telemetry] Allows concatenating multiple Disabled & Enabled decorators

BUG=552477
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect
==========

to

==========
[Telemetry] Allows concatenating multiple Disabled & Enabled decorators.

For this to work, we also disable the "@Disabled" & 
"@Disabled()" semantic, which used to mean unconditionally 
disable. Those places that used such semantic are switched 
to @Disabled('all').
The reason for this is to simplify the handling logic of 
combining multiple decorators & make the unconditional 
@Disabled(..) clearer.

BUG=552477
==========

[nednguyen, 2015-11-06 22:02:18]

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

[nednguyen, 2015-11-06 22:03:29]

On 2015/11/06 22:02:19, nednguyen wrote:

There has been some substantial change, PTAL again.

[nednguyen, 2015-11-06 22:05:45]

Description was changed from

==========
[Telemetry] Allows concatenating multiple Disabled & Enabled decorators.

For this to work, we also disable the "@Disabled" & 
"@Disabled()" semantic, which used to mean unconditionally 
disable. Those places that used such semantic are switched 
to @Disabled('all').
The reason for this is to simplify the handling logic of 
combining multiple decorators & make the unconditional 
@Disabled(..) clearer.

BUG=552477
==========

to

==========
[Telemetry] Allows concatenating multiple Disabled & Enabled decorators.

For this to work, we also abandon the "@Disabled" & 
"@Disabled()" syntax, which used to mean unconditionally 
disable. Those places that used such semantic are switched 
to @Disabled('all').
The reason for this is to simplify the handling logic of 
combining multiple decorators & make the unconditional 
@Disabled(..) clearer.

BUG=552477
==========

[nednguyen, 2015-11-06 22:05:59]

Description was changed from

==========
[Telemetry] Allows concatenating multiple Disabled & Enabled decorators.

For this to work, we also abandon the "@Disabled" & 
"@Disabled()" syntax, which used to mean unconditionally 
disable. Those places that used such semantic are switched 
to @Disabled('all').
The reason for this is to simplify the handling logic of 
combining multiple decorators & make the unconditional 
@Disabled(..) clearer.

BUG=552477
==========

to

==========
[Telemetry] Allows concatenating multiple Disabled & Enabled decorators.

For this to work, we also abandon the "@Disabled" & 
"@Disabled()" syntax, which used to mean unconditionally 
disable. Those places that used such syntax are switched 
to @Disabled('all').
The reason for this is to simplify the handling logic of 
combining multiple decorators & make the unconditional 
@Disabled(..) clearer.

BUG=552477
==========

[nednguyen, 2015-11-06 22:06:12]

Description was changed from

==========
[Telemetry] Allows concatenating multiple Disabled & Enabled decorators.

For this to work, we also abandon the "@Disabled" & 
"@Disabled()" syntax, which used to mean unconditionally 
disable. Those places that used such syntax are switched 
to @Disabled('all').
The reason for this is to simplify the handling logic of 
combining multiple decorators & make the unconditional 
@Disabled(..) clearer.

BUG=552477
==========

to

==========
[Telemetry] Allows concatenating multiple Disabled & Enabled decorators.

For this to work, we also abandon the "@Disabled" & 
"@Disabled()" syntax, which used to mean unconditionally 
disable. The places that use such syntax are switched 
to @Disabled('all').
The reason for this is to simplify the handling logic of 
combining multiple decorators & make the unconditional 
@Disabled(..) clearer.

BUG=552477
==========

[eakuefner, 2015-11-06 22:06:23]

lgtm % test

https://codereview.chromium.org/1428133005/diff/70001/tools/telemetry/telemet...
File tools/telemetry/telemetry/decorators_unittest.py (right):

https://codereview.chromium.org/1428133005/diff/70001/tools/telemetry/telemet...
tools/telemetry/telemetry/decorators_unittest.py:36: class
TestDisableDecorators(unittest.TestCase):
Please add test coverage for 'all'

[eakuefner, 2015-11-06 22:07:58]

https://codereview.chromium.org/1428133005/diff/70001/tools/telemetry/telemet...
File tools/telemetry/telemetry/testing/disabled_cases.py (right):

https://codereview.chromium.org/1428133005/diff/70001/tools/telemetry/telemet...
tools/telemetry/telemetry/testing/disabled_cases.py:17:
@decorators.Disabled('all')
Actually this is fine, lgtm

[dtu, 2015-11-06 22:16:02]

https://codereview.chromium.org/1428133005/diff/70001/tools/telemetry/telemet...
File tools/telemetry/telemetry/decorators.py (right):

https://codereview.chromium.org/1428133005/diff/70001/tools/telemetry/telemet...
tools/telemetry/telemetry/decorators.py:103: @Disabled  # Unconditionally
disabled.
Update doc.

https://codereview.chromium.org/1428133005/diff/70001/tools/telemetry/telemet...
tools/telemetry/telemetry/decorators.py:120: func(*args, **kwargs)
This wrapper doesn't do anything - why not just add these attributes to the
original function/class?

https://codereview.chromium.org/1428133005/diff/70001/tools/telemetry/telemet...
tools/telemetry/telemetry/decorators.py:125: assert args, '@Disabled(..)
requires arguments'
This check doesn't do anything - if no args are passed, the argument is the
function or class being decorated.

https://codereview.chromium.org/1428133005/diff/70001/tools/telemetry/telemet...
tools/telemetry/telemetry/decorators.py:189: def IsEnabled(test,
possible_browser):
lol why does this function exist!?!?!?

https://codereview.chromium.org/1428133005/diff/70001/tools/telemetry/telemet...
tools/telemetry/telemetry/decorators.py:237: return False, None  # No arguments
to @Enabled means always enable.
Update comment.

[nednguyen, 2015-11-06 22:28:46]

https://codereview.chromium.org/1428133005/diff/70001/tools/telemetry/telemet...
File tools/telemetry/telemetry/decorators.py (right):

https://codereview.chromium.org/1428133005/diff/70001/tools/telemetry/telemet...
tools/telemetry/telemetry/decorators.py:103: @Disabled  # Unconditionally
disabled.
On 2015/11/06 22:16:02, dtu wrote:
> Update doc.

Done.

https://codereview.chromium.org/1428133005/diff/70001/tools/telemetry/telemet...
tools/telemetry/telemetry/decorators.py:120: func(*args, **kwargs)
On 2015/11/06 22:16:02, dtu wrote:
> This wrapper doesn't do anything - why not just add these attributes to the
> original function/class?

Good call, this simplifies things a lot.

https://codereview.chromium.org/1428133005/diff/70001/tools/telemetry/telemet...
tools/telemetry/telemetry/decorators.py:125: assert args, '@Disabled(..)
requires arguments'
On 2015/11/06 22:16:02, dtu wrote:
> This check doesn't do anything - if no args are passed, the argument is the
> function or class being decorated.

Done.

https://codereview.chromium.org/1428133005/diff/70001/tools/telemetry/telemet...
tools/telemetry/telemetry/decorators.py:189: def IsEnabled(test,
possible_browser):
On 2015/11/06 22:16:02, dtu wrote:
> lol why does this function exist!?!?!?

Left TODO()

https://codereview.chromium.org/1428133005/diff/70001/tools/telemetry/telemet...
tools/telemetry/telemetry/decorators.py:237: return False, None  # No arguments
to @Enabled means always enable.
On 2015/11/06 22:16:02, dtu wrote:
> Update comment.

Done.

[nednguyen, 2015-11-06 22:29:53]

Description was changed from

==========
[Telemetry] Allows concatenating multiple Disabled & Enabled decorators.

For this to work, we also abandon the "@Disabled" & 
"@Disabled()" syntax, which used to mean unconditionally 
disable. The places that use such syntax are switched 
to @Disabled('all').
The reason for this is to simplify the handling logic of 
combining multiple decorators & make the unconditional 
@Disabled(..) clearer.

BUG=552477
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect
==========

to

==========
[Telemetry] Allows concatenating multiple Disabled & Enabled decorators.

For this to work, we also abandon the "@Disabled" & 
"@Disabled()" syntax, which used to mean unconditionally 
disable. The places that use such syntax are switched 
to @Disabled('all').
The reason for this is to simplify the handling logic of 
combining multiple decorators & make the unconditional 
@Disabled(..) clearer.

BUG=552477
==========

[dtu, 2015-11-06 22:40:52]

lgtm, thanks :)

[nednguyen, 2015-11-06 22:41:15]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2015-11-06 22:41:15]

The patchset sent to the CQ was uploaded after l-g-t-m from aiolos@chromium.org,
eakuefner@chromium.org
Link to the patchset: https://codereview.chromium.org/1428133005/#ps90001
(title: "Address Dave's comment")

[commit-bot: I haz the power, 2015-11-06 22:42:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428133005/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428133005/90001

[commit-bot: I haz the power, 2015-11-07 00:06:12]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2015-11-07 00:08:47]

Patchset 6 (id:??) landed as
https://crrev.com/f83d36c71ece47e23fac753b42437148090636c1
Cr-Commit-Position: refs/heads/master@{#358457}