Only show tree status if the tree isn't open.

Only show tree status if the tree isn't open.

In the spirit of sheriff-o-matic only showing problems that
need solving, there's no point in showing tree open status.
We might want to reconsider this in the future where we
show you the actual status message, but right now the open
state is hard-coded to say "Tree is open", which is not
information I need to see.

Also, cleaned up a couple minor polymer things:
-No need to set the attributes attribute if we're using publish.
-Initialize all the member variables on ct-tree-status.
-Don't use setAttribute. Should basically always set element
values by property.

NOTRY=true
R=abarth@chromium.org

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

[eseidel, 2014-07-19 04:11:40]

Amen brother. Amen. Success is noise!

On Friday, July 18, 2014, <ojan@chromium.org> wrote:

> Reviewers: abarth, szager1, eseidel, esprehn, Michael Giuffrida, leviw,
>
> Description:
> Only show tree status if the tree isn't open.
>
> In the spirit of sheriff-o-matic only showing problems that
> need solving, there's no point in showing tree open status.
> We might want to reconsider this in the future where we
> show you the actual status message, but right now the open
> state is hard-coded to say "Tree is open", which is not
> information I need to see.
>
> Also, cleaned up a couple minor polymer things:
> -No need to set the attributes attribute if we're using publish.
> -Initialize all the member variables on ct-tree-status.
> -Don't use setAttribute. Should basically always set element
> values by property.
>
> Please review this at https://codereview.chromium.org/403723005/
>
> SVN Base: svn://svn.chromium.org/blink/trunk
>
> Affected files (+31, -20 lines):
>   M Tools/GardeningServer/ui/ct-tree-status.html
>   M Tools/GardeningServer/ui/ct-tree-status-tests.html
>
>
> Index: Tools/GardeningServer/ui/ct-tree-status-tests.html
> diff --git a/Tools/GardeningServer/ui/ct-tree-status-tests.html
> b/Tools/GardeningServer/ui/ct-tree-status-tests.html
> index cdac1562d366954ee16c13c2f5bf0cd9b90c4ae5..
> 817e86cfa2451419e28b02a71c1653c2c5b18d63 100644
> --- a/Tools/GardeningServer/ui/ct-tree-status-tests.html
> +++ b/Tools/GardeningServer/ui/ct-tree-status-tests.html
> @@ -29,7 +29,7 @@ closedTreeJson = {
>    "can_commit_freely": false
>  }
>
> -asyncTest("basic", 7, function() {
> +asyncTest("basic", 9, function() {
>    var simulator = new NetworkSimulator();
>    simulator.json = function(url) {
>      if (url.indexOf('closed') != -1)
> @@ -39,12 +39,14 @@ asyncTest("basic", 7, function() {
>      else
>        return Promise.resolve(openTreeJson);
>    };
> +
>    var opentree = document.createElement("ct-tree-status");
> -  opentree.setAttribute("project", "open-tree-project");
> +  opentree.project = "open-tree-project";
>    var throttledtree = document.createElement("ct-tree-status");
> -  throttledtree.setAttribute("project", "throttled-tree-project");
> +  throttledtree.project = "throttled-tree-project";
>    var closedtree = document.createElement("ct-tree-status");
> -  closedtree.setAttribute("project", "closed-tree-project");
> +  closedtree.project = "closed-tree-project";
> +
>    simulator.runTest(function() {
>      var urlByName = treestatus.urlByName;
>      treestatus.urlByName = function(name) {
> @@ -52,7 +54,6 @@ asyncTest("basic", 7, function() {
>      }
>      Promise.all([
>        opentree.update().then(function() {
> -        equal(opentree.message, "Tree is open");
>          equal(opentree.status, "open");
>        }),
>        throttledtree.update().then(function() {
> @@ -66,8 +67,13 @@ asyncTest("basic", 7, function() {
>          equal(closedtree.status, "closed");
>        })
>      ]).then(function() {
> -      start();
> -      treestatus.urlByName = urlByName;
> +      requestAnimationFrame(function() {
> +        ok(!opentree.shadowRoot.textContent.has("open-tree-project"));
> +        ok(throttledtree.shadowRoot.textContent.has("throttled-
> tree-project"));
> +        ok(closedtree.shadowRoot.textContent.has("closed-tree-project"));
> +        start();
> +        treestatus.urlByName = urlByName;
> +      });
>      });
>    });
>  });
> Index: Tools/GardeningServer/ui/ct-tree-status.html
> diff --git a/Tools/GardeningServer/ui/ct-tree-status.html
> b/Tools/GardeningServer/ui/ct-tree-status.html
> index 0ee4d32e8514e2da067cadbf746501d7e8c67054..
> 7cc4b516fc9ecef22cec42747e8f0a3cdee0d58a 100644
> --- a/Tools/GardeningServer/ui/ct-tree-status.html
> +++ b/Tools/GardeningServer/ui/ct-tree-status.html
> @@ -4,7 +4,7 @@ Use of this source code is governed by a BSD-style license
> that can be
>  found in the LICENSE file.
>  -->
>
> -<polymer-element name="ct-tree-status" attributes="project status">
> +<polymer-element name="ct-tree-status">
>    <template>
>      <style>
>        :host {
> @@ -23,13 +23,17 @@ found in the LICENSE file.
>          background-color: tomato;
>        }
>      </style>
> -    {{project}}: {{message}}
> +    <template if="{{ message }}">
> +      {{project}}: {{message}}
> +    </template>
>    </template>
>    <script>
>      Polymer({
>        publish: {
> +        project: '',
> +        message: '',
>          status: {
> -          value: 'unknown',
> +          value: 'open',
>            reflect: true,
>          },
>        },
> @@ -43,18 +47,19 @@ found in the LICENSE file.
>
>        updateStatus: function(status) {
>          if (status.can_commit_freely) {
> -          this.message = 'Tree is open';
> +          this.message = '';
>            this.status = 'open';
> +          return;
> +        }
> +
> +        this.message = status.message + ' by ' + status.username;
> +        var responseLowerCase = status.message.toLowerCase();
> +        if (responseLowerCase.indexOf('throttled') != -1) {
> +          this.status = 'throttled';
> +        } else if (responseLowerCase.indexOf("closed") != -1) {
> +          this.status = 'closed';
>          } else {
> -          this.message = status.message + ' by ' + status.username;
> -          var responseLowerCase = status.message.toLowerCase();
> -          if (responseLowerCase.indexOf('throttled') != -1) {
> -            this.status = 'throttled';
> -          } else if (responseLowerCase.indexOf("closed") != -1) {
> -            this.status = 'closed';
> -          } else {
> -            this.status = 'unknown';
> -          }
> +          this.status = 'unknown';
>          }
>        },
>      });
>
>
>

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

[abarth-chromium, 2014-07-19 04:18:20]

LGTM

https://codereview.chromium.org/403723005/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-tree-status.html (right):

https://codereview.chromium.org/403723005/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-tree-status.html:26: <template if="{{ message }}">
Move if to outer template?

https://codereview.chromium.org/403723005/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-tree-status.html:50: this.message = '';
null?

[ojan, 2014-07-19 05:21:43]

https://codereview.chromium.org/403723005/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-tree-status.html (right):

https://codereview.chromium.org/403723005/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-tree-status.html:26: <template if="{{ message }}">
On 2014/07/19 04:18:20, abarth wrote:
> Move if to outer template?

Good idea. I tried it, but it didn't work. I think maybe you can't data-bind to
the outer template. I did move this if to contain the style element though.

https://codereview.chromium.org/403723005/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-tree-status.html:50: this.message = '';
On 2014/07/19 04:18:20, abarth wrote:
> null?

How does that work with V8 hidden classes? Will string-->null do a class
transition? Not that this class actually matters for performance of course, but
I'd like to use performant patterns by default.

[abarth-chromium, 2014-07-19 17:24:03]

On 2014/07/19 at 05:21:43, ojan wrote:
>
https://codereview.chromium.org/403723005/diff/1/Tools/GardeningServer/ui/ct-...
> File Tools/GardeningServer/ui/ct-tree-status.html (right):
> 
>
https://codereview.chromium.org/403723005/diff/1/Tools/GardeningServer/ui/ct-...
> Tools/GardeningServer/ui/ct-tree-status.html:26: <template if="{{ message }}">
> On 2014/07/19 04:18:20, abarth wrote:
> > Move if to outer template?
> 
> Good idea. I tried it, but it didn't work. I think maybe you can't data-bind
to the outer template. I did move this if to contain the style element though.

ok

>
https://codereview.chromium.org/403723005/diff/1/Tools/GardeningServer/ui/ct-...
> Tools/GardeningServer/ui/ct-tree-status.html:50: this.message = '';
> On 2014/07/19 04:18:20, abarth wrote:
> > null?
> 
> How does that work with V8 hidden classes? Will string-->null do a class
transition? Not that this class actually matters for performance of course, but
I'd like to use performant patterns by default.

Dunno.  Seems like premature optimization...

[ojan, 2014-07-20 03:21:12]

> >
https://codereview.chromium.org/403723005/diff/1/Tools/GardeningServer/ui/ct-...
> > Tools/GardeningServer/ui/ct-tree-status.html:50: this.message = '';
> > On 2014/07/19 04:18:20, abarth wrote:
> > > null?
> > 
> > How does that work with V8 hidden classes? Will string-->null do a class
transition? Not that this class actually matters for performance of course, but
I'd like to use performant patterns by default.
> 
> Dunno.  Seems like premature optimization...

It's certainly premature optimization. I just didn't see null as better than
empty string, so, all things being equal, may as well choose the (needlessly)
more performant option. In either case, it really doesn't matter. I'll change it
to null.

[ojan, 2014-07-20 03:35:18]

Committed patchset #2 manually as r178524 (presubmit successful).