Fix private style in ct-commit-data.html.

Fix private style in ct-commit-data.html.

Also, while at it, add a basic test to make sure this code executes
correctly.

NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177804

[ojan, 2014-07-10 02:58:57]

This was the only instance of a post-fixed underscore in the garden-o-matic
code.

[michaelpg, 2014-07-10 03:02:37]

So we're committing to this style? The Google JS style guide uses trailing
underscores for "private" members, and all of Chromium follows that.


On Wed, Jul 9, 2014 at 7:58 PM, <ojan@chromium.org> wrote:

> Reviewers: abarth, Michael Giuffrida, esprehn,
>
> Message:
> This was the only instance of a post-fixed underscore in the garden-o-matic
> code.
>
> Description:
> Fix private style in ct-commit-data.html.
>
> Also, while at it, add a basic test to make sure this code executes
> correctly.
>
> NOTRY=true
>
> Please review this at https://codereview.chromium.org/380983002/
>
> SVN Base: svn://svn.chromium.org/blink/trunk
>
> Affected files (+46, -3 lines):
>   M Tools/GardeningServer/run-unittests.html
>   M Tools/GardeningServer/ui/ct-commit-data.html
>   A Tools/GardeningServer/ui/ct-commit-data-tests.html
>
>
> Index: Tools/GardeningServer/run-unittests.html
> diff --git a/Tools/GardeningServer/run-unittests.html
> b/Tools/GardeningServer/run-unittests.html
> index 9ff4e7e2b60b16337f9125ec212e2c8002bc56fc..
> 885dacc75703dfa463e1f122fcab05e9ea031f8f 100644
> --- a/Tools/GardeningServer/run-unittests.html
> +++ b/Tools/GardeningServer/run-unittests.html
> @@ -76,6 +76,7 @@ THE POSSIBILITY OF SUCH DAMAGE.
>  <link rel="import" href="polymer-load-warning.html">
>  <link rel="import" href="ui/ct-builder-grid-tests.html">
>  <link rel="import" href="ui/ct-builder-tests.html">
> +<link rel="import" href="ui/ct-commit-data-tests.html">
>  <link rel="import" href="ui/ct-commit-tests.html">
>  <link rel="import" href="ui/ct-failure-card-tests.html">
>  <link rel="import" href="ui/ct-failure-grouper-tests.html">
> Index: Tools/GardeningServer/ui/ct-commit-data-tests.html
> diff --git a/Tools/GardeningServer/ui/ct-commit-data-tests.html
> b/Tools/GardeningServer/ui/ct-commit-data-tests.html
> new file mode 100644
> index 0000000000000000000000000000000000000000..
> 8c23af0b8505f70e878a0d3804efac9d2be02711
> --- /dev/null
> +++ b/Tools/GardeningServer/ui/ct-commit-data-tests.html
> @@ -0,0 +1,42 @@
> +<!--
> +Copyright 2014 The Chromium Authors. All rights reserved.
> +Use of this source code is governed by a BSD-style license that can be
> +found in the LICENSE file.
> +-->
> +
> +<link rel="import" href="ct-commit-data.html">
> +
> +<script>
> +(function () {
> +
> +module("ct-commit-data");
> +
> +asyncTest("basic", 2, function() {
> +  // FIXME: ct-commit-data should be self-contained instead of depending
> on this static function.
> +  var oldCommitDataListForRevisionRange = model.
> commitDataListForRevisionRange;
> +  var reset = function() {
> +    model.commitDataListForRevisionRange = oldCommitDataListForRevisionRa
> nge;
> +  }
> +
> +  try {
> +    model.commitDataListForRevisionRange = function(first, last) {
> return [first + ':' + last]; };
> +
> +    var commitData = document.createElement('ct-commit-data');
> +
> +    commitData.first = 1;
> +    commitData.last = 2;
> +
> +    commitData.async(function() {
> +      reset();
> +      equal(commitData.data.length, 1);
> +      equal(commitData.data[0], '1:2');
> +      start();
> +    });
> +  } catch(e) {
> +    reset();
> +  }
> +
> +});
> +
> +})()
> +</script>
> Index: Tools/GardeningServer/ui/ct-commit-data.html
> diff --git a/Tools/GardeningServer/ui/ct-commit-data.html
> b/Tools/GardeningServer/ui/ct-commit-data.html
> index b976b5279877ca2cc49dee82b182e0e64acee259..
> f91e9cf660fcd6203d499e74848e67e4a8658c6d 100644
> --- a/Tools/GardeningServer/ui/ct-commit-data.html
> +++ b/Tools/GardeningServer/ui/ct-commit-data.html
> @@ -12,14 +12,14 @@ found in the LICENSE file.
>        last: 0,
>
>        firstChanged: function() {
> -        this.update_();
> +        this._update();
>        },
>
>        lastChanged: function() {
> -        this.update_();
> +        this._update();
>        },
>
> -      update_: function() {
> +      _update: function() {
>          this.data = model.commitDataListForRevisionRange(this.first,
> this.last);
>        },
>      });
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[ojan, 2014-07-10 03:09:16]

On 2014/07/10 at 03:02:37, michaelpg wrote:
> So we're committing to this style? The Google JS style guide uses trailing
> underscores for "private" members, and all of Chromium follows that.

Not necessarily. Currently garden-o-matic has dozens or more of instances of
prefixed underscores and only these three of trailing. This patch is just about
making garden-o-matic self-consistent. We can reconsider the larger style issue,
but that's a separate discussion since we should do so for the whole code base
instead of just this one place.

It's a bummer to me that our JS and Python styles are inconsistent, but I'm fine
with going with the larger Chromium style if we really are consistent elsewhere.
I suspect that in practice right now we use trailing underscores in chrome:
page's javascript and that we use preceding underscores in all of the buildbot
and infra related javascript since that was written mostly by python hackers.

[abarth-chromium, 2014-07-10 05:49:35]

LGTM

[abarth-chromium, 2014-07-10 05:49:44]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-07-10 05:50:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ojan@chromium.org/380983002/20001

[commit-bot: I haz the power, 2014-07-10 05:50:41]

Change committed as 177804