Add support for custom entries in <cr_network_select/>.

Add support for custom entries in <cr_network_select/>.

This CL adds support for custom menu entries in <cr_network_select/> .

BUG=604119
TEST=none
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/12d8289a3c40f7ad998962c07f35fdffb949ee94
Cr-Commit-Position: refs/heads/master@{#403090}

[Alexander Alekseev, 2016-06-16 01:00:13]

Description was changed from

==========
Add support for custom entries in <cr_network_select/>.

This CL adds support for custom menu entries in <cr_network_select/> .

BUG=604119
TEST=none
==========

to

==========
Add support for custom entries in <cr_network_select/>.

This CL adds support for custom menu entries in <cr_network_select/> .

BUG=604119
TEST=none
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Alexander Alekseev, 2016-06-16 01:03:13]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[Alexander Alekseev, 2016-06-16 01:03:29]

alemate@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Alexander Alekseev, 2016-06-16 01:03:30]

Please review.

[commit-bot: I haz the power, 2016-06-16 01:03:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2069323002/20001

[Alexander Alekseev, 2016-06-16 01:17:05]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-16 01:17:40]

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

[commit-bot: I haz the power, 2016-06-16 01:28:56]

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

[commit-bot: I haz the power, 2016-06-16 01:28:57]

Dry run: Try jobs failed on following builders:
  closure_compilation on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[stevenjb, 2016-06-16 17:41:34]

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/network/cr_network_list.html (right):

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_list.html:29:
list-item-type="[[listType]]" on-tap="onTap_" no-buttons="[[noButtons]]"
Can we invert this, e.g. show-buttons?

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_list.html:30:
network-connected-event="[[networkConnectedEvent]]">
Rather than providing a custom event in this element, we should just have
cr-network-list-item fire a 'network-connect' event and have any wrapper handle
it.

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right):

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_list_item.html:4: <link
rel="import" href="chrome://resources/polymer/v1_0/iron-icons/icons.html">
This is going to be disallowed soon. It takes a very long time to load. Instead
we need to create a custom subset with the supported iconset copied into it. See
chrome/browser/resources/settings/icons.html for an example.

(But also see my next comment)

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/network/cr_network_list_item.js (right):

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_list_item.js:99: *       )}
This is pretty ugly. I don't think that wedging in button behavior to this
(poorly named) element is the correct approach.

We should create a separate element, e.g. cr_network_list_button.html. It should
be straightforward to allow cr-network-list to support either.

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/network/cr_network_select.js (right):

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_select.js:217:
this.onNetworkListItemSelectedObserver(state);
Why have a callback in addition to firing an event?

Most of this should be unnecessary. We should be able to just handle
'on-selected' in the container without any extra work. The only thing we need to
add is something like:

if (!state.GUID)
  return;  // The 'selected' event may be handled by a container.

(We might want to rename the event 'network-item-selected' to make it more
specific)

[Alexander Alekseev, 2016-06-22 08:24:18]

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/network/cr_network_list.html (right):

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_list.html:29:
list-item-type="[[listType]]" on-tap="onTap_" no-buttons="[[noButtons]]"
On 2016/06/16 17:41:34, stevenjb wrote:
> Can we invert this, e.g. show-buttons?

Done.

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_list.html:30:
network-connected-event="[[networkConnectedEvent]]">
On 2016/06/16 17:41:34, stevenjb wrote:
> Rather than providing a custom event in this element, we should just have
> cr-network-list-item fire a 'network-connect' event and have any wrapper
handle
> it.

Done.

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right):

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_list_item.html:4: <link
rel="import" href="chrome://resources/polymer/v1_0/iron-icons/icons.html">
On 2016/06/16 17:41:34, stevenjb wrote:
> This is going to be disallowed soon. It takes a very long time to load.
Instead
> we need to create a custom subset with the supported iconset copied into it.
See
> chrome/browser/resources/settings/icons.html for an example.
> 
> (But also see my next comment)

Done.

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/network/cr_network_list_item.js (right):

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_list_item.js:99: *       )}
On 2016/06/16 17:41:34, stevenjb wrote:
> This is pretty ugly. I don't think that wedging in button behavior to this
> (poorly named) element is the correct approach.
> 
> We should create a separate element, e.g. cr_network_list_button.html. It
should
> be straightforward to allow cr-network-list to support either.

Done.

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/network/cr_network_select.js (right):

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_select.js:217:
this.onNetworkListItemSelectedObserver(state);
On 2016/06/16 17:41:34, stevenjb wrote:
> Why have a callback in addition to firing an event?
> 
> Most of this should be unnecessary. We should be able to just handle
> 'on-selected' in the container without any extra work. The only thing we need
to
> add is something like:
> 
> if (!state.GUID)
>   return;  // The 'selected' event may be handled by a container.
> 
> (We might want to rename the event 'network-item-selected' to make it more
> specific)

Before user clicks on a network, we cannot proceed even if some network is
connected. Because it might be a wrong network. So we first wait for user click
(via onNetworkListItemSelectedObserver) and then wait for selected network to
receive "Connected" status (via OnNetworkConnected event).

[Alexander Alekseev, 2016-06-22 08:24:31]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-22 08:24:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2069323002/60001

[commit-bot: I haz the power, 2016-06-22 09:15:25]

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

[commit-bot: I haz the power, 2016-06-22 09:15:26]

Dry run: This issue passed the CQ dry run.

[Alexander Alekseev, 2016-06-22 10:37:58]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-22 10:38:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2069323002/80001

[commit-bot: I haz the power, 2016-06-22 11:31:44]

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

[commit-bot: I haz the power, 2016-06-22 11:31:45]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2016-06-22 21:00:52]

On 2016/06/22 11:31:45, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

So, I apologize in advance, I really am not trying to be difficult, but I am
finding this CL very hard to follow.

It is not clear to me why we need:
cr_network_list_item
cr_network_list_item_base
cr_network_list_network_item
cr_network_list_custom_item

Why not just:
cr_network_list_item  (should be mostly unchanged)
cr_network_list_custom_item (new behavior)

There doesn't appear to be a lot of shared behavior between the two.

Also, I don't see any need to store them in the same list, i.e.
cr_network_list.hml could look something like:

    <iron-collapse opened="{{opened}}">
      <div id="container" class="layout vertical flex">
        <template is="dom-repeat" items="[[networks]]">
          <cr-network-list-item network-state="[[item]]"
              list-item-type="[[listType]]" on-tap="onTap_">
          </cr-network-list-item>
        </template>

        <template is="dom-repeat" items="[[customStates]]">
          <cr-network-list-custom-item custom-state="[[item]]>
          </cr-network-list-custom-item>
        </template>

        </template>
      </div>
    </iron-collapse>

[stevenjb, 2016-06-22 21:03:24]

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/network/cr_network_select.js (right):

https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/network/cr_network_select.js:217:
this.onNetworkListItemSelectedObserver(state);
On 2016/06/22 08:24:18, Alexander Alekseev wrote:
> On 2016/06/16 17:41:34, stevenjb wrote:
> > Why have a callback in addition to firing an event?
> > 
> > Most of this should be unnecessary. We should be able to just handle
> > 'on-selected' in the container without any extra work. The only thing we
need
> to
> > add is something like:
> > 
> > if (!state.GUID)
> >   return;  // The 'selected' event may be handled by a container.
> > 
> > (We might want to rename the event 'network-item-selected' to make it more
> > specific)
> 
> Before user clicks on a network, we cannot proceed even if some network is
> connected. Because it might be a wrong network. So we first wait for user
click
> (via onNetworkListItemSelectedObserver) and then wait for selected network to
> receive "Connected" status (via OnNetworkConnected event).

I am still confused. If we relaly need to do something different in OOBE then we
should just provide a way to override the selected behavior rather than provide
an observer in addition to an event. It is very confusing.

[Alexander Alekseev, 2016-06-22 22:42:21]

On 2016/06/22 21:00:52, stevenjb wrote:
> On 2016/06/22 11:31:45, commit-bot: I haz the power wrote:
> > Dry run: This issue passed the CQ dry run.
> 
> So, I apologize in advance, I really am not trying to be difficult, but I am
> finding this CL very hard to follow.
> 
> It is not clear to me why we need:
> cr_network_list_item
> cr_network_list_item_base
> cr_network_list_network_item
> cr_network_list_custom_item
> 
> Why not just:
> cr_network_list_item  (should be mostly unchanged)
> cr_network_list_custom_item (new behavior)
> 
> There doesn't appear to be a lot of shared behavior between the two.

We need to make styles shared. That's why cr_network_list_base exists: it
defines all visual presentation.


> Also, I don't see any need to store them in the same list, i.e.
> cr_network_list.hml could look something like:
> 
>     <iron-collapse opened="{{opened}}">
>       <div id="container" class="layout vertical flex">
>         <template is="dom-repeat" items="[[networks]]">
>           <cr-network-list-item network-state="[[item]]"
>               list-item-type="[[listType]]" on-tap="onTap_">
>           </cr-network-list-item>
>         </template>
> 
>         <template is="dom-repeat" items="[[customStates]]">
>           <cr-network-list-custom-item custom-state="[[item]]>
>           </cr-network-list-custom-item>
>         </template>
> 
>         </template>
>       </div>
>     </iron-collapse>

[Alexander Alekseev, 2016-06-22 22:44:54]

On 2016/06/22 21:03:24, stevenjb wrote:
>
https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
> File ui/webui/resources/cr_elements/network/cr_network_select.js (right):
> 
>
https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
> ui/webui/resources/cr_elements/network/cr_network_select.js:217:
> this.onNetworkListItemSelectedObserver(state);
> On 2016/06/22 08:24:18, Alexander Alekseev wrote:
> > On 2016/06/16 17:41:34, stevenjb wrote:
> > > Why have a callback in addition to firing an event?
> > > 
> > > Most of this should be unnecessary. We should be able to just handle
> > > 'on-selected' in the container without any extra work. The only thing we
> need
> > to
> > > add is something like:
> > > 
> > > if (!state.GUID)
> > >   return;  // The 'selected' event may be handled by a container.
> > > 
> > > (We might want to rename the event 'network-item-selected' to make it more
> > > specific)
> > 
> > Before user clicks on a network, we cannot proceed even if some network is
> > connected. Because it might be a wrong network. So we first wait for user
> click
> > (via onNetworkListItemSelectedObserver) and then wait for selected network
to
> > receive "Connected" status (via OnNetworkConnected event).
> 
> I am still confused. If we relaly need to do something different in OOBE then
we

"to do something different" as compared to what?
I've just implemented this behavior, I didn't change any existing behavior.



> should just provide a way to override the selected behavior rather than
provide
> an observer in addition to an event. It is very confusing.

[stevenjb, 2016-06-22 22:58:19]

1. The styling isn't that complicated, we shouldn't be using 4 html+JS
files just to share styling. We can also put common styling in a shared
style module, see settings_shared_css.html for an example.

2. I specifically do not understand the intention behind
NetworkListItemSelectedObserver, and I do not think it belongs in
cr_network_select.js. If we want to skip or override the existing
onNetworkListItemSelected_()
behavior we can simple provide a way to early exit, e.g:

onNetworkListItemSelected_: function(event) {
  if (!this.handleConnect)
    return;  // Container will handle 'selected' event.
  ...
}


On Wed, Jun 22, 2016 at 3:44 PM, <alemate@chromium.org> wrote:

> On 2016/06/22 21:03:24, stevenjb wrote:
> >
>
>
https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
> > File ui/webui/resources/cr_elements/network/cr_network_select.js (right):
> >
> >
>
>
https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
> > ui/webui/resources/cr_elements/network/cr_network_select.js:217:
> > this.onNetworkListItemSelectedObserver(state);
> > On 2016/06/22 08:24:18, Alexander Alekseev wrote:
> > > On 2016/06/16 17:41:34, stevenjb wrote:
> > > > Why have a callback in addition to firing an event?
> > > >
> > > > Most of this should be unnecessary. We should be able to just handle
> > > > 'on-selected' in the container without any extra work. The only
> thing we
> > need
> > > to
> > > > add is something like:
> > > >
> > > > if (!state.GUID)
> > > > return; // The 'selected' event may be handled by a container.
> > > >
> > > > (We might want to rename the event 'network-item-selected' to make
> it more
> > > > specific)
> > >
> > > Before user clicks on a network, we cannot proceed even if some
> network is
> > > connected. Because it might be a wrong network. So we first wait for
> user
> > click
> > > (via onNetworkListItemSelectedObserver) and then wait for selected
> network
> to
> > > receive "Connected" status (via OnNetworkConnected event).
> >
> > I am still confused. If we relaly need to do something different in OOBE
> then
> we
>
> "to do something different" as compared to what?
> I've just implemented this behavior, I didn't change any existing behavior.
>
>
>
> > should just provide a way to override the selected behavior rather than
> provide
> > an observer in addition to an event. It is very confusing.
>
>
>
> https://codereview.chromium.org/2069323002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexander Alekseev, 2016-06-22 23:49:45]

On 2016/06/22 22:58:19, stevenjb wrote:
> 1. The styling isn't that complicated, we shouldn't be using 4 html+JS
> files just to share styling. We can also put common styling in a shared
> style module, see settings_shared_css.html for an example.
But it can't be local (for some reason CSS in /settings/ is not inlined during
compile).

> 2. I specifically do not understand the intention behind
> NetworkListItemSelectedObserver, and I do not think it belongs in
> cr_network_select.js. If we want to skip or override the existing
> onNetworkListItemSelected_()
> behavior we can simple provide a way to early exit, e.g:
> 
> onNetworkListItemSelected_: function(event) {
>   if (!this.handleConnect)
>     return;  // Container will handle 'selected' event.
>   ...
> }

onNetworkListItemSelected_ is a member of cr-network-select.
This is the entry point for "selected" event for an entry.
It sends "try to connect" request to chrome.

And we still need this request to be sent.
But before it is sent, we need to notify the screen that we are now waiting for
"this" network to connect.


> On Wed, Jun 22, 2016 at 3:44 PM, <mailto:alemate@chromium.org> wrote:
> 
> > On 2016/06/22 21:03:24, stevenjb wrote:
> > >
> >
> >
>
https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
> > > File ui/webui/resources/cr_elements/network/cr_network_select.js (right):
> > >
> > >
> >
> >
>
https://codereview.chromium.org/2069323002/diff/40001/ui/webui/resources/cr_e...
> > > ui/webui/resources/cr_elements/network/cr_network_select.js:217:
> > > this.onNetworkListItemSelectedObserver(state);
> > > On 2016/06/22 08:24:18, Alexander Alekseev wrote:
> > > > On 2016/06/16 17:41:34, stevenjb wrote:
> > > > > Why have a callback in addition to firing an event?
> > > > >
> > > > > Most of this should be unnecessary. We should be able to just handle
> > > > > 'on-selected' in the container without any extra work. The only
> > thing we
> > > need
> > > > to
> > > > > add is something like:
> > > > >
> > > > > if (!state.GUID)
> > > > > return; // The 'selected' event may be handled by a container.
> > > > >
> > > > > (We might want to rename the event 'network-item-selected' to make
> > it more
> > > > > specific)
> > > >
> > > > Before user clicks on a network, we cannot proceed even if some
> > network is
> > > > connected. Because it might be a wrong network. So we first wait for
> > user
> > > click
> > > > (via onNetworkListItemSelectedObserver) and then wait for selected
> > network
> > to
> > > > receive "Connected" status (via OnNetworkConnected event).
> > >
> > > I am still confused. If we relaly need to do something different in OOBE
> > then
> > we
> >
> > "to do something different" as compared to what?
> > I've just implemented this behavior, I didn't change any existing behavior.
> >
> >
> >
> > > should just provide a way to override the selected behavior rather than
> > provide
> > > an observer in addition to an event. It is very confusing.
> >
> >
> >
> > https://codereview.chromium.org/2069323002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2016-06-22 23:59:49]

On Wed, Jun 22, 2016 at 4:49 PM, <alemate@chromium.org> wrote:

> On 2016/06/22 22:58:19, stevenjb wrote:
> > 1. The styling isn't that complicated, we shouldn't be using 4 html+JS
> > files just to share styling. We can also put common styling in a shared
> > style module, see settings_shared_css.html for an example.
> But it can't be local (for some reason CSS in /settings/ is not inlined
> during
> compile).
>
> > 2. I specifically do not understand the intention behind
> > NetworkListItemSelectedObserver, and I do not think it belongs in
> > cr_network_select.js. If we want to skip or override the existing
> > onNetworkListItemSelected_()
> > behavior we can simple provide a way to early exit, e.g:
> >
> > onNetworkListItemSelected_: function(event) {
> > if (!this.handleConnect)
> > return; // Container will handle 'selected' event.
> > ...
> > }
>
> onNetworkListItemSelected_ is a member of cr-network-select.
> This is the entry point for "selected" event for an entry.
> It sends "try to connect" request to chrome.
>
> And we still need this request to be sent.
> But before it is sent, we need to notify the screen that we are now
> waiting for
> "this" network to connect.
>
>
Can you point me to where you use this observer?

I don't understand why the notification needs to occur "before" calling
chrome.networkingPrivate.startConnect, but if it does, I would prefer to
just copy the chrome.networkingPrivate.startConnect code to the container,
it's not very complicated. Adding the overhead of an observer to this
element seems like overkill.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexander Alekseev, 2016-06-23 00:06:53]

On 2016/06/22 23:59:49, stevenjb wrote:
> On Wed, Jun 22, 2016 at 4:49 PM, <mailto:alemate@chromium.org> wrote:
> 
> > On 2016/06/22 22:58:19, stevenjb wrote:
> > > 1. The styling isn't that complicated, we shouldn't be using 4 html+JS
> > > files just to share styling. We can also put common styling in a shared
> > > style module, see settings_shared_css.html for an example.
> > But it can't be local (for some reason CSS in /settings/ is not inlined
> > during
> > compile).
> >
> > > 2. I specifically do not understand the intention behind
> > > NetworkListItemSelectedObserver, and I do not think it belongs in
> > > cr_network_select.js. If we want to skip or override the existing
> > > onNetworkListItemSelected_()
> > > behavior we can simple provide a way to early exit, e.g:
> > >
> > > onNetworkListItemSelected_: function(event) {
> > > if (!this.handleConnect)
> > > return; // Container will handle 'selected' event.
> > > ...
> > > }
> >
> > onNetworkListItemSelected_ is a member of cr-network-select.
> > This is the entry point for "selected" event for an entry.
> > It sends "try to connect" request to chrome.
> >
> > And we still need this request to be sent.
> > But before it is sent, we need to notify the screen that we are now
> > waiting for
> > "this" network to connect.
> >
> >
> Can you point me to where you use this observer?

In a dependent CL:

https://codereview.chromium.org/2067153002/diff/80001/chrome/browser/resource...

(see last three methods)


> I don't understand why the notification needs to occur "before" calling
> chrome.networkingPrivate.startConnect, but if it does, I would prefer to
> just copy the chrome.networkingPrivate.startConnect code to the container,
> it's not very complicated. Adding the overhead of an observer to this
> element seems like overkill.

This probably requires extending API permissions (but probably not).

[stevenjb, 2016-06-23 00:19:49]

Yeah, looking at that code I think it would be much cleaner if it just
handled the network connect request directly in the OOBE code instead, e.g:

  <cr-network-select ... handle-connect=false
on-selected="onNetworkSelected_">

  onNetworkSelected_: function(event) {
    var state = event.detail;
    if (state.ConnectionState != CrOnc.ConnectionState.NOT_CONNECTED)
      return;
    chrome.networkingPrivate.startConnect(state.GUID, function() {
      var lastError = chrome.runtime.lastError;
      if (lastError && lastError != 'connecting') {
        // TODO: Show an error message in the oobe screen
        return;
      }
      $('oobe-connect').hidden = false;
      $('oobe-welcome-md').hidden = true;
    });
  },

That way you don't need onNetworkConnected, you don't
need networkLastSelectedGuid_ (which seems a bit fragile), and the only
thing we need to add to cr-network-select is a handleConnect property that
defaults to true and the oobe screen sets to false.

The API permissions won't change, it's tied to the base url not the JS
module.


On Wed, Jun 22, 2016 at 5:06 PM, <alemate@chromium.org> wrote:

> On 2016/06/22 23:59:49, stevenjb wrote:
>
> > On Wed, Jun 22, 2016 at 4:49 PM, <mailto:alemate@chromium.org> wrote:
> >
> > > On 2016/06/22 22:58:19, stevenjb wrote:
> > > > 1. The styling isn't that complicated, we shouldn't be using 4
> html+JS
> > > > files just to share styling. We can also put common styling in a
> shared
> > > > style module, see settings_shared_css.html for an example.
> > > But it can't be local (for some reason CSS in /settings/ is not inlined
> > > during
> > > compile).
> > >
> > > > 2. I specifically do not understand the intention behind
> > > > NetworkListItemSelectedObserver, and I do not think it belongs in
> > > > cr_network_select.js. If we want to skip or override the existing
> > > > onNetworkListItemSelected_()
> > > > behavior we can simple provide a way to early exit, e.g:
> > > >
> > > > onNetworkListItemSelected_: function(event) {
> > > > if (!this.handleConnect)
> > > > return; // Container will handle 'selected' event.
> > > > ...
> > > > }
> > >
> > > onNetworkListItemSelected_ is a member of cr-network-select.
> > > This is the entry point for "selected" event for an entry.
> > > It sends "try to connect" request to chrome.
> > >
> > > And we still need this request to be sent.
> > > But before it is sent, we need to notify the screen that we are now
> > > waiting for
> > > "this" network to connect.
> > >
> > >
> > Can you point me to where you use this observer?
>
> In a dependent CL:
>
>
>
https://codereview.chromium.org/2067153002/diff/80001/chrome/browser/resource...
>
> (see last three methods)
>
>
> > I don't understand why the notification needs to occur "before" calling
> > chrome.networkingPrivate.startConnect, but if it does, I would prefer to
> > just copy the chrome.networkingPrivate.startConnect code to the
> container,
> > it's not very complicated. Adding the overhead of an observer to this
> > element seems like overkill.
>
> This probably requires extending API permissions (but probably not).
>
> https://codereview.chromium.org/2069323002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexander Alekseev, 2016-06-23 00:46:04]

On 2016/06/23 00:19:49, stevenjb wrote:
> Yeah, looking at that code I think it would be much cleaner if it just
> handled the network connect request directly in the OOBE code instead, e.g:
> 
>   <cr-network-select ... handle-connect=false
> on-selected="onNetworkSelected_">
> 
>   onNetworkSelected_: function(event) {
>     var state = event.detail;
>     if (state.ConnectionState != CrOnc.ConnectionState.NOT_CONNECTED)
>       return;
>     chrome.networkingPrivate.startConnect(state.GUID, function() {
>       var lastError = chrome.runtime.lastError;
>       if (lastError && lastError != 'connecting') {
>         // TODO: Show an error message in the oobe screen
>         return;
>       }
>       $('oobe-connect').hidden = false;
>       $('oobe-welcome-md').hidden = true;
>     });
>   },
> 
> That way you don't need onNetworkConnected, you don't
> need networkLastSelectedGuid_ (which seems a bit fragile), and the only
> thing we need to add to cr-network-select is a handleConnect property that
> defaults to true and the oobe screen sets to false.
> 
> The API permissions won't change, it's tied to the base url not the JS
> module.


Looking at all that code, it seems to me that the initial approach of
https://codereview.chromium.org/2067153002/#ps20001 (the same as Patch Set 3 in
this CL) was correct, because it doesn't hide the functionality behind extra
level of abstractions, while still preserving "single entry - single exit" for
the event flow.

I don't think that splitting one list of networks into two lists (two
"dom-repeated" templates) which visually look the same, is convenient, because
we will have to remember to update code in two places when making future
changes, and to verify visual similarity by our own eyes. I believe, if we see
one list visually, we should have a single list object behind it. And list is a
list of entries, so it must have one "list entry" object.

Thus, if we want to split custom list entry and network list entry into separate
objects, we still need to have all visual elements in one place. I implemented
this, and I agree that this is overkill for the functionality we need.


But you are the owner of this code, so you are to decide.
I'll implement what you suggest.

[stevenjb, 2016-06-23 01:00:06]

While I see your point, this is html and javascript, we are used to
containers (be it a scrolling list or just a div) containing items of
different types. Handling widely different behaviors in a single element is
confusing, and html/JS does not do C++ style inheritance particularly well.

We should be able to used a shared CSS style to share the visual style so
hopefully we shouldn't have to make changes in more than one place.

i appreciate your patience with this. Hopefully we can come up with
something that we both feel is reasonably easy to use and maintainable.


On Wed, Jun 22, 2016 at 5:46 PM, <alemate@chromium.org> wrote:

> On 2016/06/23 00:19:49, stevenjb wrote:
> > Yeah, looking at that code I think it would be much cleaner if it just
> > handled the network connect request directly in the OOBE code instead,
> e.g:
> >
> > <cr-network-select ... handle-connect=false
> > on-selected="onNetworkSelected_">
> >
> > onNetworkSelected_: function(event) {
> > var state = event.detail;
> > if (state.ConnectionState != CrOnc.ConnectionState.NOT_CONNECTED)
> > return;
> > chrome.networkingPrivate.startConnect(state.GUID, function() {
> > var lastError = chrome.runtime.lastError;
> > if (lastError && lastError != 'connecting') {
> > // TODO: Show an error message in the oobe screen
> > return;
> > }
> > $('oobe-connect').hidden = false;
> > $('oobe-welcome-md').hidden = true;
> > });
> > },
> >
> > That way you don't need onNetworkConnected, you don't
> > need networkLastSelectedGuid_ (which seems a bit fragile), and the only
> > thing we need to add to cr-network-select is a handleConnect property
> that
> > defaults to true and the oobe screen sets to false.
> >
> > The API permissions won't change, it's tied to the base url not the JS
> > module.
>
>
> Looking at all that code, it seems to me that the initial approach of
> https://codereview.chromium.org/2067153002/#ps20001 (the same as Patch
> Set 3 in
> this CL) was correct, because it doesn't hide the functionality behind
> extra
> level of abstractions, while still preserving "single entry - single exit"
> for
> the event flow.
>
> I don't think that splitting one list of networks into two lists (two
> "dom-repeated" templates) which visually look the same, is convenient,
> because
> we will have to remember to update code in two places when making future
> changes, and to verify visual similarity by our own eyes. I believe, if we
> see
> one list visually, we should have a single list object behind it. And list
> is a
> list of entries, so it must have one "list entry" object.
>
> Thus, if we want to split custom list entry and network list entry into
> separate
> objects, we still need to have all visual elements in one place. I
> implemented
> this, and I agree that this is overkill for the functionality we need.
>
>
> But you are the owner of this code, so you are to decide.
> I'll implement what you suggest.
>
> https://codereview.chromium.org/2069323002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexander Alekseev, 2016-06-24 06:34:16]

Please review.

[stevenjb, 2016-06-24 22:33:09]

Thanks! FWIW, this was *much* more straightforward to review.

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/compiled_resources2.gyp (right):

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/compiled_resources2.gyp:41:
'target_name': 'cr_network_list_network_item',
Duplicate?

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list_custom_item.js
(right):

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_custom_item.js:48: *
@type {!Object}
Instead of commenting the properties like this, use closure notation to provide
a typedef, e.g. @type {CrNetworkListCustomItemState}. See cr_onc_types.js for a
bunch of typedef examples.

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_custom_item.js:59: */
nit: Comment not needed

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_custom_item.js:74: }
This shouldn't be necessary, you can just use
[[getText(itemState.customItemName)]] in the HTML.

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_custom_item.js:76: 
provide @param doc for |e|

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_custom_item.js:82:
this.fire(this.itemState.onTapEvent, this.itemState);
Instead of using a custom event, just fire e.g. 'network-custom-item-selected'.
The container can use itemState in the event handler to direct the behavior.

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list_network_item.html
(right):

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.html:11:
<style include="cr-network-list-item-css"></style>
Could you upload with a lower similarity value so that we get a diff for this? I
assume it is mostly unchanged other than the CSS getting moved... Thanks.

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list_network_item.js
(right):

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.js:101: *
Permission to show buttons.
nit: 'Whether to show buttons.' sounds a bit more natural.

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.js:115:
networkConnectedEvent: {
I don't think we need/want this, see comment below.

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.js:122: *
Determines visibility of the "Settings" button.
* @private {boolean}

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.js:127: *
Determines visibility of "known buttons" block.
* @private {boolean}

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.js:151:
this.fire(this.networkConnectedEvent, this.networkState);
Rather than provide a custom property for this event, this could just fire
'network-connected' and document that in the header. That way we wouldn't need
to propagate this property through the layers.

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_select.js (right):

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_select.js:84: value: false,
nit: This could default to true, then we don't have to explicitly set it in the
existing code.

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_select.js:94:
itemSelectedEventOverride: {
I think this should just be a boolean, e.g. 'handleConnect' which could default
to true, and if set to false fire a well defined event, e.g. 'network-selected'.

[Alexander Alekseev, 2016-06-25 06:04:43]

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/compiled_resources2.gyp (right):

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/compiled_resources2.gyp:41:
'target_name': 'cr_network_list_network_item',
On 2016/06/24 22:33:08, stevenjb wrote:
> Duplicate?

Done.

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list_custom_item.js
(right):

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_custom_item.js:48: *
@type {!Object}
On 2016/06/24 22:33:08, stevenjb wrote:
> Instead of commenting the properties like this, use closure notation to
provide
> a typedef, e.g. @type {CrNetworkListCustomItemState}. See cr_onc_types.js for
a
> bunch of typedef examples.

Done.

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_custom_item.js:59: */
On 2016/06/24 22:33:08, stevenjb wrote:
> nit: Comment not needed

Done.

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_custom_item.js:74: }
On 2016/06/24 22:33:08, stevenjb wrote:
> This shouldn't be necessary, you can just use
> [[getText(itemState.customItemName)]] in the HTML.
> 

Done.

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_custom_item.js:76: 
On 2016/06/24 22:33:08, stevenjb wrote:
> provide @param doc for |e|

Done.

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_custom_item.js:82:
this.fire(this.itemState.onTapEvent, this.itemState);
On 2016/06/24 22:33:08, stevenjb wrote:
> Instead of using a custom event, just fire e.g.
'network-custom-item-selected'.
> The container can use itemState in the event handler to direct the behavior.

Done.

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list_network_item.js
(right):

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.js:101: *
Permission to show buttons.
On 2016/06/24 22:33:08, stevenjb wrote:
> nit: 'Whether to show buttons.' sounds a bit more natural.

Done.

[stevenjb, 2016-06-27 20:06:04]

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list_network_item.html
(right):

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.html:11:
<style include="cr-network-list-item-css"></style>
On 2016/06/24 22:33:08, stevenjb wrote:
> Could you upload with a lower similarity value so that we get a diff for this?
I
> assume it is mostly unchanged other than the CSS getting moved... Thanks.

This looks like a new file, even though you just renamed it and moved the CSS.

You should be able to force this to diff against the original file with
something like:

git cl upload --similarity=30

(So that files only have to be 30% similar to be considered the same file
instead of the default of 50%)

[stevenjb, 2016-06-27 20:09:45]

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_select.js (right):

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_select.js:94:
itemSelectedEventOverride: {
On 2016/06/24 22:33:08, stevenjb wrote:
> I think this should just be a boolean, e.g. 'handleConnect' which could
default
> to true, and if set to false fire a well defined event, e.g.
'network-selected'.

How about this?

https://codereview.chromium.org/2069323002/diff/120001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list_network_item.js
(right):

https://codereview.chromium.org/2069323002/diff/120001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.js:111: *
Determines visibility of the "Settings" button.
Please add the following to this comment and the next one:

* @private {boolean}

[Alexander Alekseev, 2016-06-28 06:51:16]

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_select.js (right):

https://codereview.chromium.org/2069323002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_select.js:94:
itemSelectedEventOverride: {
On 2016/06/27 20:09:45, stevenjb wrote:
> On 2016/06/24 22:33:08, stevenjb wrote:
> > I think this should just be a boolean, e.g. 'handleConnect' which could
> default
> > to true, and if set to false fire a well defined event, e.g.
> 'network-selected'.
> 
> How about this?

Done.

https://codereview.chromium.org/2069323002/diff/120001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list_network_item.js
(right):

https://codereview.chromium.org/2069323002/diff/120001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.js:111: *
Determines visibility of the "Settings" button.
On 2016/06/27 20:09:45, stevenjb wrote:
> Please add the following to this comment and the next one:
> 
> * @private {boolean}

Done.

[stevenjb, 2016-06-28 18:35:34]

Looking good, thanks for re-uploading, it made reviewing that file much easier.
Just a couple more comments.

https://codereview.chromium.org/2069323002/diff/180001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list_network_item.html
(right):

https://codereview.chromium.org/2069323002/diff/180001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.html:19:
<span id="itemName"></span>
optional nit: can we leave this networkName now, or does it get accessed from
cr-network-list? It's fine either way.

https://codereview.chromium.org/2069323002/diff/180001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.html:23:
<div class="buttons" hidden$="[[!showSettingsButton_]]">
This is not reliable, see comment in .js.

https://codereview.chromium.org/2069323002/diff/180001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list_network_item.js
(right):

https://codereview.chromium.org/2069323002/diff/180001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.js:257:
this.isListItemType_(this.listItemType, 'visible');
This is not guaranteed to update the visibility in the html unless
showSettingsButton_ is a declared property.

A better way to do this is to have a function like
isSettingsButtonVisible(showButtons, listItemType) and use that in the html,
e.g.

hidden$="[[!isSettingsButtonVisible(showButtons, listItemType)]]"

Note: it is important to pass both properties to isSettingsButtonVisible so that
if either one changes the hidden property changes.

Also we can then remove the showButtonsChanged_ observer and the private
variables.

[Alexander Alekseev, 2016-06-29 03:30:32]

https://codereview.chromium.org/2069323002/diff/180001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list_network_item.html
(right):

https://codereview.chromium.org/2069323002/diff/180001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.html:19:
<span id="itemName"></span>
On 2016/06/28 18:35:33, stevenjb wrote:
> optional nit: can we leave this networkName now, or does it get accessed from
> cr-network-list? It's fine either way.
> 

This is because CSS is shared, so this name is used both in
<cr-network-list-network-item/> and <cr-network-list-custom-item/>

https://codereview.chromium.org/2069323002/diff/180001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list_network_item.js
(right):

https://codereview.chromium.org/2069323002/diff/180001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.js:257:
this.isListItemType_(this.listItemType, 'visible');
On 2016/06/28 18:35:34, stevenjb wrote:
> This is not guaranteed to update the visibility in the html unless
> showSettingsButton_ is a declared property.
> 
> A better way to do this is to have a function like
> isSettingsButtonVisible(showButtons, listItemType) and use that in the html,
> e.g.
> 
> hidden$="[[!isSettingsButtonVisible(showButtons, listItemType)]]"
> 
> Note: it is important to pass both properties to isSettingsButtonVisible so
that
> if either one changes the hidden property changes.
> 
> Also we can then remove the showButtonsChanged_ observer and the private
> variables.

Done.

[stevenjb, 2016-06-29 17:32:56]

https://codereview.chromium.org/2069323002/diff/180001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list_network_item.html
(right):

https://codereview.chromium.org/2069323002/diff/180001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.html:19:
<span id="itemName"></span>
On 2016/06/29 03:30:31, Alexander Alekseev wrote:
> On 2016/06/28 18:35:33, stevenjb wrote:
> > optional nit: can we leave this networkName now, or does it get accessed
from
> > cr-network-list? It's fine either way.
> > 
> 
> This is because CSS is shared, so this name is used both in
> <cr-network-list-network-item/> and <cr-network-list-custom-item/>

Ah. It would be better to use a class than an id in that case (that was my
fault, the CSS was written when I hadn't done much CSS), but again, this is fine
as-is.

https://codereview.chromium.org/2069323002/diff/200001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list_network_item.html
(right):

https://codereview.chromium.org/2069323002/diff/200001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.html:23:
<div class="buttons"
hidden$="[[!isSettingsButtonVisible_(showButtons,listItemType)]]">
nit: Move 'hidden...' to next line to avoid (or at least reduce) wrapping, here
and below.

https://codereview.chromium.org/2069323002/diff/200001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list_network_item.js
(right):

https://codereview.chromium.org/2069323002/diff/200001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.js:234: *
automatically re-run it when any of them are changed.
We use this pattern throughout Polymer code so no need to comment it here, even
if it does take some getting used to. Otherwise we would have comments like this
all over the place :)

https://codereview.chromium.org/2069323002/diff/200001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.js:236: *
@param {boolean}
name parameters:
  * @param {boolean} showButtons

[Alexander Alekseev, 2016-06-29 22:55:34]

https://codereview.chromium.org/2069323002/diff/200001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list_network_item.html
(right):

https://codereview.chromium.org/2069323002/diff/200001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.html:23:
<div class="buttons"
hidden$="[[!isSettingsButtonVisible_(showButtons,listItemType)]]">
On 2016/06/29 17:32:55, stevenjb wrote:
> nit: Move 'hidden...' to next line to avoid (or at least reduce) wrapping,
here
> and below.
> 

Done.

https://codereview.chromium.org/2069323002/diff/200001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list_network_item.js
(right):

https://codereview.chromium.org/2069323002/diff/200001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.js:234: *
automatically re-run it when any of them are changed.
On 2016/06/29 17:32:55, stevenjb wrote:
> We use this pattern throughout Polymer code so no need to comment it here,
even
> if it does take some getting used to. Otherwise we would have comments like
this
> all over the place :)

Done.

https://codereview.chromium.org/2069323002/diff/200001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list_network_item.js:236: *
@param {boolean}
On 2016/06/29 17:32:55, stevenjb wrote:
> name parameters:
>   * @param {boolean} showButtons
> 

Done.

[stevenjb, 2016-06-29 23:25:06]

lgtm, thanks for doing the re-factoring.

Make sure to run ./third_party/closure_compiler/run_compiler if you haven't
already.

[Alexander Alekseev, 2016-06-30 00:41:49]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-30 00:42:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 00:45:18]

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

[commit-bot: I haz the power, 2016-06-30 00:45:19]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alexander Alekseev, 2016-06-30 01:16:45]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-30 01:17:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 01:28:17]

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

[commit-bot: I haz the power, 2016-06-30 01:28:18]

Dry run: Try jobs failed on following builders:
  closure_compilation on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[Alexander Alekseev, 2016-06-30 02:59:08]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-30 02:59:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 04:46:21]

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

[commit-bot: I haz the power, 2016-06-30 04:46:22]

Dry run: This issue passed the CQ dry run.

[Alexander Alekseev, 2016-06-30 04:49:21]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2016-06-30 04:49:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2069323002/#ps260001
(title: "Fix compile.")

[commit-bot: I haz the power, 2016-06-30 04:49:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 04:53:58]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-06-30 04:54:01]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 04:55:43]

Description was changed from

==========
Add support for custom entries in <cr_network_select/>.

This CL adds support for custom menu entries in <cr_network_select/> .

BUG=604119
TEST=none
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Add support for custom entries in <cr_network_select/>.

This CL adds support for custom menu entries in <cr_network_select/> .

BUG=604119
TEST=none
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/12d8289a3c40f7ad998962c07f35fdffb949ee94
Cr-Commit-Position: refs/heads/master@{#403090}
==========

[commit-bot: I haz the power, 2016-06-30 04:55:45]

Patchset 14 (id:??) landed as
https://crrev.com/12d8289a3c40f7ad998962c07f35fdffb949ee94
Cr-Commit-Position: refs/heads/master@{#403090}