Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
700383008:
Use setProperties for Cellular APN (Closed)
|
Created:
6 years, 1 month ago by stevenjb Modified:
5 years, 10 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, hashimoto+watch_chromium.org, stevenjb+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@issue_430113_internet_options_2 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse setProperties for Cellular APN
This actually represents a pretty huge milestone - no more usage of
{Unmanaged} NetworkConnectionHandler from InternetOptionsHandler!
Everything is now using setProperties which uses MNCH.
BUG=430113
Committed: https://crrev.com/24c82bc3a6b006c8792be56faa35b7deff8c0582
Cr-Commit-Position: refs/heads/master@{#313578}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Feedback #
Total comments: 2
Patch Set 3 : Convert selectApn, fix bugs #
Total comments: 31
Patch Set 4 : Feedback #Patch Set 5 : Rebase #Patch Set 6 : Allow Cellular in shill_property_util::CopyIdentifyingProperties to fix unit test #
Messages
Total messages: 26 (6 generated)
stevenjb@chromium.org changed reviewers: + armansito@chromium.org, pneubeck@chromium.org
lgtm
for far looks good to me. I can take another look after rebasing / changing from NULL-value to 'Automatic APN' https://codereview.chromium.org/700383008/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/700383008/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:812: var onc = this.onc_; used before declaration and not afterwards? https://codereview.chromium.org/700383008/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:857: // Clear APN lists, keep only last element that "other". i don't understand this sentence. could you rephrase it? https://codereview.chromium.org/700383008/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:874: this.userApnIndex_ = this.selectedApnIndex_; before this change, userApnIndex_ was not modified by initializeApnList_ if this.selectedApnIndex_ != -1 || !activeApn. Please check, whether you really wanted to do that. (I can't say as I don't understand the code :-) ) https://codereview.chromium.org/700383008/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:895: this.onc_.setManagedProperty('Cellular.APN', activeApn); IIRC, this will change to setProperty after the next rebase. https://codereview.chromium.org/700383008/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:898: oncData.setManagedProperty('Cellular.APN', activeApn); IIUC, you will change this from NULL to an 'Automatic APN' value? https://codereview.chromium.org/700383008/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:946: if (this.userApnIndex_ != -1) { Sorry. Even after staring at this for a while, I have no idea what's going on here. (Not because of your change but because of the existing weirdness of the old code). E.g. why is setApn_ modifying the dom tree?! What for is 'userApnIndex_' compared to 'selectedApnIndex_'... I think you will have to rely a lot on testing this (manually). https://codereview.chromium.org/700383008/diff/1/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (left): https://codereview.chromium.org/700383008/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:37: #include "chromeos/network/network_configuration_handler.h" yuhuu!
I need to get my hands on a device to test this with before committing, I'll add a comment to the CL at that point if you want to wait to re-review. https://codereview.chromium.org/700383008/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/700383008/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:812: var onc = this.onc_; On 2014/12/01 13:54:10, pneubeck wrote: > used before declaration and not afterwards? Rebase conflict. Fixed. https://codereview.chromium.org/700383008/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:857: // Clear APN lists, keep only last element that "other". On 2014/12/01 13:54:10, pneubeck wrote: > i don't understand this sentence. could you rephrase it? s/that/,/ https://codereview.chromium.org/700383008/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:874: this.userApnIndex_ = this.selectedApnIndex_; On 2014/12/01 13:54:10, pneubeck wrote: > before this change, userApnIndex_ was not modified by initializeApnList_ if > this.selectedApnIndex_ != -1 || !activeApn. > > Please check, whether you really wanted to do that. (I can't say as I don't > understand the code :-) ) Good catch, I'm not sure why/how this got moved here, but it belongs in populateApnList_. Moved. https://codereview.chromium.org/700383008/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:895: this.onc_.setManagedProperty('Cellular.APN', activeApn); On 2014/12/01 13:54:10, pneubeck wrote: > IIRC, this will change to setProperty after the next rebase. Acknowledged. https://codereview.chromium.org/700383008/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:898: oncData.setManagedProperty('Cellular.APN', activeApn); On 2014/12/01 13:54:10, pneubeck wrote: > IIUC, you will change this from NULL to an 'Automatic APN' value? I will need to test whether or not we can send an empty dictionary to Shill for "Automatic" (and confirm that you can live with that), otherwise we may need to add another Type property, set that, and add more functionality to the ONC translation. https://codereview.chromium.org/700383008/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:946: if (this.userApnIndex_ != -1) { On 2014/12/01 13:54:10, pneubeck wrote: > Sorry. Even after staring at this for a while, I have no idea what's going on > here. (Not because of your change but because of the existing weirdness of the > old code). > E.g. why is setApn_ modifying the dom tree?! What for is 'userApnIndex_' > compared to 'selectedApnIndex_'... > > I think you will have to rely a lot on testing this (manually). 'userApnIndex_' points to a user defined entry in the dropdown. I agree that this is awkward at best and plan to re-factor the UI in the redesign. For now I will add comments to help clarify things. https://codereview.chromium.org/700383008/diff/1/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (left): https://codereview.chromium.org/700383008/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:37: #include "chromeos/network/network_configuration_handler.h" On 2014/12/01 13:54:10, pneubeck wrote: > yuhuu! :)
lgtm if you decide to go the empty-dictionary route (which I would recommend not to do). https://codereview.chromium.org/700383008/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/700383008/diff/20001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:888: if (apnName) { is the 'else' case meant for clearing the APN settings? I don't care so much about policy in this case, but I wonder how this goes together with partial updates in 'setProperties'. The semantics that you require from networkingPrivate.setProperties is that an empty-dictionary assigned to a field X means clearing that field X (like Shill does it in some cases). That in turn means, if we ever want to merge the partial update with existing user settings in ONC, that we can't use DictionaryValue::MergeDictionary anymore. That is doable but IMO rather delicate and might be easy to implement and use incorrectly.
https://codereview.chromium.org/700383008/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/700383008/diff/20001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:888: if (apnName) { On 2015/01/13 09:48:15, pneubeck wrote: > is the 'else' case meant for clearing the APN settings? > > I don't care so much about policy in this case, but I wonder how this goes > together with partial updates in 'setProperties'. > > The semantics that you require from networkingPrivate.setProperties is that an > empty-dictionary assigned to a field X means clearing that field X (like Shill > does it in some cases). > That in turn means, if we ever want to merge the partial update with existing > user settings in ONC, that we can't use DictionaryValue::MergeDictionary > anymore. > > That is doable but IMO rather delicate and might be easy to implement and use > incorrectly. re: 'else': Correct. I can make that explicit or comment it better. re: semantics: Yes, we would have to be careful about any future merging of Shill->ONC dictionaries with partial updates, but that is already true to some extent with IP config (the explicit types may mitigate that, but I'm not sure that will 'just work'). We could add another explicit type here, but that seems... tedious. We could use 'null' to clear the property, but that didn't seem popular last time :) If you have a suggestion / preference I am happy to go with that.
On 2015/01/13 17:13:06, stevenjb wrote: > https://codereview.chromium.org/700383008/diff/20001/chrome/browser/resources... > File chrome/browser/resources/options/chromeos/internet_detail.js (right): > > https://codereview.chromium.org/700383008/diff/20001/chrome/browser/resources... > chrome/browser/resources/options/chromeos/internet_detail.js:888: if (apnName) { > On 2015/01/13 09:48:15, pneubeck wrote: > > is the 'else' case meant for clearing the APN settings? > > > > I don't care so much about policy in this case, but I wonder how this goes > > together with partial updates in 'setProperties'. > > > > The semantics that you require from networkingPrivate.setProperties is that an > > empty-dictionary assigned to a field X means clearing that field X (like Shill > > does it in some cases). > > That in turn means, if we ever want to merge the partial update with existing > > user settings in ONC, that we can't use DictionaryValue::MergeDictionary > > anymore. > > > > That is doable but IMO rather delicate and might be easy to implement and use > > incorrectly. > > re: 'else': Correct. I can make that explicit or comment it better. > > re: semantics: > > Yes, we would have to be careful about any future merging of Shill->ONC > dictionaries with partial updates, but that is already true to some extent with > IP config (the explicit types may mitigate that, but I'm not sure that will > 'just work'). Because of the explicit types, an update containing 'DHCP' will make any existing static config be ignored. An partial update to 'Static' without concrete static properties will reuse whatever values already exist (if we didn't clean them up before). > > We could add another explicit type here, but that seems... tedious. > > We could use 'null' to clear the property, but that didn't seem popular last > time :) I think that still holds as whatever semantics we choose, they should be consistent over all properties and not specific to a subdictionary. That's also why I think that the empty dictionary will be more complicated as we should eventually support it everywhere for consistency. > > If you have a suggestion / preference I am happy to go with that. I would suggest some explicit non-null value, e.g. like we did with IPConfig (maybe there are other values that make sense). I know it's more work now, but it appears to me to be the safest route.
On 2015/01/13 17:27:10, pneubeck wrote: > On 2015/01/13 17:13:06, stevenjb wrote: > > > https://codereview.chromium.org/700383008/diff/20001/chrome/browser/resources... > > File chrome/browser/resources/options/chromeos/internet_detail.js (right): > > > > > https://codereview.chromium.org/700383008/diff/20001/chrome/browser/resources... > > chrome/browser/resources/options/chromeos/internet_detail.js:888: if (apnName) > { > > On 2015/01/13 09:48:15, pneubeck wrote: > > > is the 'else' case meant for clearing the APN settings? > > > > > > I don't care so much about policy in this case, but I wonder how this goes > > > together with partial updates in 'setProperties'. > > > > > > The semantics that you require from networkingPrivate.setProperties is that > an > > > empty-dictionary assigned to a field X means clearing that field X (like > Shill > > > does it in some cases). > > > That in turn means, if we ever want to merge the partial update with > existing > > > user settings in ONC, that we can't use DictionaryValue::MergeDictionary > > > anymore. > > > > > > That is doable but IMO rather delicate and might be easy to implement and > use > > > incorrectly. > > > > re: 'else': Correct. I can make that explicit or comment it better. > > > > re: semantics: > > > > Yes, we would have to be careful about any future merging of Shill->ONC > > dictionaries with partial updates, but that is already true to some extent > with > > IP config (the explicit types may mitigate that, but I'm not sure that will > > 'just work'). > Because of the explicit types, an update containing 'DHCP' will make any > existing static config be ignored. > An partial update to 'Static' without concrete static properties will reuse > whatever values already exist (if we didn't clean them up before). > > > > > We could add another explicit type here, but that seems... tedious. > > > > We could use 'null' to clear the property, but that didn't seem popular last > > time :) > I think that still holds as whatever semantics we choose, they should be > consistent over all properties and not specific to a subdictionary. > That's also why I think that the empty dictionary will be more complicated as we > should eventually support it everywhere for consistency. > > > > > If you have a suggestion / preference I am happy to go with that. > > I would suggest some explicit non-null value, e.g. like we did with IPConfig > (maybe there are other values that make sense). I know it's more work now, but > it appears to me to be the safest route. btw. if your current CL works already, I'm fine with separating additional changes into new CLs.
On 2015/01/13 17:27:10, pneubeck wrote:
> I would suggest some explicit non-null value, e.g. like we did with IPConfig
> (maybe there are other values that make sense). I know it's more work now, but
> it appears to me to be the safest route.
Could you elaborate / be more specific? IPConfig was kind of complicated with
the different types for IP and NameServers (which is why I'm not sure that a
dictionary merge will 'just work', but that's a separate issue).
Are you suggesting:
a) Adding Cellular.APNType = [ 'User' | 'Default' ]
b) Something like Cellular.APN = { 'AccessPointName': '' } (which we may need to
handle explicitly in the ONC -> Shill translation)
?
On 2015/01/13 17:38:34, stevenjb wrote:
> On 2015/01/13 17:27:10, pneubeck wrote:
>
> > I would suggest some explicit non-null value, e.g. like we did with IPConfig
> > (maybe there are other values that make sense). I know it's more work now,
but
> > it appears to me to be the safest route.
>
> Could you elaborate / be more specific? IPConfig was kind of complicated with
> the different types for IP and NameServers (which is why I'm not sure that a
> dictionary merge will 'just work', but that's a separate issue).
I don't have enough context about the APN stuff to make a good concrete
suggestion.
I can do that tomorrow, or maybe Arman can help with that.
>
> Are you suggesting:
> a) Adding Cellular.APNType = [ 'User' | 'Default' ]
or maybe 'Automatic', 'Custom', ...
> b) Something like Cellular.APN = { 'AccessPointName': '' } (which we may need
to
> handle explicitly in the ONC -> Shill translation)
both sound good to me.
Are there more principally different settings or is it always one of 'Default'
or 'User'?
Are there some independent groups of settings like we had it with IPAddress vs.
Nameserver?
Patchset #3 (id:40001) has been deleted
stevenjb@chromium.org changed reviewers: + benchan@chromium.org
I discovered that I forgot to convert the 'selectApn_' method to use setProperties, and also found a few bugs when testing. (I realized that I can kind of test with an LTE pixel in that I can at least test defining and setting a custom APN with the --enable-carrier-switching flag set). PTAL benchan@ - can you test this on a device with more than one available APN? Thanks!
sorry for the many comments but this code is nearly impossible to review... mental interpreter running hot... I _think_ the latest changes are ok. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:71: return value ? String(value) : ''; btw. this function seems weird to me. this should probably be something like if (value === undefined || value === null) return ''; return String(value); ? otherwise things like stringFromValue(0) -> '' happen https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:796: populateApnList_: function(apnList) { nit: the semantics of this functions would be a bit simpler if it would only populate the entries from |apnList|. The 'user' entry could be added (in the same way as 'default') in initializeApnList_. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:799: var otherOption = apnSelector[0]; this only works if 'other' is the only option, thus this should at least be preceded by a assert(apnSelector.size() == 1) alternatively, don't rely on this fact and just say that you insert at the beginning of the list and rename 'otherOption' to 'oldFirstOption' or so. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:826: this.selectedApnIndex_ = i; this indexing relies on the list initially being empty. this should be documented/asserted or this assumption be removed. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:832: var activeOption = document.createElement('option'); activeOption -> userOption https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:855: while (apnSelector.length != 1) it's a rather fragile and harder to understand if this initialize code assumes that 'other' is the last entry. to more reliably (i.e. with less assumption) keep only the 'other' entry, you could do: var otherOption = $('select-apn-other-option'); while (apnSelector.length != 0) apnSelector.remove(0); apnSelector.add(otherOption); https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:864: var otherOption = apnSelector[0]; a whole bunch of code has to be read to understand why this indeed is the 'other' entry. please see the next few comments to simplify this. and please put a comment at the top of this function that explains what the result will look like. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:865: var activeOption = document.createElement('option'); nit: activeOption -> defaultOption https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:869: apnSelector.add(activeOption, otherOption); to prepend add the beginning of the list: apnSelector.add(activeOption, apnSelector[0]) https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:870: this.selectedApnIndex_ = apnSelector.length - 2; length is 2 here. could be simplified to this.selectedApnIndex_ = 0; https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:907: // Remove any 'user' entry. nit: can there be more than one 'user' entry? if not -> 'Remove the 'user' entry if it exists.' https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:919: this.setActiveApn_({}); // Use default so with 'Use default' you mean the 'default' entry if apnList is empty and the first item of apnList if its not empty. please clarify that in a comment. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:955: apnSelector.add(option, apnSelector[apnSelector.length - 1]); this is now putting the userOption in front of the last element. please unify this code with the code in populateApnList_. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:988: var apnDict; this code is only for pre-filling some edit fields at the moment you select the 'custom APN' option, i guess. if so, lgtm. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:990: apnDict = this.userApn_; this is rather confusing. there seem to be two static entries in the selector list: 'user' and 'other' (and 'default'). Why is now this.userApn_ relevant if 'other' is selected ? or is just the comment in line 987 wrong?
PTAL https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:71: return value ? String(value) : ''; On 2015/01/23 14:53:29, pneubeck wrote: > btw. this function seems weird to me. > this should probably be something like > if (value === undefined || value === null) > return ''; > return String(value); > ? > > otherwise things like > stringFromValue(0) -> '' > happen Acknowledged. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:796: populateApnList_: function(apnList) { On 2015/01/23 14:53:29, pneubeck wrote: > nit: > > the semantics of this functions would be a bit simpler if it would only populate > the entries from |apnList|. > The 'user' entry could be added (in the same way as 'default') in > initializeApnList_. If (when) I re-write this I would organize it very differently. I'm trying to slightly improve readability (mostly for my own sake) without completely re-writing it here. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:799: var otherOption = apnSelector[0]; On 2015/01/23 14:53:28, pneubeck wrote: > this only works if 'other' is the only option, thus this should at least be > preceded by a assert(apnSelector.size() == 1) > alternatively, don't rely on this fact and just say that you insert at the > beginning of the list and rename 'otherOption' to 'oldFirstOption' or so. Acknowledged. I'll add an assert. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:826: this.selectedApnIndex_ = i; On 2015/01/23 14:53:28, pneubeck wrote: > this indexing relies on the list initially being empty. > this should be documented/asserted or this assumption be removed. Ack. handled by same assert above. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:832: var activeOption = document.createElement('option'); On 2015/01/23 14:53:28, pneubeck wrote: > activeOption -> userOption Done. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:855: while (apnSelector.length != 1) On 2015/01/23 14:53:28, pneubeck wrote: > it's a rather fragile and harder to understand if this initialize code assumes > that 'other' is the last entry. > to more reliably (i.e. with less assumption) keep only the 'other' entry, you > could do: > > var otherOption = $('select-apn-other-option'); > while (apnSelector.length != 0) apnSelector.remove(0); > apnSelector.add(otherOption); Again, I would re-write this very differently, this evolved from where it started, I don't want to mess with the logic too much. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:864: var otherOption = apnSelector[0]; On 2015/01/23 14:53:29, pneubeck wrote: > a whole bunch of code has to be read to understand why this indeed is the > 'other' entry. please see the next few comments to simplify this. > and please put a comment at the top of this function that explains what the > result will look like. Ditto. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:865: var activeOption = document.createElement('option'); On 2015/01/23 14:53:29, pneubeck wrote: > nit: > activeOption -> defaultOption Done. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:869: apnSelector.add(activeOption, otherOption); On 2015/01/23 14:53:28, pneubeck wrote: > to prepend add the beginning of the list: > apnSelector.add(activeOption, apnSelector[0]) I'm not convinced that is more clear (not that any of this is especially clear). https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:870: this.selectedApnIndex_ = apnSelector.length - 2; On 2015/01/23 14:53:29, pneubeck wrote: > length is 2 here. could be simplified to > this.selectedApnIndex_ = 0; Yes... that is true here. Changed and added an assert. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:907: // Remove any 'user' entry. On 2015/01/23 14:53:28, pneubeck wrote: > nit: can there be more than one 'user' entry? if not -> 'Remove the 'user' entry > if it exists.' "any" can mean that, but that's kind of subtle in English, I'll reword. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:919: this.setActiveApn_({}); // Use default On 2015/01/23 14:53:28, pneubeck wrote: > so with 'Use default' you mean the 'default' entry if apnList is empty > and the first item of apnList if its not empty. > > please clarify that in a comment. Done. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:955: apnSelector.add(option, apnSelector[apnSelector.length - 1]); On 2015/01/23 14:53:29, pneubeck wrote: > this is now putting the userOption in front of the last element. > please unify this code with the code in populateApnList_. Done... we are now deep into the grey area of almost but not quite completely re-factoring this... https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:988: var apnDict; On 2015/01/23 14:53:28, pneubeck wrote: > this code is only for pre-filling some edit fields at the moment you select the > 'custom APN' option, i guess. > > if so, lgtm. Correct. https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:990: apnDict = this.userApn_; On 2015/01/23 14:53:29, pneubeck wrote: > this is rather confusing. > there seem to be two static entries in the selector list: 'user' and 'other' > (and 'default'). > Why is now this.userApn_ relevant if 'other' is selected ? > or is just the comment in line 987 wrong? I attempted to improve this. I'll add comments, but: * If we select 'Other' and have defined a 'User' option, populate the custom fields with 'User' under the assumption that the user wants to modify those. * If we do not have a 'User' option, populate it with the 'Active' value.
thanks for explaining what this actually does.
lgtm
I'd suggest taking special care that setDefaultApn_ / setActiveApn_({}) is
working.
https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):
https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:990: apnDict =
this.userApn_;
On 2015/01/23 20:25:33, stevenjb wrote:
> On 2015/01/23 14:53:29, pneubeck wrote:
> > this is rather confusing.
> > there seem to be two static entries in the selector list: 'user' and 'other'
> > (and 'default').
> > Why is now this.userApn_ relevant if 'other' is selected ?
> > or is just the comment in line 987 wrong?
>
> I attempted to improve this. I'll add comments, but:
> * If we select 'Other' and have defined a 'User' option, populate the custom
> fields with 'User' under the assumption that the user wants to modify those.
> * If we do not have a 'User' option, populate it with the 'Active' value.
thanks for clarifying.
On 2015/01/24 09:33:13, pneubeck wrote:
> thanks for explaining what this actually does.
> lgtm
> I'd suggest taking special care that setDefaultApn_ / setActiveApn_({}) is
> working.
>
>
https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources...
> File chrome/browser/resources/options/chromeos/internet_detail.js (right):
>
>
https://codereview.chromium.org/700383008/diff/60001/chrome/browser/resources...
> chrome/browser/resources/options/chromeos/internet_detail.js:990: apnDict =
> this.userApn_;
> On 2015/01/23 20:25:33, stevenjb wrote:
> > On 2015/01/23 14:53:29, pneubeck wrote:
> > > this is rather confusing.
> > > there seem to be two static entries in the selector list: 'user' and
'other'
> > > (and 'default').
> > > Why is now this.userApn_ relevant if 'other' is selected ?
> > > or is just the comment in line 987 wrong?
> >
> > I attempted to improve this. I'll add comments, but:
> > * If we select 'Other' and have defined a 'User' option, populate the custom
> > fields with 'User' under the assumption that the user wants to modify those.
> > * If we do not have a 'User' option, populate it with the 'Active' value.
>
> thanks for clarifying.
No problem. Thanks for helping me make this more clear; I'm more likely to
remember it all now when I go to re-write it in the new UI :)
I did specifically test that setting to {} clears the property in Shill.
benchang@ - have you had a chance to try this patch with an actual device with
multiple carrier options and verify that the APN is set correctly?
If it is easier I am fairly comfortable pushing the code as-is, I am reasonably
confident that this doesn't introduce any new bugs...
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/700383008/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/700383008/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/24c82bc3a6b006c8792be56faa35b7deff8c0582 Cr-Commit-Position: refs/heads/master@{#313578} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
