Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
492383002:
Use ONC for Cellular APN and SimLock in Settings (Closed)
|
Created:
6 years, 4 months ago by stevenjb Modified:
6 years, 2 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, stevenjb+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org, Dan Beam Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionUse ONC for Cellular APN and SimLock in Settings
BUG=279351
For chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:
TBR=xiyuan@chromium.org
Committed: https://crrev.com/4ff8121cc52bb887c2a6d35c3eace08e11d841ca
Cr-Commit-Position: refs/heads/master@{#292245}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 22
Patch Set 4 : Feedback + fixes #Patch Set 5 : Rebase #Patch Set 6 : Rebase, Address feedback, fix compile errors #Patch Set 7 : Fix tests #
Total comments: 6
Messages
Total messages: 33 (2 generated)
armansito@ - PTAL pneubeck@ - Mostly FYI, but please do look at setManagedProperty in internet_detail.js at least. This eliminates the remaining shill property dependencies (not counting Managed properties and IP config properties). The next CL will switch to using ManagedNetworkConfigurationHandler and should (mostly) eliminate Shill dependencies for getters.
https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (left): https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:547: dictionary->SetString(kTagSupportUrl, url); This is unused in the JS code. https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:633: support_url.empty()) { Since the support URL is unused, it doesn't seem to make any sense to test for it when determining whether or not to display the show portal button below.
thieule@: PTAL at my comment regarding the payment url in internet_options_handler.cc. https://codereview.chromium.org/492383002/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/492383002/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:382: stringFromValue(apnDict['Name']), Shouldn't this be 'APN' rather than 'Name'? https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (left): https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:633: support_url.empty()) { On 2014/08/21 23:06:58, stevenjb wrote: > Since the support URL is unused, it doesn't seem to make any sense to test for > it when determining whether or not to display the show portal button below. Even though the settings JS isn't using it, I think we still need the check here. As far as I can remember, this is how the code distinguishes between activation flows that don't use the mobile setup dialog vs those that do. thieule@, can you comment on this?
https://codereview.chromium.org/492383002/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/492383002/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:127: (typeof data[key] != 'object') || !('Active' in data[key])) { Active might not be present if the value from Shill is not translated back, which should be legitimate in some cases. Checking 'Active' and 'Effective' should be sufficiently safe. https://codereview.chromium.org/492383002/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:130: // For now, just uodare the active value. TODO(stevenjb): Possibly check typo: uodare -> update https://codereview.chromium.org/492383002/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:132: data[key]['Active'] = value; If I get it right, then you want to use the ONC properties in |data| to store the transient state of the UI? To be consistent with the data structure as it is received initially from C++, this should update: 'Active' = value if (value != undefined) { 'UserSetting' = value 'Effective' = 'UserSetting' } else { remove 'UserSetting' if ('Effective' == 'UserSetting') { // In general, this case is problematic, as 'Effective' should now be set to something else than 'UserSetting'. // For Cellular where you know that there is no policy, you could just remove the 'Effective' field. } } Maybe you want to double-check that 'UserEditable' or 'DeviceEditable' was true and fail otherwise. A more reliable approach might be to send the update request to C++, and let C++ send a new dictionary (the whole one or only for this value) to JS. That way you would get information about whether the setting could be actually applied and you would not have to fake the data structure. https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/chromeos/network_config_message_handler.cc (right): https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:198: dictionary_copy->Set("Device", device_dictionary); nit: FWIW, in ManagedNetworkConfigurationHandlerImpl::GetPropertiesCallback, we used shill::kDeviceProperty as key constant. https://codereview.chromium.org/492383002/diff/40001/components/onc/onc_const... File components/onc/onc_constants.cc (right): https://codereview.chromium.org/492383002/diff/40001/components/onc/onc_const... components/onc/onc_constants.cc:119: const char kApn[] = "APN"; IMHO, the constant name and the value are very confusing: cellular_network = { ... "APN": { "apn": "...." ??? }, "APNList": [ { "apn": ... }, { } ] }; Looking into Shill's source of this property, unveils: (mobile_operator_info.h:104-109) // Encapsulates information on a mobile access point name. This information // is usually necessary for 3GPP networks to be able to connect to a mobile // network. So far, CDMA networks don't use this information. struct MobileAPN { // The access point url, which is fed to the modemmanager while connecting. std::string apn; How about "AccessPointUrl" then? @Arman, does that sound right to you?
https://codereview.chromium.org/492383002/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/492383002/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:127: (typeof data[key] != 'object') || !('Active' in data[key])) { On 2014/08/22 08:55:32, pneubeck wrote: > Active might not be present if the value from Shill is not translated back, > which should be legitimate in some cases. > > Checking 'Active' and 'Effective' should be sufficiently safe. Done. https://codereview.chromium.org/492383002/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:130: // For now, just uodare the active value. TODO(stevenjb): Possibly check On 2014/08/22 08:55:32, pneubeck wrote: > typo: uodare -> update Done. https://codereview.chromium.org/492383002/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:132: data[key]['Active'] = value; On 2014/08/22 08:55:32, pneubeck wrote: > If I get it right, then you want to use the ONC properties in |data| to store > the transient state of the UI? > > To be consistent with the data structure as it is received initially from C++, > this should update: > > 'Active' = value > if (value != undefined) { > 'UserSetting' = value > 'Effective' = 'UserSetting' > } else { > remove 'UserSetting' > if ('Effective' == 'UserSetting') { > // In general, this case is problematic, as 'Effective' should now be set to > something else than 'UserSetting'. > // For Cellular where you know that there is no policy, you could just > remove the 'Effective' field. > } > } > Maybe you want to double-check that 'UserEditable' or 'DeviceEditable' was true > and fail otherwise. > > > A more reliable approach might be to send the update request to C++, and let C++ > send a new dictionary (the whole one or only for this value) to JS. That way you > would get information about whether the setting could be actually applied and > you would not have to fake the data structure. My intention was to do something better than clobber any ONC properties, but not as thorough as you describe (thus the TODO). This is only designed to update the locally cached value. I will add a couple of asserts, but we certainly don't want to do a round-trip here, that's a level of refactoring I think we should avoid until we have eliminated the C++ glue code. https://codereview.chromium.org/492383002/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:382: stringFromValue(apnDict['Name']), On 2014/08/22 00:56:45, armansito wrote: > Shouldn't this be 'APN' rather than 'Name'? Done. https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/chromeos/network_config_message_handler.cc (right): https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/network_config_message_handler.cc:198: dictionary_copy->Set("Device", device_dictionary); On 2014/08/22 08:55:32, pneubeck wrote: > nit: FWIW, in ManagedNetworkConfigurationHandlerImpl::GetPropertiesCallback, we > used shill::kDeviceProperty as key constant. Done. https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (left): https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:633: support_url.empty()) { On 2014/08/22 00:56:45, armansito wrote: > On 2014/08/21 23:06:58, stevenjb wrote: > > Since the support URL is unused, it doesn't seem to make any sense to test for > > it when determining whether or not to display the show portal button below. > > Even though the settings JS isn't using it, I think we still need the check > here. As far as I can remember, this is how the code distinguishes between > activation flows that don't use the mobile setup dialog vs those that do. > > thieule@, can you comment on this? According to service_constants.h, kPaymentPortalProperty is deprecated. showMorePlanInfo controls a button that calls ShowMorePlanInfoCallback which calls ash::network_connect::ShowMobileSetup(), which checks activation state and calls MobileSetupDialog::Show(). So, it seems that relying on MobileConfig::GetInstance()->GetCarrier(carrier_id)->show_portal_button() should be sufficient, i.e. any logic to determine whether or not the button should be visible should be done there. https://codereview.chromium.org/492383002/diff/40001/components/onc/onc_const... File components/onc/onc_constants.cc (right): https://codereview.chromium.org/492383002/diff/40001/components/onc/onc_const... components/onc/onc_constants.cc:119: const char kApn[] = "APN"; On 2014/08/22 08:55:32, pneubeck wrote: > IMHO, the constant name and the value are very confusing: > cellular_network = { > ... > "APN": { > "apn": "...." ??? > }, > "APNList": [ { > "apn": ... > }, { > } > ] > }; > > Looking into Shill's source of this property, unveils: > (mobile_operator_info.h:104-109) > // Encapsulates information on a mobile access point name. This information > // is usually necessary for 3GPP networks to be able to connect to a mobile > // network. So far, CDMA networks don't use this information. > struct MobileAPN { > // The access point url, which is fed to the modemmanager while connecting. > std::string apn; > > How about "AccessPointUrl" then? > @Arman, does that sound right to you? Happy to change this, Arman let me know what you suggest.
https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (left): https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:633: support_url.empty()) { We should be using the new kActivationType property to determine the type of activation flow. On 2014/08/22 00:56:45, armansito wrote: > On 2014/08/21 23:06:58, stevenjb wrote: > > Since the support URL is unused, it doesn't seem to make any sense to test for > > it when determining whether or not to display the show portal button below. > > Even though the settings JS isn't using it, I think we still need the check > here. As far as I can remember, this is how the code distinguishes between > activation flows that don't use the mobile setup dialog vs those that do. > > thieule@, can you comment on this?
https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (left): https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:633: support_url.empty()) { On 2014/08/22 19:13:04, thieule wrote: > We should be using the new kActivationType property to determine the type of > activation flow. > > > On 2014/08/22 00:56:45, armansito wrote: > > On 2014/08/21 23:06:58, stevenjb wrote: > > > Since the support URL is unused, it doesn't seem to make any sense to test > for > > > it when determining whether or not to display the show portal button below. > > > > Even though the settings JS isn't using it, I think we still need the check > > here. As far as I can remember, this is how the code distinguishes between > > activation flows that don't use the mobile setup dialog vs those that do. > > > > thieule@, can you comment on this? > So... is there an additional check that we should be doing here or not?
https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (left): https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:633: support_url.empty()) { > According to service_constants.h, kPaymentPortalProperty is deprecated. > Yeah...I wouldn't trust that. Then again I think the actual payment portal URL is the Cellular.OLP property? > showMorePlanInfo controls a button that calls ShowMorePlanInfoCallback which > calls ash::network_connect::ShowMobileSetup(), which checks activation state and > calls MobileSetupDialog::Show(). > > So, it seems that relying on > MobileConfig::GetInstance()->GetCarrier(carrier_id)->show_portal_button() should > be sufficient, i.e. any logic to determine whether or not the button should be > visible should be done there. Yeah, I guess this should suffice then. I think we don't really provide that support URL thing on any of the newer versions (or carriers) so I think this should be OK. I don't know if there will be any regressions (perhaps some older plans) but might as well commit this. https://codereview.chromium.org/492383002/diff/40001/components/onc/onc_const... File components/onc/onc_constants.cc (right): https://codereview.chromium.org/492383002/diff/40001/components/onc/onc_const... components/onc/onc_constants.cc:119: const char kApn[] = "APN"; On 2014/08/22 08:55:32, pneubeck wrote: > IMHO, the constant name and the value are very confusing: > cellular_network = { > ... > "APN": { > "apn": "...." ??? > }, > "APNList": [ { > "apn": ... > }, { > } > ] > }; > > Looking into Shill's source of this property, unveils: > (mobile_operator_info.h:104-109) > // Encapsulates information on a mobile access point name. This information > // is usually necessary for 3GPP networks to be able to connect to a mobile > // network. So far, CDMA networks don't use this information. > struct MobileAPN { > // The access point url, which is fed to the modemmanager while connecting. > std::string apn; > > How about "AccessPointUrl" then? > @Arman, does that sound right to you? It's not really a URL though. The thing is, APN stands for "Access Point Name" and I think that's why we went with "Name" as the field there. I think that "AccessPointName" definitely sounds better than just "Name", so let's go with that for now.
https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (left): https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:633: support_url.empty()) { On 2014/08/25 21:48:50, armansito wrote: > > > According to service_constants.h, kPaymentPortalProperty is deprecated. > > > > Yeah...I wouldn't trust that. Then again I think the actual payment portal URL > is the Cellular.OLP property? > > > > showMorePlanInfo controls a button that calls ShowMorePlanInfoCallback which > > calls ash::network_connect::ShowMobileSetup(), which checks activation state > and > > calls MobileSetupDialog::Show(). > > > > So, it seems that relying on > > MobileConfig::GetInstance()->GetCarrier(carrier_id)->show_portal_button() > should > > be sufficient, i.e. any logic to determine whether or not the button should be > > visible should be done there. > > Yeah, I guess this should suffice then. I think we don't really provide that > support URL thing on any of the newer versions (or carriers) so I think this > should be OK. I don't know if there will be any regressions (perhaps some older > plans) but might as well commit this. > So, I looked at the code again and realized that mobile_setup_ui.cc is independent of mobile_confg_ui.cc. I think we need to do some cleanup of that code at some point, but for now I am going to try to preserve the existing logic. https://codereview.chromium.org/492383002/diff/40001/components/onc/onc_const... File components/onc/onc_constants.cc (right): https://codereview.chromium.org/492383002/diff/40001/components/onc/onc_const... components/onc/onc_constants.cc:119: const char kApn[] = "APN"; On 2014/08/25 21:48:50, armansito wrote: > On 2014/08/22 08:55:32, pneubeck wrote: > > IMHO, the constant name and the value are very confusing: > > cellular_network = { > > ... > > "APN": { > > "apn": "...." ??? > > }, > > "APNList": [ { > > "apn": ... > > }, { > > } > > ] > > }; > > > > Looking into Shill's source of this property, unveils: > > (mobile_operator_info.h:104-109) > > // Encapsulates information on a mobile access point name. This information > > // is usually necessary for 3GPP networks to be able to connect to a mobile > > // network. So far, CDMA networks don't use this information. > > struct MobileAPN { > > // The access point url, which is fed to the modemmanager while > connecting. > > std::string apn; > > > > How about "AccessPointUrl" then? > > @Arman, does that sound right to you? > > It's not really a URL though. The thing is, APN stands for "Access Point Name" > and I think that's why we went with "Name" as the field there. I think that > "AccessPointName" definitely sounds better than just "Name", so let's go with > that for now. From shill/doc/device-api.txt: "apn" The APN, to be used when making "name" Optional description of the APN, not localized. "localized_name" Optional description of the APN, localized. Maybe spell out AccessPointName to separate it from the wrapper object, i.e. APN.AccessPointName?
PTAL
On 2014/08/25 22:32:35, stevenjb wrote: > https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... > File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc > (left): > > https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... > chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:633: > support_url.empty()) { > On 2014/08/25 21:48:50, armansito wrote: > > > > > According to service_constants.h, kPaymentPortalProperty is deprecated. > > > > > > > Yeah...I wouldn't trust that. Then again I think the actual payment portal URL > > is the Cellular.OLP property? > > > > > > > showMorePlanInfo controls a button that calls ShowMorePlanInfoCallback which > > > calls ash::network_connect::ShowMobileSetup(), which checks activation state > > and > > > calls MobileSetupDialog::Show(). > > > > > > So, it seems that relying on > > > MobileConfig::GetInstance()->GetCarrier(carrier_id)->show_portal_button() > > should > > > be sufficient, i.e. any logic to determine whether or not the button should > be > > > visible should be done there. > > > > Yeah, I guess this should suffice then. I think we don't really provide that > > support URL thing on any of the newer versions (or carriers) so I think this > > should be OK. I don't know if there will be any regressions (perhaps some > older > > plans) but might as well commit this. > > > > So, I looked at the code again and realized that mobile_setup_ui.cc is > independent of mobile_confg_ui.cc. I think we need to do some cleanup of that > code at some point, but for now I am going to try to preserve the existing > logic. > > https://codereview.chromium.org/492383002/diff/40001/components/onc/onc_const... > File components/onc/onc_constants.cc (right): > > https://codereview.chromium.org/492383002/diff/40001/components/onc/onc_const... > components/onc/onc_constants.cc:119: const char kApn[] = "APN"; > On 2014/08/25 21:48:50, armansito wrote: > > On 2014/08/22 08:55:32, pneubeck wrote: > > > IMHO, the constant name and the value are very confusing: > > > cellular_network = { > > > ... > > > "APN": { > > > "apn": "...." ??? > > > }, > > > "APNList": [ { > > > "apn": ... > > > }, { > > > } > > > ] > > > }; > > > > > > Looking into Shill's source of this property, unveils: > > > (mobile_operator_info.h:104-109) > > > // Encapsulates information on a mobile access point name. This > information > > > // is usually necessary for 3GPP networks to be able to connect to a > mobile > > > // network. So far, CDMA networks don't use this information. > > > struct MobileAPN { > > > // The access point url, which is fed to the modemmanager while > > connecting. > > > std::string apn; > > > > > > How about "AccessPointUrl" then? > > > @Arman, does that sound right to you? > > > > It's not really a URL though. The thing is, APN stands for "Access Point Name" > > and I think that's why we went with "Name" as the field there. I think that > > "AccessPointName" definitely sounds better than just "Name", so let's go with > > that for now. > > From shill/doc/device-api.txt: > > "apn" The APN, to be used when making > "name" Optional description of the APN, not localized. > "localized_name" Optional description of the APN, localized. > > Maybe spell out AccessPointName to separate it from the wrapper object, i.e. > APN.AccessPointName? That's what I meant :) Let's spell it out in the ONC spec.
On 2014/08/26 00:05:26, armansito wrote: > On 2014/08/25 22:32:35, stevenjb wrote: > > > https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... > > File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc > > (left): > > > > > https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... > > chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:633: > > support_url.empty()) { > > On 2014/08/25 21:48:50, armansito wrote: > > > > > > > According to service_constants.h, kPaymentPortalProperty is deprecated. > > > > > > > > > > Yeah...I wouldn't trust that. Then again I think the actual payment portal > URL > > > is the Cellular.OLP property? > > > > > > > > > > showMorePlanInfo controls a button that calls ShowMorePlanInfoCallback > which > > > > calls ash::network_connect::ShowMobileSetup(), which checks activation > state > > > and > > > > calls MobileSetupDialog::Show(). > > > > > > > > So, it seems that relying on > > > > MobileConfig::GetInstance()->GetCarrier(carrier_id)->show_portal_button() > > > should > > > > be sufficient, i.e. any logic to determine whether or not the button > should > > be > > > > visible should be done there. > > > > > > Yeah, I guess this should suffice then. I think we don't really provide that > > > support URL thing on any of the newer versions (or carriers) so I think this > > > should be OK. I don't know if there will be any regressions (perhaps some > > older > > > plans) but might as well commit this. > > > > > > > So, I looked at the code again and realized that mobile_setup_ui.cc is > > independent of mobile_confg_ui.cc. I think we need to do some cleanup of that > > code at some point, but for now I am going to try to preserve the existing > > logic. > > > > > https://codereview.chromium.org/492383002/diff/40001/components/onc/onc_const... > > File components/onc/onc_constants.cc (right): > > > > > https://codereview.chromium.org/492383002/diff/40001/components/onc/onc_const... > > components/onc/onc_constants.cc:119: const char kApn[] = "APN"; > > On 2014/08/25 21:48:50, armansito wrote: > > > On 2014/08/22 08:55:32, pneubeck wrote: > > > > IMHO, the constant name and the value are very confusing: > > > > cellular_network = { > > > > ... > > > > "APN": { > > > > "apn": "...." ??? > > > > }, > > > > "APNList": [ { > > > > "apn": ... > > > > }, { > > > > } > > > > ] > > > > }; > > > > > > > > Looking into Shill's source of this property, unveils: > > > > (mobile_operator_info.h:104-109) > > > > // Encapsulates information on a mobile access point name. This > > information > > > > // is usually necessary for 3GPP networks to be able to connect to a > > mobile > > > > // network. So far, CDMA networks don't use this information. > > > > struct MobileAPN { > > > > // The access point url, which is fed to the modemmanager while > > > connecting. > > > > std::string apn; > > > > > > > > How about "AccessPointUrl" then? > > > > @Arman, does that sound right to you? > > > > > > It's not really a URL though. The thing is, APN stands for "Access Point > Name" > > > and I think that's why we went with "Name" as the field there. I think that > > > "AccessPointName" definitely sounds better than just "Name", so let's go > with > > > that for now. > > > > From shill/doc/device-api.txt: > > > > "apn" The APN, to be used when making > > "name" Optional description of the APN, not localized. > > "localized_name" Optional description of the APN, localized. > > > > Maybe spell out AccessPointName to separate it from the wrapper object, i.e. > > APN.AccessPointName? > > That's what I meant :) Let's spell it out in the ONC spec. lgtm once you rename it to AccessPointName.
On 2014/08/26 00:07:10, armansito wrote: > On 2014/08/26 00:05:26, armansito wrote: > > On 2014/08/25 22:32:35, stevenjb wrote: > > > > > > https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... > > > File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc > > > (left): > > > > > > > > > https://codereview.chromium.org/492383002/diff/40001/chrome/browser/ui/webui/... > > > chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:633: > > > support_url.empty()) { > > > On 2014/08/25 21:48:50, armansito wrote: > > > > > > > > > According to service_constants.h, kPaymentPortalProperty is deprecated. > > > > > > > > > > > > > Yeah...I wouldn't trust that. Then again I think the actual payment portal > > URL > > > > is the Cellular.OLP property? > > > > > > > > > > > > > showMorePlanInfo controls a button that calls ShowMorePlanInfoCallback > > which > > > > > calls ash::network_connect::ShowMobileSetup(), which checks activation > > state > > > > and > > > > > calls MobileSetupDialog::Show(). > > > > > > > > > > So, it seems that relying on > > > > > > MobileConfig::GetInstance()->GetCarrier(carrier_id)->show_portal_button() > > > > should > > > > > be sufficient, i.e. any logic to determine whether or not the button > > should > > > be > > > > > visible should be done there. > > > > > > > > Yeah, I guess this should suffice then. I think we don't really provide > that > > > > support URL thing on any of the newer versions (or carriers) so I think > this > > > > should be OK. I don't know if there will be any regressions (perhaps some > > > older > > > > plans) but might as well commit this. > > > > > > > > > > So, I looked at the code again and realized that mobile_setup_ui.cc is > > > independent of mobile_confg_ui.cc. I think we need to do some cleanup of > that > > > code at some point, but for now I am going to try to preserve the existing > > > logic. > > > > > > > > > https://codereview.chromium.org/492383002/diff/40001/components/onc/onc_const... > > > File components/onc/onc_constants.cc (right): > > > > > > > > > https://codereview.chromium.org/492383002/diff/40001/components/onc/onc_const... > > > components/onc/onc_constants.cc:119: const char kApn[] = "APN"; > > > On 2014/08/25 21:48:50, armansito wrote: > > > > On 2014/08/22 08:55:32, pneubeck wrote: > > > > > IMHO, the constant name and the value are very confusing: > > > > > cellular_network = { > > > > > ... > > > > > "APN": { > > > > > "apn": "...." ??? > > > > > }, > > > > > "APNList": [ { > > > > > "apn": ... > > > > > }, { > > > > > } > > > > > ] > > > > > }; > > > > > > > > > > Looking into Shill's source of this property, unveils: > > > > > (mobile_operator_info.h:104-109) > > > > > // Encapsulates information on a mobile access point name. This > > > information > > > > > // is usually necessary for 3GPP networks to be able to connect to a > > > mobile > > > > > // network. So far, CDMA networks don't use this information. > > > > > struct MobileAPN { > > > > > // The access point url, which is fed to the modemmanager while > > > > connecting. > > > > > std::string apn; > > > > > > > > > > How about "AccessPointUrl" then? > > > > > @Arman, does that sound right to you? > > > > > > > > It's not really a URL though. The thing is, APN stands for "Access Point > > Name" > > > > and I think that's why we went with "Name" as the field there. I think > that > > > > "AccessPointName" definitely sounds better than just "Name", so let's go > > with > > > > that for now. > > > > > > From shill/doc/device-api.txt: > > > > > > "apn" The APN, to be used when making > > > "name" Optional description of the APN, not localized. > > > "localized_name" Optional description of the APN, localized. > > > > > > Maybe spell out AccessPointName to separate it from the wrapper object, i.e. > > > APN.AccessPointName? > > > > That's what I meant :) Let's spell it out in the ONC spec. > > lgtm once you rename it to AccessPointName. AccessPointName should already be spelled out in this patch, please let me know if I missed anywhere. Note: Cellular is currently undocumented in onc_spec.html. I think we should add the documentation, including APN, in a separate CL.
lgtm. We should definitely add documentation for Cellular in the future and at least mention that it only includes read-only fields.
lgtm
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/492383002/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming 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-status.appspot.com/cq/stevenjb@chromium.org/492383002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as 0c079538e35cbe1232438c302ed84499e79f6f30
Message was sent while issue was closed.
vitalyp@chromium.org changed reviewers: + vitalyp@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/492383002/diff/120001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/492383002/diff/120001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:319: activeApn['AccessPointName'] = Could you please explain, why |activeApn| is defined and is an object at this point? I see only declaration on line 313, according to which it seems to be "undefined" at line 319.
Message was sent while issue was closed.
https://codereview.chromium.org/492383002/diff/120001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/492383002/diff/120001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:319: activeApn['AccessPointName'] = On 2014/09/07 05:25:15, Vitaly Pavlenko wrote: > Could you please explain, why |activeApn| is defined and is an object at this > point? I see only declaration on line 313, according to which it seems to be > "undefined" at line 319. activeApn will either be 'undefined' or will be a valid APN object when it is used in line 328. We set it to 'undefined' rather than providing a special 'unsetManagedProperty' method for simplicity.
Message was sent while issue was closed.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/492383002/diff/120001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/492383002/diff/120001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:319: activeApn['AccessPointName'] = On 2014/09/08 17:48:22, stevenjb wrote: > On 2014/09/07 05:25:15, Vitaly Pavlenko wrote: > > Could you please explain, why |activeApn| is defined and is an object at this > > point? I see only declaration on line 313, according to which it seems to be > > "undefined" at line 319. > > activeApn will either be 'undefined' or will be a valid APN object when it is > used in line 328. We set it to 'undefined' rather than providing a special > 'unsetManagedProperty' method for simplicity. i think what vitalyp@ means is: var activeApn; // undefined if (...) { activeApn['AccessPointName'] // undefined['key'] should blow up }
Message was sent while issue was closed.
https://codereview.chromium.org/492383002/diff/120001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/492383002/diff/120001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:319: activeApn['AccessPointName'] = On 2014/09/08 20:32:14, Dan Beam wrote: > On 2014/09/08 17:48:22, stevenjb wrote: > > On 2014/09/07 05:25:15, Vitaly Pavlenko wrote: > > > Could you please explain, why |activeApn| is defined and is an object at > this > > > point? I see only declaration on line 313, according to which it seems to be > > > "undefined" at line 319. > > > > activeApn will either be 'undefined' or will be a valid APN object when it is > > used in line 328. We set it to 'undefined' rather than providing a special > > 'unsetManagedProperty' method for simplicity. > > i think what vitalyp@ means is: > > var activeApn; // undefined > if (...) { > activeApn['AccessPointName'] // undefined['key'] should blow up > } Ah, I see, right, we have to define activeApn as a dictionary here. I will fix this in https://codereview.chromium.org/547703004/, thanks!
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4ff8121cc52bb887c2a6d35c3eace08e11d841ca Cr-Commit-Position: refs/heads/master@{#292245}
Message was sent while issue was closed.
https://codereview.chromium.org/492383002/diff/120001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/492383002/diff/120001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:1377: var activeApn = getManagedValue(data, 'Cellular.APN'); while looking for the VPN hostname cause, I saw that this CL didn't update the internet_detail.html, which refers to 'providerApnList' in the controlled-setting-indicator. Similarly, I fear that several other controlled-setting-indicators were not updated.
Message was sent while issue was closed.
https://codereview.chromium.org/492383002/diff/120001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/492383002/diff/120001/chrome/browser/resource... chrome/browser/resources/options/chromeos/internet_detail.js:1377: var activeApn = getManagedValue(data, 'Cellular.APN'); On 2014/10/20 16:48:04, pneubeck wrote: > while looking for the VPN hostname cause, I saw that this CL didn't update the > internet_detail.html, which refers to 'providerApnList' in the > controlled-setting-indicator. > > Similarly, I fear that several other controlled-setting-indicators were not > updated. crbug.com/425164 |
