MD Settings: Use network name for detail page title

MD Settings: Use network name for detail page title

This CL also correctly sorts network types in the main page.

BUG=609156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/05803f278e293ff6d07d4df206d1a2dc3dded7c3
Cr-Commit-Position: refs/heads/master@{#416040}

[stevenjb, 2016-08-29 21:50:24]

Description was changed from

==========
MD Settings: Use network name for detail page title

This CL also correctly sorts network types in the main page.

BUG=609156
==========

to

==========
MD Settings: Use network name for detail page title

This CL also correctly sorts network types in the main page.

BUG=609156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[stevenjb, 2016-08-29 22:20:46]

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

[stevenjb, 2016-08-29 22:20:47]

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

[dschuyler, 2016-08-30 00:50:01]

https://codereview.chromium.org/2290853002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_summary.js (right):

https://codereview.chromium.org/2290853002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary.js:338: /**
@type {!NetworkStateListObject} */ var newNetworkStateLists = {
Optional: I'm used to seeing the var on the
line after the type comment (unless it's a 
cast).

https://codereview.chromium.org/2290853002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary.js:390: });
This looks like the same sort order could be gained
from newNetworkStateLists by concatenating the arrays
akin to 
foo = newNetworkStateLists[CrOnc.Type.ETHERNET].concat( 
    newNetworkStateLists[CrOnc.Type.WI_FI]).concat(   
    newNetworkStateLists[CrOnc.Type.CELLULAR]).concat(   
    newNetworkStateLists[CrOnc.Type.WI_MAX]).concat(   
    newNetworkStateLists[CrOnc.Type.VPN]);
Which seems like it should be more efficient (though
I haven't timed them). The sort is averaging ~7 function
calls per item and function calls are generally slow
in JS.

(My concern may also be tainted by my experiences 
commuting on the bus where hundreds networks will come 
and go while driving down the street).

[dschuyler, 2016-08-30 00:55:37]

https://codereview.chromium.org/2290853002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_summary.js (right):

https://codereview.chromium.org/2290853002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary.js:390: });
On 2016/08/30 00:50:01, dschuyler wrote:
> This looks like the same sort order could be gained
> from newNetworkStateLists by concatenating the arrays
> akin to 
> foo = newNetworkStateLists[CrOnc.Type.ETHERNET].concat( 
>     newNetworkStateLists[CrOnc.Type.WI_FI]).concat(   
>     newNetworkStateLists[CrOnc.Type.CELLULAR]).concat(   
>     newNetworkStateLists[CrOnc.Type.WI_MAX]).concat(   
>     newNetworkStateLists[CrOnc.Type.VPN]);
> Which seems like it should be more efficient (though
> I haven't timed them). The sort is averaging ~7 function
> calls per item and function calls are generally slow
> in JS.
> 
> (My concern may also be tainted by my experiences 
> commuting on the bus where hundreds networks will come 
> and go while driving down the street).

Whoops that ~7 is mixing JS calls and array iterations.
That's wrong. Still, avoiding a sort is likely to be good.

[stevenjb, 2016-08-30 15:36:49]

https://codereview.chromium.org/2290853002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_summary.js (right):

https://codereview.chromium.org/2290853002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary.js:338: /**
@type {!NetworkStateListObject} */ var newNetworkStateLists = {
On 2016/08/30 00:50:01, dschuyler wrote:
> Optional: I'm used to seeing the var on the
> line after the type comment (unless it's a 
> cast).

This is how clang formats it. I do not argue with clang.

https://codereview.chromium.org/2290853002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary.js:390: });
On 2016/08/30 00:55:37, dschuyler wrote:
> On 2016/08/30 00:50:01, dschuyler wrote:
> > This looks like the same sort order could be gained
> > from newNetworkStateLists by concatenating the arrays
> > akin to 
> > foo = newNetworkStateLists[CrOnc.Type.ETHERNET].concat( 
> >     newNetworkStateLists[CrOnc.Type.WI_FI]).concat(   
> >     newNetworkStateLists[CrOnc.Type.CELLULAR]).concat(   
> >     newNetworkStateLists[CrOnc.Type.WI_MAX]).concat(   
> >     newNetworkStateLists[CrOnc.Type.VPN]);
> > Which seems like it should be more efficient (though
> > I haven't timed them). The sort is averaging ~7 function
> > calls per item and function calls are generally slow
> > in JS.
> > 
> > (My concern may also be tainted by my experiences 
> > commuting on the bus where hundreds networks will come 
> > and go while driving down the street).
> 
> Whoops that ~7 is mixing JS calls and array iterations.
> That's wrong. Still, avoiding a sort is likely to be good.

The sort is likely to be trivial compared to the moving of data that goes on in
this (effectively infrequent) update. Also, the array length is the number of
network *types* not the number of networks.

The problem with using concat is that some types will not exist, so every type
would have to tested, otherwise we need to deal with undefined entries in the
array.

[dschuyler, 2016-08-30 22:45:19]

lgtm

[stevenjb, 2016-08-31 17:44:37]

He Dave, a slightly simpler way occurred to me to order the active networks
without sorting. Let me know if you think this is clearer, if not I can revert
it. Thanks!

[dschuyler, 2016-09-01 18:30:23]

:) LGTM

[stevenjb, 2016-09-01 19:27:03]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2016-09-01 19:27:22]

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-09-01 20:45:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-01 20:47:01]

Description was changed from

==========
MD Settings: Use network name for detail page title

This CL also correctly sorts network types in the main page.

BUG=609156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Use network name for detail page title

This CL also correctly sorts network types in the main page.

BUG=609156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/05803f278e293ff6d07d4df206d1a2dc3dded7c3
Cr-Commit-Position: refs/heads/master@{#416040}
==========

[commit-bot: I haz the power, 2016-09-01 20:47:02]

Patchset 4 (id:??) landed as
https://crrev.com/05803f278e293ff6d07d4df206d1a2dc3dded7c3
Cr-Commit-Position: refs/heads/master@{#416040}