Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2471723002:
chromeos: networkingPrivate: Set active proxy values (Closed)
|
Created:
4 years, 1 month ago by stevenjb Modified:
4 years, 1 month ago CC:
chromium-reviews, extensions-reviews_chromium.org, michaelpg+watch-elements_chromium.org, dbeam+watch-elements_chromium.org, michaelpg+watch-md-settings_chromium.org, stevenjb+watch_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-settings_chromium.org, dbeam+watch-closure_chromium.org, stevenjb+watch-md-settings_chromium.org, jlklein+watch-closure_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionchromeos: networkingPrivate: Set active proxy values
This CL sets the 'Active' ONC property for the Proxy when the proxy is
being set by an extension.
It also introduces the 'ActiveExtension' value for the 'Effective'
field of managed ONC property dictionaries indicating that the Active
value has been set by an extension.
BUG=658015
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/7b704d273945c123d864d49838695f36e0151a47
Cr-Commit-Position: refs/heads/master@{#432361}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Separate out UI changes #
Total comments: 11
Patch Set 4 : Rebase + Feedback #
Total comments: 9
Patch Set 5 : Fix proxy configuration #Patch Set 6 : Rebase #Patch Set 7 : Add comments, rename function #Patch Set 8 : Fix state test #Patch Set 9 : Fix extension controlled #Patch Set 10 : Rebase #
Total comments: 7
Patch Set 11 : Rebase + WrapUnique -> MakeUnique #Patch Set 12 : Fix test expectations #Messages
Total messages: 38 (22 generated)
Description was changed from ========== chromeos: networkingPrivate: Set active proxy values This CL sets the 'Active' ONC property for the Proxy when the proxy is being set by an extension. It also introduces the 'ActiveExtension' value for the 'Effective' field of managed ONC property dictionaries indicating that the Active value has been set by an extension. The CL also updats cr_policy_network_behavior.js to recognize managed properties set to ActiveExtensiion. BUG=658015 ========== to ========== chromeos: networkingPrivate: Set active proxy values This CL sets the 'Active' ONC property for the Proxy when the proxy is being set by an extension. It also introduces the 'ActiveExtension' value for the 'Effective' field of managed ONC property dictionaries indicating that the Active value has been set by an extension. The CL also updats cr_policy_network_behavior.js to recognize managed properties set to ActiveExtensiion. BUG=658015 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== chromeos: networkingPrivate: Set active proxy values This CL sets the 'Active' ONC property for the Proxy when the proxy is being set by an extension. It also introduces the 'ActiveExtension' value for the 'Effective' field of managed ONC property dictionaries indicating that the Active value has been set by an extension. The CL also updats cr_policy_network_behavior.js to recognize managed properties set to ActiveExtensiion. BUG=658015 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== chromeos: networkingPrivate: Set active proxy values This CL sets the 'Active' ONC property for the Proxy when the proxy is being set by an extension. It also introduces the 'ActiveExtension' value for the 'Effective' field of managed ONC property dictionaries indicating that the Active value has been set by an extension. The CL also updats cr_policy_network_behavior.js to recognize managed properties set to ActiveExtensiion. BUG=658015 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== chromeos: networkingPrivate: Set active proxy values This CL sets the 'Active' ONC property for the Proxy when the proxy is being set by an extension. It also introduces the 'ActiveExtension' value for the 'Effective' field of managed ONC property dictionaries indicating that the Active value has been set by an extension. The CL also updats cr_policy_network_behavior.js to recognize managed properties set to ActiveExtensiion. BUG=658015 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== chromeos: networkingPrivate: Set active proxy values and show in Settings UI This CL sets the 'Active' ONC property for the Proxy when the proxy is being set by an extension. It also introduces the 'ActiveExtension' value for the 'Effective' field of managed ONC property dictionaries indicating that the Active value has been set by an extension. The CL also updats cr_policy_network_behavior.js to recognize managed properties set to ActiveExtensiion. BUG=658015 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== chromeos: networkingPrivate: Set active proxy values and show in Settings UI This CL sets the 'Active' ONC property for the Proxy when the proxy is being set by an extension. It also introduces the 'ActiveExtension' value for the 'Effective' field of managed ONC property dictionaries indicating that the Active value has been set by an extension. The CL also updats cr_policy_network_behavior.js to recognize managed properties set to ActiveExtensiion. BUG=658015 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== chromeos: networkingPrivate: Set active proxy values and show in Settings UI This CL sets the 'Active' ONC property for the Proxy when the proxy is being set by an extension. It also introduces the 'ActiveExtension' value for the 'Effective' field of managed ONC property dictionaries indicating that the Active value has been set by an extension. The CL also updats cr_policy_network_behavior.js to recognize managed properties set to ActiveExtensiion. BUG=658015 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== chromeos: networkingPrivate: Set active proxy values and show in Settings UI This CL sets the 'Active' ONC property for the Proxy when the proxy is being set by an extension. It also introduces the 'ActiveExtension' value for the 'Effective' field of managed ONC property dictionaries indicating that the Active value has been set by an extension. The CL also updats cr_policy_network_behavior.js to recognize managed properties set to ActiveExtensiion. BUG=658015 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== chromeos: networkingPrivate: Set active proxy values and show in Settings UI This CL sets the 'Active' ONC property for the Proxy when the proxy is being set by an extension. It also introduces the 'ActiveExtension' value for the 'Effective' field of managed ONC property dictionaries indicating that the Active value has been set by an extension. The CL also updatspolicy behaviors to recognize managed properties set to ActiveExtensiion. BUG=658015 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== chromeos: networkingPrivate: Set active proxy values and show in Settings UI This CL sets the 'Active' ONC property for the Proxy when the proxy is being set by an extension. It also introduces the 'ActiveExtension' value for the 'Effective' field of managed ONC property dictionaries indicating that the Active value has been set by an extension. The CL also updatspolicy behaviors to recognize managed properties set to ActiveExtensiion. BUG=658015 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== chromeos: networkingPrivate: Set active proxy values and show in Settings UI This CL sets the 'Active' ONC property for the Proxy when the proxy is being set by an extension. It also introduces the 'ActiveExtension' value for the 'Effective' field of managed ONC property dictionaries indicating that the Active value has been set by an extension. BUG=658015 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #3 (id:40001) has been deleted
stevenjb@chromium.org changed reviewers: + michaelpg@chromium.org
stevenjb@chromium.org changed reviewers: + emaxx@chromium.org
+emaxx@, largely FYI but feedback welcome.
nits https://codereview.chromium.org/2471723002/diff/60001/chromeos/network/networ... File chromeos/network/network_state_handler.h (right): https://codereview.chromium.org/2471723002/diff/60001/chromeos/network/networ... chromeos/network/network_state_handler.h:218: // Inform NetworkStateHandler to notify observers that the properties for nit: Informs https://codereview.chromium.org/2471723002/diff/60001/extensions/browser/api/... File extensions/browser/api/networking_private/networking_private_chromeos.cc (right): https://codereview.chromium.org/2471723002/diff/60001/extensions/browser/api/... extensions/browser/api/networking_private/networking_private_chromeos.cc:222: base::DictionaryValue* EnsureDictionaryValue(const std::string& key, comment, please: ensure dictionary is what? set_active what? https://codereview.chromium.org/2471723002/diff/60001/extensions/browser/api/... extensions/browser/api/networking_private/networking_private_chromeos.cc:723: // Ensure that the ProxySettings dictionary exists. nit: line break above https://codereview.chromium.org/2471723002/diff/60001/extensions/browser/api/... extensions/browser/api/networking_private/networking_private_chromeos.cc:732: if (config.mode == UIProxyConfig::MODE_SINGLE_PROXY) { nit: line break above https://codereview.chromium.org/2471723002/diff/60001/extensions/browser/api/... extensions/browser/api/networking_private/networking_private_chromeos.cc:732: if (config.mode == UIProxyConfig::MODE_SINGLE_PROXY) { nit: may be more readable as a switch. I don't normally care but this code is very dense-looking. https://codereview.chromium.org/2471723002/diff/60001/extensions/browser/api/... File extensions/browser/api/networking_private/networking_private_chromeos.h (right): https://codereview.chromium.org/2471723002/diff/60001/extensions/browser/api/... extensions/browser/api/networking_private/networking_private_chromeos.h:114: // Set the active proxy values for connected networks. nit: "Sets"
Description was changed from ========== chromeos: networkingPrivate: Set active proxy values and show in Settings UI This CL sets the 'Active' ONC property for the Proxy when the proxy is being set by an extension. It also introduces the 'ActiveExtension' value for the 'Effective' field of managed ONC property dictionaries indicating that the Active value has been set by an extension. BUG=658015 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== chromeos: networkingPrivate: Set active proxy values This CL sets the 'Active' ONC property for the Proxy when the proxy is being set by an extension. It also introduces the 'ActiveExtension' value for the 'Effective' field of managed ONC property dictionaries indicating that the Active value has been set by an extension. BUG=658015 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
PTAL https://codereview.chromium.org/2471723002/diff/60001/chromeos/network/networ... File chromeos/network/network_state_handler.h (right): https://codereview.chromium.org/2471723002/diff/60001/chromeos/network/networ... chromeos/network/network_state_handler.h:218: // Inform NetworkStateHandler to notify observers that the properties for On 2016/11/02 23:55:25, michaelpg wrote: > nit: Informs Done. https://codereview.chromium.org/2471723002/diff/60001/extensions/browser/api/... File extensions/browser/api/networking_private/networking_private_chromeos.cc (right): https://codereview.chromium.org/2471723002/diff/60001/extensions/browser/api/... extensions/browser/api/networking_private/networking_private_chromeos.cc:222: base::DictionaryValue* EnsureDictionaryValue(const std::string& key, On 2016/11/02 23:55:25, michaelpg wrote: > comment, please: ensure dictionary is what? set_active what? Added comment and split up function. https://codereview.chromium.org/2471723002/diff/60001/extensions/browser/api/... extensions/browser/api/networking_private/networking_private_chromeos.cc:723: // Ensure that the ProxySettings dictionary exists. On 2016/11/02 23:55:25, michaelpg wrote: > nit: line break above Done. https://codereview.chromium.org/2471723002/diff/60001/extensions/browser/api/... extensions/browser/api/networking_private/networking_private_chromeos.cc:732: if (config.mode == UIProxyConfig::MODE_SINGLE_PROXY) { On 2016/11/02 23:55:25, michaelpg wrote: > nit: line break above There are a couple of modes we don't care about so I will stick to an if, but added a line break and comment. https://codereview.chromium.org/2471723002/diff/60001/extensions/browser/api/... File extensions/browser/api/networking_private/networking_private_chromeos.h (right): https://codereview.chromium.org/2471723002/diff/60001/extensions/browser/api/... extensions/browser/api/networking_private/networking_private_chromeos.h:114: // Set the active proxy values for connected networks. On 2016/11/02 23:55:25, michaelpg wrote: > nit: "Sets" Done.
The CQ bit was checked by stevenjb@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.
lgtm (again, according to my limited context with this piece) https://codereview.chromium.org/2471723002/diff/80001/extensions/browser/api/... File extensions/browser/api/networking_private/networking_private_chromeos.cc (right): https://codereview.chromium.org/2471723002/diff/80001/extensions/browser/api/... extensions/browser/api/networking_private/networking_private_chromeos.cc:746: manual->RemoveWithoutPathExpansion(::onc::proxy::kHttps, nullptr); I'm lacking the context here, but maybe some explanation would be helpful regarding the reason why these values are removed in this case and why this is not necessary in other cases.
https://codereview.chromium.org/2471723002/diff/80001/extensions/browser/api/... File extensions/browser/api/networking_private/networking_private_chromeos.cc (right): https://codereview.chromium.org/2471723002/diff/80001/extensions/browser/api/... extensions/browser/api/networking_private/networking_private_chromeos.cc:260: const std::string& key, unused? https://codereview.chromium.org/2471723002/diff/80001/extensions/browser/api/... extensions/browser/api/networking_private/networking_private_chromeos.cc:265: manual->SetIntegerWithoutPathExpansion(::onc::proxy::kHost, kHost is being set twice... https://codereview.chromium.org/2471723002/diff/80001/extensions/browser/api/... extensions/browser/api/networking_private/networking_private_chromeos.cc:729: // Ensure that the ProxySettings dictionary exists. The rest of the function assumes kProxySettings is a ManagedProxySettings dict (not a ProxySettings dict). Is that because CONFIG_EXTENSION means that |dictionary| is a ManagedProperties? There's an odd dependency on UIProxyConfig I wouldn't have expected; can you add a comment to the effect that we know we're dealing with managed properties? https://codereview.chromium.org/2471723002/diff/80001/extensions/browser/api/... extensions/browser/api/networking_private/networking_private_chromeos.cc:746: manual->RemoveWithoutPathExpansion(::onc::proxy::kHttps, nullptr); On 2016/11/03 21:15:44, emaxx wrote: > I'm lacking the context here, but maybe some explanation would be helpful > regarding the reason why these values are removed in this case and why this is > not necessary in other cases. +1; SetManualProxy doesn't seem like it's doing whatever it's supposed to be doing. Also not sure why a single proxy has to be http or if http is the default; don't see that documented anywhere.
Apologies, there were several issues with the previous iteration. It may be easiest just to re-review this. https://codereview.chromium.org/2471723002/diff/80001/extensions/browser/api/... File extensions/browser/api/networking_private/networking_private_chromeos.cc (right): https://codereview.chromium.org/2471723002/diff/80001/extensions/browser/api/... extensions/browser/api/networking_private/networking_private_chromeos.cc:260: const std::string& key, On 2016/11/04 00:58:28, michaelpg wrote: > unused? Done. https://codereview.chromium.org/2471723002/diff/80001/extensions/browser/api/... extensions/browser/api/networking_private/networking_private_chromeos.cc:265: manual->SetIntegerWithoutPathExpansion(::onc::proxy::kHost, On 2016/11/04 00:58:28, michaelpg wrote: > kHost is being set twice... Done. https://codereview.chromium.org/2471723002/diff/80001/extensions/browser/api/... extensions/browser/api/networking_private/networking_private_chromeos.cc:729: // Ensure that the ProxySettings dictionary exists. On 2016/11/04 00:58:28, michaelpg wrote: > The rest of the function assumes kProxySettings is a ManagedProxySettings dict > (not a ProxySettings dict). Is that because CONFIG_EXTENSION means that > |dictionary| is a ManagedProperties? There's an odd dependency on UIProxyConfig > I wouldn't have expected; can you add a comment to the effect that we know we're > dealing with managed properties? This is only called for managed dictionaries (managed is true in GetPropertiesCallback). I renamed this and updated the comment in the header. I also commented about the fubar nature of using UIProxyConfig. https://codereview.chromium.org/2471723002/diff/80001/extensions/browser/api/... extensions/browser/api/networking_private/networking_private_chromeos.cc:746: manual->RemoveWithoutPathExpansion(::onc::proxy::kHttps, nullptr); On 2016/11/04 00:58:28, michaelpg wrote: > On 2016/11/03 21:15:44, emaxx wrote: > > I'm lacking the context here, but maybe some explanation would be helpful > > regarding the reason why these values are removed in this case and why this is > > not necessary in other cases. > > +1; SetManualProxy doesn't seem like it's doing whatever it's supposed to be > doing. Also not sure why a single proxy has to be http or if http is the > default; don't see that documented anywhere. Short version: We only care about kHttp when we indicate "use the same proxy for all protocols". However, I have since determined that it makes more sense just to set all properties to the same value (which is what Shill does). I also added a comment.
Patchset #10 (id:200001) has been deleted
https://codereview.chromium.org/2471723002/diff/220001/extensions/browser/api... File extensions/browser/api/networking_private/networking_private_chromeos.cc (right): https://codereview.chromium.org/2471723002/diff/220001/extensions/browser/api... extensions/browser/api/networking_private/networking_private_chromeos.cc:284: base::WrapUnique<base::Value>(new base::StringValue( can WrapUnique become MakeUnique without the `new`? (as suggested by ptr_util.h:43) or something like base::MakeUnique<base::StringValue>(proxy.server.blah) might work to prevent copying? https://codereview.chromium.org/2471723002/diff/220001/extensions/browser/api... extensions/browser/api/networking_private/networking_private_chromeos.cc:292: new base::FundamentalValue(static_cast<int>(port)))); same question https://codereview.chromium.org/2471723002/diff/220001/extensions/browser/api... extensions/browser/api/networking_private/networking_private_chromeos.cc:292: new base::FundamentalValue(static_cast<int>(port)))); dumb noob question: is a static cast necessary? https://codereview.chromium.org/2471723002/diff/220001/extensions/browser/api... extensions/browser/api/networking_private/networking_private_chromeos.cc:755: VLOG(1) << "CONFIG STATE FOR: " << guid << ": " << state intentionally left in?
PTAL https://codereview.chromium.org/2471723002/diff/220001/extensions/browser/api... File extensions/browser/api/networking_private/networking_private_chromeos.cc (right): https://codereview.chromium.org/2471723002/diff/220001/extensions/browser/api... extensions/browser/api/networking_private/networking_private_chromeos.cc:284: base::WrapUnique<base::Value>(new base::StringValue( On 2016/11/14 21:19:23, michaelpg wrote: > can WrapUnique become MakeUnique without the `new`? (as suggested by > ptr_util.h:43) > > or something like > base::MakeUnique<base::StringValue>(proxy.server.blah) > might work to prevent copying? MakeUnique<Value>(string) doesn't work because Value doesn't have a (string) constructor (or any public constructors for that matter). MakeUnique<StringValue>(string) does work. I didn't think we supported casting unique_ptr, but maybe that was a scoped_ptr limitation (or maybe I am misremembering). Done. https://codereview.chromium.org/2471723002/diff/220001/extensions/browser/api... extensions/browser/api/networking_private/networking_private_chromeos.cc:292: new base::FundamentalValue(static_cast<int>(port)))); On 2016/11/14 21:19:23, michaelpg wrote: > dumb noob question: is a static cast necessary? No, I think it's OK, since u16 -> int isn't lossy, just habit. Done. https://codereview.chromium.org/2471723002/diff/220001/extensions/browser/api... extensions/browser/api/networking_private/networking_private_chromeos.cc:755: VLOG(1) << "CONFIG STATE FOR: " << guid << ": " << state On 2016/11/14 21:19:23, michaelpg wrote: > intentionally left in? Yeah. I'm not convinced I'm done debugging this.
lgtm
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emaxx@chromium.org Link to the patchset: https://codereview.chromium.org/2471723002/#ps240001 (title: "Rebase + WrapUnique -> MakeUnique")
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
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org, emaxx@chromium.org Link to the patchset: https://codereview.chromium.org/2471723002/#ps260001 (title: "Fix test expectations")
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 ========== chromeos: networkingPrivate: Set active proxy values This CL sets the 'Active' ONC property for the Proxy when the proxy is being set by an extension. It also introduces the 'ActiveExtension' value for the 'Effective' field of managed ONC property dictionaries indicating that the Active value has been set by an extension. BUG=658015 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== chromeos: networkingPrivate: Set active proxy values This CL sets the 'Active' ONC property for the Proxy when the proxy is being set by an extension. It also introduces the 'ActiveExtension' value for the 'Effective' field of managed ONC property dictionaries indicating that the Active value has been set by an extension. BUG=658015 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #12 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== chromeos: networkingPrivate: Set active proxy values This CL sets the 'Active' ONC property for the Proxy when the proxy is being set by an extension. It also introduces the 'ActiveExtension' value for the 'Effective' field of managed ONC property dictionaries indicating that the Active value has been set by an extension. BUG=658015 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== chromeos: networkingPrivate: Set active proxy values This CL sets the 'Active' ONC property for the Proxy when the proxy is being set by an extension. It also introduces the 'ActiveExtension' value for the 'Effective' field of managed ONC property dictionaries indicating that the Active value has been set by an extension. BUG=658015 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/7b704d273945c123d864d49838695f36e0151a47 Cr-Commit-Position: refs/heads/master@{#432361} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/7b704d273945c123d864d49838695f36e0151a47 Cr-Commit-Position: refs/heads/master@{#432361} |
