Issue 2848683003: MD Settings: Add settings-internet-config (WiFi only, no certs)

Issue 2848683003: MD Settings: Add settings-internet-config (WiFi only, no certs) (Closed)

Created:
3 years, 7 months ago by stevenjb

Modified:
3 years, 7 months ago

Reviewers:
tbarzic, michaelpg

CC:
chromium-reviews, extensions-reviews_chromium.org, michaelpg+watch-md-settings_chromium.org, dbeam+watch-closure_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, chromium-apps-reviews_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-settings_chromium.org, srahim+watch_chromium.org, stevenjb+watch-md-settings_chromium.org, jlklein+watch-closure_chromium.org

Target Ref:
refs/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

MD Settings: Add settings-internet-config (WiFi only, no certs) This introduces a network configuration subpage behind a flag. In order to keep the review reasonable, this does not yet support certificate selection, or Etherne, VPN, or WiMAX configuration. BUG=380937 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation // js_checker.py is broken, crbug.com/725971 NOPRESUBMIT=true Review-Url: https://codereview.chromium.org/2848683003 Cr-Commit-Position: refs/heads/master@{#474454} Committed: https://chromium.googlesource.com/chromium/src/+/e1d45738b6894f006efddd9a21ff9f6d89176427

Patch Set 1 #

Patch Set 2 : . #

Total comments: 17

Patch Set 3 : Rebase self review #

Patch Set 4 : Rebase #

Total comments: 1

Patch Set 5 : . #

Total comments: 52

Patch Set 6 : Rebase fix #

Patch Set 7 : Feedback, add network-confg-input + select #

Total comments: 24

Patch Set 8 : Rebase + Feedback #

Total comments: 14

Patch Set 9 : More Feedback #

Total comments: 2

Patch Set 10 : Rebase + add histogram enum #

Created: 3 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+943 lines, -59 lines)			Patch
M	chrome/app/settings_strings.grdp	View	1 2 3 4 5 6 7 8 9	5 chunks	+72 lines, -36 lines	0 comments	Download
M	chrome/browser/about_flags.cc	View	1 2 3 4 5 6 7 8 9	1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/browser/flag_descriptions.h	View	1 2 3 4 5 6 7 8 9	1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/browser/flag_descriptions.cc	View	1 2 3 4 5 6 7 8 9	1 chunk	+7 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/settings/internet_page/compiled_resources2.gyp	View	1 2 3 4 5 6	2 chunks	+25 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/settings/internet_page/internet_config.html	View	1 2 3 4 5 6 7 8	1 chunk	+100 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/settings/internet_page/internet_config.js	View	1 2 3 4 5 6 7 8	1 chunk	+478 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/settings/internet_page/internet_detail_page.html	View	1 2 3 4 5 6 7	2 chunks	+1 line, -6 lines	0 comments	Download
M	chrome/browser/resources/settings/internet_page/internet_detail_page.js	View	1 2 3	1 chunk	+4 lines, -1 line	0 comments	Download
M	chrome/browser/resources/settings/internet_page/internet_page.html	View	1 2 3 4 5 6 7	2 chunks	+8 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/settings/internet_page/internet_page.js	View	1 2 3 4 5 6 7 8	3 chunks	+31 lines, -1 line	0 comments	Download
M	chrome/browser/resources/settings/internet_page/internet_shared_css.html	View	1 2 3 4	1 chunk	+5 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/settings/internet_page/internet_subpage.js	View	1 2 3 4 5 6 7 8	2 chunks	+6 lines, -2 lines	0 comments	Download
A	chrome/browser/resources/settings/internet_page/network_config_input.html	View	1 2 3 4 5 6	1 chunk	+20 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/settings/internet_page/network_config_input.js	View	1 2 3 4 5 6	1 chunk	+21 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/settings/internet_page/network_config_select.html	View	1 2 3 4 5 6	1 chunk	+24 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/settings/internet_page/network_config_select.js	View	1 2 3 4 5 6 7	1 chunk	+62 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/settings/route.js	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/settings/settings_resources.grd	View	1 2 3 4 5 6	2 chunks	+18 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc	View	1 2 3 4 5 6 7 8 9	5 chunks	+29 lines, -12 lines	0 comments	Download
M	chromeos/chromeos_switches.h	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	chromeos/chromeos_switches.cc	View	1 2 3 4 5 6 7	1 chunk	+3 lines, -0 lines	0 comments	Download
M	tools/metrics/histograms/enums.xml	View	1 2 3 4 5 6 7 8 9	1 chunk	+1 line, -0 lines	0 comments	Download
M	ui/webui/resources/cr_elements/network/cr_onc_types.js	View	1 2 3 4 5 6 7 8	2 chunks	+20 lines, -1 line	0 comments	Download

Messages

Total messages: 26 (12 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

stevenjb

https://codereview.chromium.org/2848683003/diff/20001/chrome/browser/resources/settings/internet_page/internet_config.html File chrome/browser/resources/settings/internet_page/internet_config.html (right): https://codereview.chromium.org/2848683003/diff/20001/chrome/browser/resources/settings/internet_page/internet_config.html#newcode8 chrome/browser/resources/settings/internet_page/internet_config.html:8: <link rel="import" href="chrome://resources/polymer/v1_0/paper-toggle-button/paper-toggle-button.html"> include iron-flex-layout-classes.html https://codereview.chromium.org/2848683003/diff/20001/chrome/browser/resources/settings/internet_page/internet_config.html#newcode21 chrome/browser/resources/settings/internet_page/internet_config.html:21: <!-- Title ...

3 years, 7 months ago (2017-05-02 00:17:45 UTC) #2

michaelpg

Just a tad concerned about the complexity of the HTML/JS. https://codereview.chromium.org/2848683003/diff/80001/chrome/app/settings_strings.grdp File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2848683003/diff/80001/chrome/app/settings_strings.grdp#newcode1099 ...

3 years, 7 months ago (2017-05-18 21:14:23 UTC) #6

stevenjb

Good feedback, thanks. Apologies for the non trivial update. PTAL. https://codereview.chromium.org/2848683003/diff/80001/chrome/app/settings_strings.grdp File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2848683003/diff/80001/chrome/app/settings_strings.grdp#newcode1099 ...

3 years, 7 months ago (2017-05-19 23:35:35 UTC) #7

Good feedback, thanks. Apologies for the non trivial update. PTAL.

https://codereview.chromium.org/2848683003/diff/80001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2848683003/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:1099: <message
name="IDS_SETTINGS_INTERNET_CONFIG" desc="Name of the settings page for
configuring networks.">
On 2017/05/18 21:14:22, michaelpg wrote:
> nit: configuring *a* network (It's not wrong as written, but singular is
clearer
> IMO)

Done.

https://codereview.chromium.org/2848683003/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:1576: <message name="IDS_ONC_WIFI_SECURITY_EAP"
desc="In settings > Internet, a string specifying security type = EAP
(8021.X).">
On 2017/05/18 21:14:22, michaelpg wrote:
> nit: 802.1X?

Done.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1289: {"network-settings-config",
flag_descriptions::kNetworkSettingsConfigName,
On 2017/05/18 21:14:22, michaelpg wrote:
> any idea why these strings don't use the existing const char*?

Searchability maybe? I'm not actually sure.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:2906: const char
kNetworkSettingsConfigName[] =
On 2017/05/18 21:14:22, michaelpg wrote:
> why aren't these in the same order as the .h? (too lazy to check the rest of
the
> file)

I think the headers were sorted while this was in progress. I moved these to
match the header order.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:2910: "Enables the Settings based network
configraiton UI instead of the Views "
On 2017/05/18 21:14:22, michaelpg wrote:
> "configuration"

Done.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:2913: const char kQuickUnlockPin[] = "Quick
Unlock (PIN)";
On 2017/05/18 21:14:22, michaelpg wrote:
> dropping "Name" intentional? (again too lazy to check the .h)

Merge bug.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_config.html
(right):

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.html:57:
value="{{configProperties_.WiFi.SSID::change}}"
On 2017/05/18 21:14:23, michaelpg wrote:
> I just discovered bind-value:
> https://github.com/PolymerElements/iron-input/tree/1.x
> 
> Might be less error prone than using |value| with "::change" (in case we
forget
> the ::change sometime).

Well, one might make the same mistake with value= vs bind-value=, but I agree
that the ::change is definitely awkward.

Unfortunately bind-value only appears to work with actual <iron-input> elements,
and I seem to a recall there was a reason we don't uses that directly?

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.html:89: <div
hidden="[[!showEap_]]">
On 2017/05/18 21:14:23, michaelpg wrote:
> could this be dom-if?

Done.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.html:105:
value="{{configProperties_.WiFi.EAP.Inner::change}}"
On 2017/05/18 21:14:22, michaelpg wrote:
> This logic is a bit complex to follow but it seems possible to update the list
> of <option>s after the |value| is set. That could cause these <select>s to not
> show the correct option as selected. Does that happen here? (see
> https://bugs.chromium.org/p/chromium/issues/detail?id=658075#c7)

Yeah, this turns out to be a little subtly complicated, so thanks for noticing.
I did two things:
1) Ensured that EAP.Inner has a valid value when EAP.Outer changes.
2) Ensured that the select value is updated when the value or items list
changes.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.html:145: <div
class="settings-box" hidden="[[!showEap.AnonymousIdentity]]">
On 2017/05/18 21:14:22, michaelpg wrote:
> sigh... it would be nice if each of these extremely similar 10-line elements
> could just be, like, a "settings-input" component. I know that introduces
> performance concerns, but this is a lot of boilerplate so it seems cumbersome.
> 
> <settings-input hidden="[[!showEap.AnonymousIdentity]]"
>     value="{{configProperties_.WiFi.EAP.AnonymousIdentity"
>     label="$i18n{OncWifi-EAP-AnonymousIdentity}">
> </settings-input>
> 
> A man can dream...

You know, I feel like I have unlearned some concepts that make web components
appealing in the first place.

This is absolutely a good place for a couple of custom elements. Performance is
not the only consideration here, and ease of maintenance was one of the
motivations of doing all this in the first place.

So, done and done - I created two custom elements, one for input and one for
select. I made them network specific so that I can change them if I need to when
we do UX review for this, but we can consider generalizing at least the input
element later.

Thanks for pointing this out.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.html:161:
</template>
On 2017/05/18 21:14:22, michaelpg wrote:
> remove extra newline and </template>

Ah, merges. Done.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_config.js (right):

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.js:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
On 2017/05/18 21:14:23, michaelpg wrote:
> 2017

Done.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.js:39: * will
always be defined as the network type being configured.
On 2017/05/18 21:14:23, michaelpg wrote:
> "always": including when configuring a new network?
> 
> if so, remove |undefined in @type

Yes. Done. (It gets set in currentRouteChanged, so technically can be briefly
undefined, but closure doesn't seem to care, so... works for me).

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.js:68:
NetworkType_: {
On 2017/05/18 21:14:23, michaelpg wrote:
> This isn't really used, apart from passing WI_FI to isType_() which could more
> simply be "isWiFi_()". Consider removing/simplifying for now.

We will definitely have other values in the very soon. Removed CELLULAR which
won't be used and added a comment.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.js:89: value:
function() {
On 2017/05/18 21:14:23, michaelpg wrote:
> if @const, doesn't really need to be returned via a function()
> 
> same for the below props

Done.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.js:137: return
['Automatic', 'MD5', 'MSCHAP', 'MSCHAPv2', 'PAP', 'CHAP', 'GTC'];
On 2017/05/18 21:14:23, michaelpg wrote:
> Suggestion: these constants may be more readable as a simple JS constant at
the
> top of the file: 
>   var kEapInnerItemsTtls = ['...', ...];
>   // etc
> rather than fully fledged Polymer properties.
> 
> The file would certainly be smaller.
Hmm, the others are used directly in the html so need to be properties. The
inner ones do not, but I think I prefer the consistency. They are smaller now
that I am just declaring them inline instead of using a function.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.js:189: // Delay
navigating to allow other subpages to load first (e.g. when called
On 2017/05/18 21:14:23, michaelpg wrote:
> I don't understand why this is necessary -- what happens if we navigate
> immediately?

It's kind of an edge case, but if a page is left open to a no-longer visible
network we close it immediately which can look odd while the rest of Settings is
getting loaded. It's much more of an issue for the details page where this was
copied from, I'll go ahead and remove it here.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.js:192:
settings.navigateToPreviousRoute();
On 2017/05/18 21:14:23, michaelpg wrote:
> verify that we are still on NETWORK_CONFIG so we don't randomly go "back" from
> an unrelated page in response to getPropertiesCallback or something

Done.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.js:245: if
(eapOuter == 'PEAP' || eapOuter == 'EAP-TTLS') {
On 2017/05/18 21:14:23, michaelpg wrote:
> I think these strings are used frequently enough that they it'd be safer to
make
> them constants: kPeap, kEapTtls, etc.

Added to OncTypes.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.js:247:
this.configProperties_.WiFi.EAP.Inner = 'Automatic';
On 2017/05/18 21:14:23, michaelpg wrote:
> please consider splitting this function up -- it has too many branches to
easily
> follow

Done.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.js:259: // TODO:
also check login state.
On 2017/05/18 21:14:23, michaelpg wrote:
> (stevenjb)

Done.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.js:376: if
(security && security == CrOnc.Security.WPA_EAP) {
On 2017/05/18 21:14:23, michaelpg wrote:
> maybe consider early returns instead of nested ifs

Done.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.js:388:
this.showEap_ = showEap;
On 2017/05/18 21:14:23, michaelpg wrote:
> er, is this property not declared/documented?

Done.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.js:401: return
key;
On 2017/05/18 21:14:23, michaelpg wrote:
> Huh? Do we intentionally translate some strings but not others?

Added an assert.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_config.js:412: else if
(outer == 'EAP-TTLS')
On 2017/05/18 21:14:23, michaelpg wrote:
> nit: no else after return

Done.

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_detail_page.js
(right):

https://codereview.chromium.org/2848683003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.js:608:
chrome.send('configureNetwork', [this.guid]);
On 2017/05/18 21:14:23, michaelpg wrote:
> nit: add a "TODO(stevenjb): remove" in the C++ handler for this callback

That's kind of automatic with the switch handling; when the switch is removed
the property from loadTimeData will get removed and this will get removed. We
don't usually put a TODO everywhere we add a switch (and removing any
chrome.send call should include an audit of the code).

tbarzic

https://codereview.chromium.org/2848683003/diff/60001/chrome/app/settings_strings.grdp File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2848683003/diff/60001/chrome/app/settings_strings.grdp#newcode1567 chrome/app/settings_strings.grdp:1567: <message name="IDS_ONC_WIFI_SECURITY_NONE" desc="In settings > Internet, a string specifying ...

3 years, 7 months ago (2017-05-22 19:29:02 UTC) #8

michaelpg

this version is easier to follow, thanks! https://codereview.chromium.org/2848683003/diff/120001/chrome/browser/resources/settings/internet_page/internet_config.js File chrome/browser/resources/settings/internet_page/internet_config.js (right): https://codereview.chromium.org/2848683003/diff/120001/chrome/browser/resources/settings/internet_page/internet_config.js#newcode25 chrome/browser/resources/settings/internet_page/internet_config.js:25: * @private ...

3 years, 7 months ago (2017-05-22 20:01:53 UTC) #9

stevenjb

PTAL https://codereview.chromium.org/2848683003/diff/120001/chrome/browser/resources/settings/internet_page/internet_config.js File chrome/browser/resources/settings/internet_page/internet_config.js (right): https://codereview.chromium.org/2848683003/diff/120001/chrome/browser/resources/settings/internet_page/internet_config.js#newcode32 chrome/browser/resources/settings/internet_page/internet_config.js:32: * @type {!chrome.networkingPrivate.NetworkProperties|undefined} On 2017/05/22 19:29:02, tbarzic wrote: ...

3 years, 7 months ago (2017-05-22 20:36:41 UTC) #10

michaelpg

lgtm w/ some opinions https://codereview.chromium.org/2848683003/diff/140001/chrome/browser/resources/settings/internet_page/internet_config.js File chrome/browser/resources/settings/internet_page/internet_config.js (right): https://codereview.chromium.org/2848683003/diff/140001/chrome/browser/resources/settings/internet_page/internet_config.js#newcode214 chrome/browser/resources/settings/internet_page/internet_config.js:214: getPropertiesCallback_: function(properties) { this function ...

3 years, 7 months ago (2017-05-23 17:05:56 UTC) #11

tbarzic

https://codereview.chromium.org/2848683003/diff/140001/chrome/browser/resources/settings/internet_page/internet_config.js File chrome/browser/resources/settings/internet_page/internet_config.js (right): https://codereview.chromium.org/2848683003/diff/140001/chrome/browser/resources/settings/internet_page/internet_config.js#newcode178 chrome/browser/resources/settings/internet_page/internet_config.js:178: var type = typeParam ? afaik, type param could ...

3 years, 7 months ago (2017-05-23 18:36:39 UTC) #12

stevenjb

PTAL https://codereview.chromium.org/2848683003/diff/140001/chrome/browser/resources/settings/internet_page/internet_config.js File chrome/browser/resources/settings/internet_page/internet_config.js (right): https://codereview.chromium.org/2848683003/diff/140001/chrome/browser/resources/settings/internet_page/internet_config.js#newcode178 chrome/browser/resources/settings/internet_page/internet_config.js:178: var type = typeParam ? On 2017/05/23 18:36:38, ...

3 years, 7 months ago (2017-05-23 20:05:22 UTC) #13

PTAL

https://codereview.chromium.org/2848683003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/internet_page/internet_config.js (right):

https://codereview.chromium.org/2848683003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/internet_page/internet_config.js:178: var type
= typeParam ?
On 2017/05/23 18:36:38, tbarzic wrote:
> afaik, type param could still be set to something that does not map to any
> network type.
> 
> Can we verify that typeParam actually is a valid network type before setting
> |type|? Something like
> if (typeParam != CrOnc.Types.ALL &&
Object.values(CrOnc.Type).indexOf(typeParam)
> >= 0)
>   type = /** @type {!CrOnc.Type} */ (typeParam)
> else
>   type = CrOnc.Type.WI_FI

I guess I'll add a helper method to CrOnc and add an assert instead... this is
largely to satisfy Closure, but I guess you can't be too careful.

https://codereview.chromium.org/2848683003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/internet_page/internet_config.js:195:
this.networkingPrivate.getProperties(
On 2017/05/23 18:36:38, tbarzic wrote:
> should we disable 'Save' until trusted network properties are loaded
> (e.g. check connectIsEnabled_, that should return false until SSID is properly
> set)?
Yes. Done.

https://codereview.chromium.org/2848683003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/internet_page/internet_config.js:214:
getPropertiesCallback_: function(properties) {
On 2017/05/23 17:05:56, michaelpg wrote:
> this function doesn't have a clear purpose. i'd suggest restricting it to
> setting this element's properties (using the data in |properties|) -- stuff
that
> is computed based on those properties might reasonably be an some updateUI_
> function or in individual computed properties (see below)

Acknowledged.

https://codereview.chromium.org/2848683003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/internet_page/internet_config.js:222:
this.title_ = properties.Name || this.i18n('OncType' + properties.Type);
On 2017/05/23 17:05:56, michaelpg wrote:
> might make more sense as a computed property keyed off properties.*

Done.

https://codereview.chromium.org/2848683003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/internet_page/internet_config.js:239:
this.savedSecurity_ = properties.WiFi.Security || '';
On 2017/05/23 17:05:56, michaelpg wrote:
> also might make more sense as a computed property
A computed property would be confusing since this is the dictionary that is
modified when an property is changed in the UI, but I will move this to a
separate update method.

https://codereview.chromium.org/2848683003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/internet_page/internet_config.js:361:
assert(!this.guid_);
On 2017/05/23 18:36:38, tbarzic wrote:
> I'd consider disabling connect button at this point, to avoid sending multiple
> identical createNetwork requests

Done.

https://codereview.chromium.org/2848683003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/internet_page/internet_config.js:362: var
propertiesToSet = Object.assign({}, this.configProperties_);
On 2017/05/23 17:05:56, michaelpg wrote:
> this looks unnecessary

Done.

tbarzic

lgtm https://codereview.chromium.org/2848683003/diff/160001/chrome/browser/resources/settings/internet_page/internet_config.js File chrome/browser/resources/settings/internet_page/internet_config.js (right): https://codereview.chromium.org/2848683003/diff/160001/chrome/browser/resources/settings/internet_page/internet_config.js#newcode37 chrome/browser/resources/settings/internet_page/internet_config.js:37: propertiesReceived_: Boolean, optional suggestion: you could rename this ...

3 years, 7 months ago (2017-05-24 00:46:57 UTC) #14

stevenjb

https://codereview.chromium.org/2848683003/diff/160001/chrome/browser/resources/settings/internet_page/internet_config.js File chrome/browser/resources/settings/internet_page/internet_config.js (right): https://codereview.chromium.org/2848683003/diff/160001/chrome/browser/resources/settings/internet_page/internet_config.js#newcode37 chrome/browser/resources/settings/internet_page/internet_config.js:37: propertiesReceived_: Boolean, On 2017/05/24 00:46:57, tbarzic wrote: > optional ...

3 years, 7 months ago (2017-05-24 16:27:26 UTC) #15

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-24 19:07:41 UTC) #23

commit-bot: I haz the power

3 years, 7 months ago (2017-05-24 22:38:24 UTC) #26

Message was sent while issue was closed.

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/e1d45738b6894f006efddd9a21ff...

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages