Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
364883002:
Replace additional network properties with ONC values (Closed)
|
Created:
6 years, 5 months ago by stevenjb Modified:
6 years, 5 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, stevenjb+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionReplace additional network properties with ONC values
BUG=279351
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281997
Patch Set 1 #Patch Set 2 : . #
Total comments: 8
Patch Set 3 : Address feedback #Patch Set 4 : Remove type restriction from RemoveConfiguration #Patch Set 5 : Clang fix #Patch Set 6 : Rebase #
Messages
Total messages: 18 (0 generated)
https://codereview.chromium.org/364883002/diff/10005/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/364883002/diff/10005/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:1145: $('operator-name').textContent = data.Cellular.ServingOperator.Name; could use the setContentOrHide function from below? https://codereview.chromium.org/364883002/diff/10005/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:1172: $('iccid').textContent = stringFromValue(data.Cellular.ICCID); stringFromValue not required here and in the next line? https://codereview.chromium.org/364883002/diff/10005/chromeos/network/network... File chromeos/network/network_configuration_handler.cc (right): https://codereview.chromium.org/364883002/diff/10005/chromeos/network/network... chromeos/network/network_configuration_handler.cc:317: InvokeErrorCallback( Why is that restriction necessary? I guess this is about the previous restriction from internet_options_handler. NCH is often used for internal actions as well and applying UI / extension API specific restrictions here is a confusing. MNCH is supposed to enforce user restrictions / enforce networkingPrivate API rules. And the RemoveConfiguration method there is so far a direct mapping to this one. How about restricting it there? Note also, that this is not accurate anymore: The EAP settings of Ethernet could are removable as well.
https://codereview.chromium.org/364883002/diff/10005/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/364883002/diff/10005/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:1145: $('operator-name').textContent = data.Cellular.ServingOperator.Name; On 2014/07/07 09:14:26, pneubeck wrote: > could use the setContentOrHide function from below? Logic is slightly different; these depend on whether or not the ServingOperator dictionary exists. https://codereview.chromium.org/364883002/diff/10005/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:1172: $('iccid').textContent = stringFromValue(data.Cellular.ICCID); On 2014/07/07 09:14:26, pneubeck wrote: > stringFromValue not required here and in the next line? Done. https://codereview.chromium.org/364883002/diff/10005/chromeos/network/network... File chromeos/network/network_configuration_handler.cc (right): https://codereview.chromium.org/364883002/diff/10005/chromeos/network/network... chromeos/network/network_configuration_handler.cc:317: InvokeErrorCallback( On 2014/07/07 09:14:26, pneubeck wrote: > Why is that restriction necessary? > > I guess this is about the previous restriction from internet_options_handler. > NCH is often used for internal actions as well and applying UI / extension API > specific restrictions here is a confusing. > > MNCH is supposed to enforce user restrictions / enforce networkingPrivate API > rules. And the RemoveConfiguration method there is so far a direct mapping to > this one. How about restricting it there? > > Note also, that this is not accurate anymore: The EAP settings of Ethernet could > are removable as well. This is intended to enforce Shill behavior, i.e. detect an error early, independent of any UI. We could eliminate it but then might get a less decipherable error down the line. I will change this to a blacklist instead of a whitelist.
lgtm https://codereview.chromium.org/364883002/diff/10005/chromeos/network/network... File chromeos/network/network_configuration_handler.cc (right): https://codereview.chromium.org/364883002/diff/10005/chromeos/network/network... chromeos/network/network_configuration_handler.cc:317: InvokeErrorCallback( On 2014/07/07 23:40:21, stevenjb wrote: > On 2014/07/07 09:14:26, pneubeck wrote: > > Why is that restriction necessary? > > > > I guess this is about the previous restriction from internet_options_handler. > > NCH is often used for internal actions as well and applying UI / extension API > > specific restrictions here is a confusing. > > > > MNCH is supposed to enforce user restrictions / enforce networkingPrivate API > > rules. And the RemoveConfiguration method there is so far a direct mapping to > > this one. How about restricting it there? > > > > Note also, that this is not accurate anymore: The EAP settings of Ethernet > could > > are removable as well. > > This is intended to enforce Shill behavior, i.e. detect an error early, > independent of any UI. AFAIK, it's not an error to remove the ProfileEntry in the shared profile for Ethernet. It should correctly remove all ethernet (not EAP) configuration. Whether that functionality is used by the UI or not should be irrelevant here. It could be used potentially. > We could eliminate it but then might get a less > decipherable error down the line. I will change this to a blacklist instead of a > whitelist. Anyways, this is only used only by internet_options_handler, so I don't feel strong about it. Please, at least document this restriction in the header.
https://codereview.chromium.org/364883002/diff/10005/chromeos/network/network... File chromeos/network/network_configuration_handler.cc (right): https://codereview.chromium.org/364883002/diff/10005/chromeos/network/network... chromeos/network/network_configuration_handler.cc:317: InvokeErrorCallback( On 2014/07/08 07:55:27, pneubeck wrote: > On 2014/07/07 23:40:21, stevenjb wrote: > > On 2014/07/07 09:14:26, pneubeck wrote: > > > Why is that restriction necessary? > > > > > > I guess this is about the previous restriction from > internet_options_handler. > > > NCH is often used for internal actions as well and applying UI / extension > API > > > specific restrictions here is a confusing. > > > > > > MNCH is supposed to enforce user restrictions / enforce networkingPrivate > API > > > rules. And the RemoveConfiguration method there is so far a direct mapping > to > > > this one. How about restricting it there? > > > > > > Note also, that this is not accurate anymore: The EAP settings of Ethernet > > could > > > are removable as well. > > > > This is intended to enforce Shill behavior, i.e. detect an error early, > > independent of any UI. > AFAIK, it's not an error to remove the ProfileEntry in the shared profile for > Ethernet. It should correctly remove all ethernet (not EAP) configuration. > Whether that functionality is used by the UI or not should be irrelevant here. > It could be used potentially. > > > We could eliminate it but then might get a less > > decipherable error down the line. I will change this to a blacklist instead of > a > > whitelist. > > Anyways, this is only used only by internet_options_handler, so I don't feel > strong about it. > Please, at least document this restriction in the header. This restriction is inherited from early days of Flimflam, so I am honestly not sure what Shill will do. I will go ahead and remove the restriction, we don't currently have any UI surface for it. If we add one and something breaks, we'll deal with it then.
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevenjb@chromium.org/364883002/50001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevenjb@chromium.org/364883002/90001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
The CQ bit was checked by pneubeck@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevenjb@chromium.org/364883002/90001
Message was sent while issue was closed.
Change committed as 281997 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
