Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2306673002:
arc: update proxy settings from UI and ONC policy. (Closed)
|
Created:
4 years, 3 months ago by Polina Bondarenko Modified:
4 years, 2 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: update proxy settings from UI and ONC policy.
BUG=643239
TEST=manual(UI),autotest(extension API),browsertest(policy)
Committed: https://crrev.com/eaa283bc635773c2dc963297d3ee22023ecf3215
Cr-Commit-Position: refs/heads/master@{#418665}
Patch Set 1 : arc: update proxy settings from UI and ONC policy. #
Total comments: 14
Patch Set 2 : Fixed comments #Patch Set 3 : Removed a log line. #
Total comments: 20
Patch Set 4 : Fixed comments #
Total comments: 2
Patch Set 5 : const -> constexpr #Patch Set 6 : Rebased #
Total comments: 7
Patch Set 7 : Fixed comments. #
Total comments: 2
Patch Set 8 : Fixed a comment. #Messages
Total messages: 44 (27 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== arc: update proxy settings from UI and ONC policy. BUG=643239 ========== to ========== arc: update proxy settings from UI and ONC policy. BUG=643239 TEST=manual(UI),autotest(extension API),browsertest(policy) ==========
The CQ bit was checked by pbond@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by pbond@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pbond@chromium.org changed reviewers: + lhchavez@chromium.org, stevenjb@chromium.org
Hi! Please review this CL. ARC proxy settings are updated when default network proxy settings are changed now, not only when profile proxy setting changes (the old implementation didn't handle the UI proxy settings update and ONC policy with proxy settings). stevenjb@chromium.org: Please review changes in chrome/browser/chromeos/* files. lhchavez@chromium.org: Please review changes in */arc/* files. Thanks, Polina.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2306673002/diff/40001/components/arc/test/fak... File components/arc/test/fake_intent_helper_instance.cc (right): https://codereview.chromium.org/2306673002/diff/40001/components/arc/test/fak... components/arc/test/fake_intent_helper_instance.cc:23: mojom::ActionType action) { Can you run git cl format / git cl lint? https://codereview.chromium.org/2306673002/diff/40001/components/arc/test/fak... File components/arc/test/fake_intent_helper_instance.h (right): https://codereview.chromium.org/2306673002/diff/40001/components/arc/test/fak... components/arc/test/fake_intent_helper_instance.h:60: void SetCallback(const BroadcastCallback& callback) { Is it possible to instead have a normal std::vector<Broadcast> (where Broadcast is a struct that holds all parameters) that stores the broadcasts and provide a getter for it? You can then grab the items and clear it from your test instead of having to do the roundabout callback way. Also put whatever methods you are not overriding above L24 to clearly identify which methods are being provided by this class vs. overriding from a superclass.
https://codereview.chromium.org/2306673002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_settings_service.cc (right): https://codereview.chromium.org/2306673002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_settings_service.cc:67: profile->GetPrefs(), network->profile_path(), onc_source)) { Can we put this logic into proxy_config_service_impl.cc? It is similar to ProxyConfigServiceImpl::DetermineEffectiveConfigFromDefaultNetwork(). Combining them might be difficult so I don't think we need to do that, but I would like to keep the logic in the same place so that if we add it somewhere else we don't create a third copy of effectively the same logic. e.g. proxy_config = chromeos::proxy_config::GetDefaultProxyConfigDictionary(...); if (proxy_config) return proxy_config; Also, this way we don't need know the implementation details of NetworkState in this class, GetDefaultProxyConfigDictionary can grab the default network itself. https://codereview.chromium.org/2306673002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_settings_service.cc:70: } nit: I would invert this to make the positive results conditional and the default an empty config. https://codereview.chromium.org/2306673002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_settings_service.cc:245: } This could be: proxy_config = GetProxyConfig(); if (proxy_config) SendProxySettingsBroadcast(proxy_config.get()); Which would probably be better, since proxy_config might be based on the prefs, not the default network. https://codereview.chromium.org/2306673002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_settings_service.cc:500: SendProxySettingsBroadcast(proxy_config.get()); Do we want to cache the proxy config and only broadcast it if it changes? Also, we should observe changes to proxy_config::prefs::kProxy also. https://codereview.chromium.org/2306673002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc (right): https://codereview.chromium.org/2306673002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc:232: } We should test that changes to the default network or settings are broadcast.
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by pbond@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks a lot for reviewing! Fixed comments. Tried to avoid sending meaningless extras with proxy settings, but it seems the proxy config is sent regardless of which default network property has been changed. https://codereview.chromium.org/2306673002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_settings_service.cc (right): https://codereview.chromium.org/2306673002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_settings_service.cc:67: profile->GetPrefs(), network->profile_path(), onc_source)) { On 2016/09/02 16:20:55, stevenjb wrote: > Can we put this logic into proxy_config_service_impl.cc? It is similar to > ProxyConfigServiceImpl::DetermineEffectiveConfigFromDefaultNetwork(). Combining > them might be difficult so I don't think we need to do that, but I would like to > keep the logic in the same place so that if we add it somewhere else we don't > create a third copy of effectively the same logic. > > e.g. > proxy_config = chromeos::proxy_config::GetDefaultProxyConfigDictionary(...); > if (proxy_config) > return proxy_config; > > Also, this way we don't need know the implementation details of NetworkState in > this class, GetDefaultProxyConfigDictionary can grab the default network itself. Moved all logic to ProxyConfigServiceImpl and removed GetProxyConfig at all. https://codereview.chromium.org/2306673002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_settings_service.cc:70: } On 2016/09/02 16:20:55, stevenjb wrote: > nit: I would invert this to make the positive results conditional and the > default an empty config. Done. https://codereview.chromium.org/2306673002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_settings_service.cc:245: } On 2016/09/02 16:20:55, stevenjb wrote: > This could be: > proxy_config = GetProxyConfig(); > if (proxy_config) > SendProxySettingsBroadcast(proxy_config.get()); > > Which would probably be better, since proxy_config might be based on the prefs, > not the default network. Done. https://codereview.chromium.org/2306673002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_settings_service.cc:500: SendProxySettingsBroadcast(proxy_config.get()); On 2016/09/02 16:20:55, stevenjb wrote: > Do we want to cache the proxy config and only broadcast it if it changes? Yes, added check of the source to reduce the amount of unnecessary calls. > Also, we should observe changes to proxy_config::prefs::kProxy also. Fixed, +ONC policy changes. https://codereview.chromium.org/2306673002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc (right): https://codereview.chromium.org/2306673002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc:232: } On 2016/09/02 16:20:55, stevenjb wrote: > We should test that changes to the default network or settings are broadcast. Done, added tests. https://codereview.chromium.org/2306673002/diff/40001/components/arc/test/fak... File components/arc/test/fake_intent_helper_instance.cc (right): https://codereview.chromium.org/2306673002/diff/40001/components/arc/test/fak... components/arc/test/fake_intent_helper_instance.cc:23: mojom::ActionType action) { On 2016/09/02 15:58:48, Luis Héctor Chávez wrote: > Can you run git cl format / git cl lint? Done. https://codereview.chromium.org/2306673002/diff/40001/components/arc/test/fak... File components/arc/test/fake_intent_helper_instance.h (right): https://codereview.chromium.org/2306673002/diff/40001/components/arc/test/fak... components/arc/test/fake_intent_helper_instance.h:60: void SetCallback(const BroadcastCallback& callback) { On 2016/09/02 15:58:48, Luis Héctor Chávez wrote: > Is it possible to instead have a normal std::vector<Broadcast> (where Broadcast > is a struct that holds all parameters) that stores the broadcasts and provide a > getter for it? You can then grab the items and clear it from your test instead > of having to do the roundabout callback way. > > Also put whatever methods you are not overriding above L24 to clearly identify > which methods are being provided by this class vs. overriding from a superclass. Done.
This is looking better, thanks. Warning; I will be OOO the rest of the week, back on Monday. https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_settings_service.cc (right): https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service.cc:276: // Do not update proxy settings if kProxy pref is applied. Rephrase this: // Only update proxy settings if kProxy pref is applied. https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service.cc:478: &config))) { Since we call PrefPrecedes twice, ignoring config, maybe wrap this call into a helper function with a comment describing what we are checking (I am a little fuzzy on that). https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc (right): https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc:64: // Retruns an amount of |broadcasts| matched with |proxy_settings|. Returns https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc:250: IN_PROC_BROWSER_TEST_F(ArcSettingsServiceTest, DefaultProxyConfigTest) { DefaultNetworkProxyConfigTest https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc:276: 1); We still need to test that when the default network changes (e.g. by disconnecting |network|), the proxy config also changes. https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/proxy_config_service_impl.cc (right): https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/proxy_config_service_impl.cc:200: } This section doesn't depend on |network|. We should move it before the DefaultNetwork() call, unless we only want to return a proxy result if we are connected to a network in which case I would early exit if !network and add a comment. https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/proxy_config_service_impl.h (right): https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/proxy_config_service_impl.h:65: // profile. |profile_prefs| must be not NULL. s/a default network/the default network/ Also, 'for this network profile' is confusing. I would omit it, or say 'for the currently active network profile', but that is generally always the case. It is also unclear in the implementation whether we should return the pref proxy config if no network is connected. We should make that clear in this comment and maybe in the name, e.g. if we only return the config if we are connected to a network, maybe name this GetProxyConfigDictionaryForActiveNetwork().
https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc (right): https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc:46: // char kCmdProxyServer[] = "proxy:8888"; nit: don't check in commented code. https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc:83: ArcSettingsServiceTest() {} nit: can you use "= default" for both this and the destructor? https://codereview.chromium.org/2306673002/diff/100001/components/arc/test/fa... File components/arc/test/fake_intent_helper_instance.cc (right): https://codereview.chromium.org/2306673002/diff/100001/components/arc/test/fa... components/arc/test/fake_intent_helper_instance.cc:62: broadcasts_.push_back(Broadcast(action, package_name, cls, extras)); nit: can you use broadcasts_.emplace_back(action, package_name, cls, extras); ? Alternatively, you can make Broadcast a plain struct with no methods and use broadcasts_.push_back(Broadcast{action, package_name, cls, extras}); which is slightly cleaner overall.
The CQ bit was checked by pbond@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks a lot! Fixed comments and finally added working browser test for disconnecting default network. https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_settings_service.cc (right): https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service.cc:276: // Do not update proxy settings if kProxy pref is applied. On 2016/09/06 15:49:06, stevenjb wrote: > Rephrase this: // Only update proxy settings if kProxy pref is applied. Fixed to: "Only update proxy settings if kProxy pref is not applied." https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service.cc:478: &config))) { On 2016/09/06 15:49:06, stevenjb wrote: > Since we call PrefPrecedes twice, ignoring config, maybe wrap this call into a > helper function with a comment describing what we are checking (I am a little > fuzzy on that). Wrapped into a local function. https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc (right): https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc:46: // char kCmdProxyServer[] = "proxy:8888"; On 2016/09/06 16:16:25, Luis Héctor Chávez wrote: > nit: don't check in commented code. Done. https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc:64: // Retruns an amount of |broadcasts| matched with |proxy_settings|. On 2016/09/06 15:49:06, stevenjb wrote: > Returns Done. https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc:83: ArcSettingsServiceTest() {} On 2016/09/06 16:16:25, Luis Héctor Chávez wrote: > nit: can you use "= default" for both this and the destructor? Done. https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc:250: IN_PROC_BROWSER_TEST_F(ArcSettingsServiceTest, DefaultProxyConfigTest) { On 2016/09/06 15:49:06, stevenjb wrote: > DefaultNetworkProxyConfigTest Done. https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc:276: 1); On 2016/09/06 15:49:06, stevenjb wrote: > We still need to test that when the default network changes (e.g. by > disconnecting |network|), the proxy config also changes. Done. https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/proxy_config_service_impl.cc (right): https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/proxy_config_service_impl.cc:200: } On 2016/09/06 15:49:06, stevenjb wrote: > This section doesn't depend on |network|. We should move it before the > DefaultNetwork() call, unless we only want to return a proxy result if we are > connected to a network in which case I would early exit if !network and add a > comment. Done. https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/proxy_config_service_impl.h (right): https://codereview.chromium.org/2306673002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/proxy_config_service_impl.h:65: // profile. |profile_prefs| must be not NULL. On 2016/09/06 15:49:06, stevenjb wrote: > s/a default network/the default network/ > > Also, 'for this network profile' is confusing. I would omit it, or say 'for the > currently active network profile', but that is generally always the case. > > It is also unclear in the implementation whether we should return the pref proxy > config if no network is connected. We should make that clear in this comment and > maybe in the name, e.g. if we only return the config if we are connected to a > network, maybe name this GetProxyConfigDictionaryForActiveNetwork(). I'd return pref proxy config regardless the network is active. According to the current implementation, if there is no network and proxy pref changes, ARC will not be notified about the change at all, even after the network becomes active. https://codereview.chromium.org/2306673002/diff/100001/components/arc/test/fa... File components/arc/test/fake_intent_helper_instance.cc (right): https://codereview.chromium.org/2306673002/diff/100001/components/arc/test/fa... components/arc/test/fake_intent_helper_instance.cc:62: broadcasts_.push_back(Broadcast(action, package_name, cls, extras)); On 2016/09/06 16:16:25, Luis Héctor Chávez wrote: > nit: can you use > > broadcasts_.emplace_back(action, package_name, cls, extras); > > ? Alternatively, you can make Broadcast a plain struct with no methods and use > > broadcasts_.push_back(Broadcast{action, package_name, cls, extras}); > > which is slightly cleaner overall. Hm, Chrome codestyle checker does not allow me to put struct without constructor since values go from mojo::String to std::string. Just fixed to emplace_back.
lgtm https://codereview.chromium.org/2306673002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc (right): https://codereview.chromium.org/2306673002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc:47: const char kONCPolicy[] = sorry, I missed this last iteration. can you make all these constexpr?
https://codereview.chromium.org/2306673002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc (right): https://codereview.chromium.org/2306673002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service_browsertest.cc:47: const char kONCPolicy[] = On 2016/09/06 19:53:22, Luis Héctor Chávez wrote: > sorry, I missed this last iteration. can you make all these constexpr? Sure, thanks for catch!
The CQ bit was checked by pbond@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2306673002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_settings_service.cc (right): https://codereview.chromium.org/2306673002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service.cc:285: if (!IsPrefProxyConfigApplied()) This looks like it is missing a { ??? But it should also now be written as: if (IsPrefProxyConfigApplied()) { LOG(ERROR) ... return; } SyncProxySettings(); https://codereview.chromium.org/2306673002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/proxy_config_service_impl.h (right): https://codereview.chromium.org/2306673002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/proxy_config_service_impl.h:66: // NULL otherwise. This comment does not seem to be accurate. We "Apply Pref Proxy configuration if available" first. https://codereview.chromium.org/2306673002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/proxy_config_service_impl.h:68: static std::unique_ptr<ProxyConfigDictionary> GetDefaultProxyConfigDictionary( I think that with the current logic, "Default" is confusing. I would just name this GetProxyConfigDictionary, or maybe GetActiveProxyConfigDictionary.
Hi Steven, I fixed comments, PTAL https://codereview.chromium.org/2306673002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_settings_service.cc (right): https://codereview.chromium.org/2306673002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service.cc:285: if (!IsPrefProxyConfigApplied()) On 2016/09/12 19:59:25, stevenjb wrote: > This looks like it is missing a { ??? > > But it should also now be written as: > > if (IsPrefProxyConfigApplied()) { > LOG(ERROR) ... > return; > } > SyncProxySettings(); Fixed, but one line "if" doesn't require curly braces, does it? https://codereview.chromium.org/2306673002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/proxy_config_service_impl.h (right): https://codereview.chromium.org/2306673002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/proxy_config_service_impl.h:66: // NULL otherwise. On 2016/09/12 19:59:25, stevenjb wrote: > This comment does not seem to be accurate. We "Apply Pref Proxy configuration if > available" first. Fixed a little bit. This method returns Pref Proxy configuration if configured regardless a connected network. https://codereview.chromium.org/2306673002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/proxy_config_service_impl.h:68: static std::unique_ptr<ProxyConfigDictionary> GetDefaultProxyConfigDictionary( On 2016/09/12 19:59:25, stevenjb wrote: > I think that with the current logic, "Default" is confusing. I would just name > this GetProxyConfigDictionary, or maybe GetActiveProxyConfigDictionary. Fixed.
lgtm https://codereview.chromium.org/2306673002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_settings_service.cc (right): https://codereview.chromium.org/2306673002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_settings_service.cc:285: if (!IsPrefProxyConfigApplied()) On 2016/09/12 20:49:05, Polina Bondarenko wrote: > On 2016/09/12 19:59:25, stevenjb wrote: > > This looks like it is missing a { ??? > > > > But it should also now be written as: > > > > if (IsPrefProxyConfigApplied()) { > > LOG(ERROR) ... > > return; > > } > > SyncProxySettings(); > > Fixed, but one line "if" doesn't require curly braces, does it? It does if followed by } else { ... } (but better now regardless). https://codereview.chromium.org/2306673002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/proxy_config_service_impl.h (right): https://codereview.chromium.org/2306673002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/proxy_config_service_impl.h:66: // Returns NULL, if there is no active connected network. // Otherwise returns null. or // Returns null if no pref configuration and no active network.
Thanks a lot for reviewing! https://codereview.chromium.org/2306673002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/proxy_config_service_impl.h (right): https://codereview.chromium.org/2306673002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/proxy_config_service_impl.h:66: // Returns NULL, if there is no active connected network. On 2016/09/12 21:21:44, stevenjb wrote: > // Otherwise returns null. > > or > > // Returns null if no pref configuration and no active network. Done.
The CQ bit was checked by pbond@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pbond@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2306673002/#ps200001 (title: "Fixed a comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== arc: update proxy settings from UI and ONC policy. BUG=643239 TEST=manual(UI),autotest(extension API),browsertest(policy) ========== to ========== arc: update proxy settings from UI and ONC policy. BUG=643239 TEST=manual(UI),autotest(extension API),browsertest(policy) ==========
Message was sent while issue was closed.
Committed patchset #8 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== arc: update proxy settings from UI and ONC policy. BUG=643239 TEST=manual(UI),autotest(extension API),browsertest(policy) ========== to ========== arc: update proxy settings from UI and ONC policy. BUG=643239 TEST=manual(UI),autotest(extension API),browsertest(policy) Committed: https://crrev.com/eaa283bc635773c2dc963297d3ee22023ecf3215 Cr-Commit-Position: refs/heads/master@{#418665} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/eaa283bc635773c2dc963297d3ee22023ecf3215 Cr-Commit-Position: refs/heads/master@{#418665}
Message was sent while issue was closed.
On 2016/09/14 20:53:58, commit-bot: I haz the power wrote: > Patchset 8 (id:??) landed as > https://crrev.com/eaa283bc635773c2dc963297d3ee22023ecf3215 > Cr-Commit-Position: refs/heads/master@{#418665} I believe this CL may be causing a Chrome crash on resume, see: https://bugs.chromium.org/p/chromium/issues/detail?id=652177 |
