Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1925083003:
Notify ARC of default network changes (Closed)
|
Created:
4 years, 7 months ago by Kevin Cernekee Modified:
4 years, 7 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, cbentzel+watch_chromium.org, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@wifi-change Project:
chromium Visibility:
Public. |
DescriptionNotify ARC of default network changes
Track the default logical service (i.e. VPN or physical interface) and
notify ARC when it changes. Allow ARC to query the default service
at startup time.
This reports IP information, MAC address, wifi parameters, etc.
BUG=b/26495433
Committed: https://crrev.com/69d6c93bea89bafef588b470c87f240577805f5c
Cr-Commit-Position: refs/heads/master@{#394836}
Patch Set 1 #Patch Set 2 : add version check, fix API call, add missing assignment #Patch Set 3 : formatting/lint fixes #Patch Set 4 : rebase on ToT #Patch Set 5 : fix merge conflicts #Patch Set 6 : upload latest changes #Patch Set 7 : Make a bunch of fields nullable #Patch Set 8 : fix a couple of trybot failures #Patch Set 9 : fix more cargo-cult invocations of std::move #Patch Set 10 : cache default_network_ changes even if ARC isn't connected yet #
Total comments: 4
Patch Set 11 : incorporate code review feedback #
Total comments: 33
Patch Set 12 : incorporate code review feedback #
Total comments: 10
Patch Set 13 : incorporate code review feedback #Patch Set 14 : fix linter error #
Total comments: 7
Patch Set 15 : move helpers to anonymous namespace #
Total comments: 16
Patch Set 16 : rename a bunch of temporary variables #
Total comments: 15
Patch Set 17 : incorporate code review feedback #
Total comments: 2
Patch Set 18 : forward-declare base::DictionaryValue #
Messages
Total messages: 37 (7 generated)
cernekee@chromium.org changed reviewers: + stevenjb@chromium.org
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
https://codereview.chromium.org/1925083003/diff/180001/components/arc/common/... File components/arc/common/net.mojom (right): https://codereview.chromium.org/1925083003/diff/180001/components/arc/common/... components/arc/common/net.mojom:66: struct WiFi { WifiInfo? WifiConfiguration? https://codereview.chromium.org/1925083003/diff/180001/components/arc/common/... components/arc/common/net.mojom:90: WiFi? wifi; This looks like it will be null or not depending on |type|. Why not remove |type| and make this a union instead of a nullable? Maybe also move mac address and ip_configs to WifiInfo.
It is intended to mirror the ONC struct: https://chromium.googlesource.com/chromium/chromium/+/0142427081581becff601f4...
On 2016/05/14 20:15:59, Kevin Cernekee wrote: > It is intended to mirror the ONC struct: > > https://chromium.googlesource.com/chromium/chromium/+/0142427081581becff601f4... fair enough. please mention that in a comment.
Done - added the comment referencing ONC. https://codereview.chromium.org/1925083003/diff/180001/components/arc/common/... File components/arc/common/net.mojom (right): https://codereview.chromium.org/1925083003/diff/180001/components/arc/common/... components/arc/common/net.mojom:66: struct WiFi { On 2016/05/14 20:07:29, Luis Héctor Chávez wrote: > WifiInfo? WifiConfiguration? Acknowledged. https://codereview.chromium.org/1925083003/diff/180001/components/arc/common/... components/arc/common/net.mojom:90: WiFi? wifi; On 2016/05/14 20:07:29, Luis Héctor Chávez wrote: > This looks like it will be null or not depending on |type|. Why not remove > |type| and make this a union instead of a nullable? > > Maybe also move mac address and ip_configs to WifiInfo. Acknowledged.
cernekee@chromium.org changed reviewers: + abhishekbh@google.com
On 2016/05/15 21:02:48, Kevin Cernekee wrote: lgtm
https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:378: NOTREACHED(); Frequency is not a required ONC property, we should provide a reasonable default. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:381: NOTREACHED(); Ditto. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:389: mojo->hex_ssid = tmp; This pattern is used a lot. I think the code in this file would be a lot more readable as: mojo->hex_ssid = GetStringFromOncDictionary(dict, onc::wifi::kHexSSID, true /* required */); https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:392: NOTREACHED(); Optional, defaults to false. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:407: NOTREACHED(); nit: Move this conversion to a helper function. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:410: NOTREACHED(); Optional, defaults to 0 https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:430: } This seems like a generally useful utility function? https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:460: NOTREACHED(); This should be written: if (!...GetList...) NOTREACHED(); mojo->name_servers = ... https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:464: NOTREACHED(); {} https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:475: NOTREACHED(); More simply: mojo->type = tmp == onc::ipconfig::kIPv6 ? mojom::IPAddressType::IPV6 : mojom::IPAddressType::IPV4; https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:503: NOTREACHED(); nit: helper function https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:553: // default logical service in both fields. string type; if (dictionary->GetString(kType, &type) && type == VPN) { physical_network = GetNetworkStateHandler()->ConnectedNetworkByType(NetworkTypePattern::NonVirtual()); if (physical_network) { GetManagedConfigurationHandler()->GetProperties( user_id_hash, physical_network->service_path(), base::Bind(&GetDefaultNetworkSuccessCallback, callback), base::Bind(&GetDefaultNetworkFailureCallback, callback)); return; } } https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:604: if (default_network_.empty()) { Not possible. (You can add a DCHECK to document that). https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:611: base::Bind(&DefaultNetworkSuccessCallback, base::Unretained(this)), You should just bind default_network here instead of setting it as a member. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.h (right): https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.h:78: std::string default_network_; Why store this?
https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:378: NOTREACHED(); On 2016/05/16 19:29:35, stevenjb wrote: > Frequency is not a required ONC property, we should provide a reasonable > default. Done. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:381: NOTREACHED(); On 2016/05/16 19:29:35, stevenjb wrote: > Ditto. Done. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:389: mojo->hex_ssid = tmp; On 2016/05/16 19:29:35, stevenjb wrote: > This pattern is used a lot. I think the code in this file would be a lot more > readable as: > mojo->hex_ssid = GetStringFromOncDictionary(dict, onc::wifi::kHexSSID, true /* > required */); Done. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:392: NOTREACHED(); On 2016/05/16 19:29:35, stevenjb wrote: > Optional, defaults to false. Done. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:407: NOTREACHED(); On 2016/05/16 19:29:35, stevenjb wrote: > nit: Move this conversion to a helper function. Done. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:410: NOTREACHED(); On 2016/05/16 19:29:35, stevenjb wrote: > Optional, defaults to 0 Done. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:430: } On 2016/05/16 19:29:35, stevenjb wrote: > This seems like a generally useful utility function? Acknowledged. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:460: NOTREACHED(); On 2016/05/16 19:29:35, stevenjb wrote: > This should be written: > if (!...GetList...) > NOTREACHED(); > mojo->name_servers = ... Done. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:464: NOTREACHED(); On 2016/05/16 19:29:35, stevenjb wrote: > {} Done. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:475: NOTREACHED(); On 2016/05/16 19:29:35, stevenjb wrote: > More simply: > mojo->type = tmp == onc::ipconfig::kIPv6 ? mojom::IPAddressType::IPV6 : > mojom::IPAddressType::IPV4; Done. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:503: NOTREACHED(); On 2016/05/16 19:29:35, stevenjb wrote: > nit: helper function Done. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:553: // default logical service in both fields. On 2016/05/16 19:29:35, stevenjb wrote: > string type; > if (dictionary->GetString(kType, &type) && type == VPN) { > physical_network = > GetNetworkStateHandler()->ConnectedNetworkByType(NetworkTypePattern::NonVirtual()); > if (physical_network) { > GetManagedConfigurationHandler()->GetProperties( > user_id_hash, physical_network->service_path(), > base::Bind(&GetDefaultNetworkSuccessCallback, callback), > base::Bind(&GetDefaultNetworkFailureCallback, callback)); > return; > } > } > Ack, will handle this in a separate CL. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:604: if (default_network_.empty()) { On 2016/05/16 19:29:35, stevenjb wrote: > Not possible. (You can add a DCHECK to document that). We hit this case when network is NULL (i.e. all interfaces are down/disabled). https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:611: base::Bind(&DefaultNetworkSuccessCallback, base::Unretained(this)), On 2016/05/16 19:29:35, stevenjb wrote: > You should just bind default_network here instead of setting it as a member. We set it as a member because we usually get a DefaultNetworkChanged event before ArcBridgeService comes up. In that case, ARC needs to explicitly query the default network through the GetDefaultNetwork mojo call, so it is convenient to have it cached. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.h (right): https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.h:78: std::string default_network_; On 2016/05/16 19:29:35, stevenjb wrote: > Why store this? c&p from the related comment on arc_net_host_impl.cc: We set it as a member because we usually get a DefaultNetworkChanged event before ArcBridgeService comes up. In that case, ARC needs to explicitly query the default network through the GetDefaultNetwork mojo call, so it is convenient to have it cached.
lgtm
https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:604: if (default_network_.empty()) { On 2016/05/17 01:56:26, Kevin Cernekee wrote: > On 2016/05/16 19:29:35, stevenjb wrote: > > Not possible. (You can add a DCHECK to document that). > > We hit this case when network is NULL (i.e. all interfaces are down/disabled). I see. I would restructure this function: if (!network) { VLOG... arc_bridge_service()->net_instance()->DefaultNetworkChanged(nullptr, nullptr); return; } https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:611: base::Bind(&DefaultNetworkSuccessCallback, base::Unretained(this)), On 2016/05/17 01:56:26, Kevin Cernekee wrote: > On 2016/05/16 19:29:35, stevenjb wrote: > > You should just bind default_network here instead of setting it as a member. > > We set it as a member because we usually get a DefaultNetworkChanged event > before ArcBridgeService comes up. In that case, ARC needs to explicitly query > the default network through the GetDefaultNetwork mojo call, so it is convenient > to have it cached. As noted: I think it is better to let NetworkStateHandler to track the default network and use GetNetworkStateHandler()->DefaultNetwork() instead. https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.h (right): https://codereview.chromium.org/1925083003/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.h:78: std::string default_network_; On 2016/05/17 01:56:27, Kevin Cernekee wrote: > On 2016/05/16 19:29:35, stevenjb wrote: > > Why store this? > > c&p from the related comment on arc_net_host_impl.cc: > > We set it as a member because we usually get a DefaultNetworkChanged event > before ArcBridgeService comes up. In that case, ARC needs to explicitly query > the default network through the GetDefaultNetwork mojo call, so it is convenient > to have it cached. I think it would be better just to call NetworkStateHandler::DefaultNetwork() in that case. https://codereview.chromium.org/1925083003/diff/220001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1925083003/diff/220001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:67: } This can be simpligied: std:string tmp; dict->GetString(key, &tmp); if (required && tmp.empty()) NOTREACHED(); return tmp; https://codereview.chromium.org/1925083003/diff/220001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:69: } All file local (i.e. non public) helper functions should be inside an anonymous namespace. https://codereview.chromium.org/1925083003/diff/220001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:432: NOTREACHED(); nit: NOTREACHED is redundant with empty() check. https://codereview.chromium.org/1925083003/diff/220001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:535: } nit: Use a helper function https://codereview.chromium.org/1925083003/diff/220001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:561: std::string user_id_hash = chromeos::LoginState::Get()->primary_user_hash(); This does extra work and logs a misleading error if default_network_ is null. Instead: default_network = GetNetworkStateHandler()->DefaultNetwork(); if (!default_network) { VLOG(1) << "GetDefaultNetwork: no default network"; callback.Run(nullptr, nullptr); return; }
cernekee@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/1925083003/diff/220001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1925083003/diff/220001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:67: } On 2016/05/17 18:02:41, stevenjb wrote: > This can be simpligied: > > std:string tmp; > dict->GetString(key, &tmp); > if (required && tmp.empty()) > NOTREACHED(); > return tmp; Done. https://codereview.chromium.org/1925083003/diff/220001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:69: } On 2016/05/17 18:02:41, stevenjb wrote: > All file local (i.e. non public) helper functions should be inside an anonymous > namespace. Done. https://codereview.chromium.org/1925083003/diff/220001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:432: NOTREACHED(); On 2016/05/17 18:02:41, stevenjb wrote: > nit: NOTREACHED is redundant with empty() check. Done. https://codereview.chromium.org/1925083003/diff/220001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:535: } On 2016/05/17 18:02:41, stevenjb wrote: > nit: Use a helper function Done. https://codereview.chromium.org/1925083003/diff/220001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:561: std::string user_id_hash = chromeos::LoginState::Get()->primary_user_hash(); On 2016/05/17 18:02:41, stevenjb wrote: > This does extra work and logs a misleading error if default_network_ is null. > Instead: > > default_network = GetNetworkStateHandler()->DefaultNetwork(); > if (!default_network) { > VLOG(1) << "GetDefaultNetwork: no default network"; > callback.Run(nullptr, nullptr); > return; > } Done.
https://codereview.chromium.org/1925083003/diff/260001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1925083003/diff/260001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:384: const base::DictionaryValue* dict) { I've expressed this in past reviews (https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc...), but I find this pretty unreadable. Can we encapsulate the ONC state into a class and provide a ToValue() for things that want the base::Value? This avoids the whole "serialize everything into a base::Value and then parse it back out" complexity.
https://codereview.chromium.org/1925083003/diff/260001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1925083003/diff/260001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:384: const base::DictionaryValue* dict) { On 2016/05/18 05:32:45, dcheng wrote: > I've expressed this in past reviews > (https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc...), > but I find this pretty unreadable. Can we encapsulate the ONC state into a class > and provide a ToValue() for things that want the base::Value? This avoids the > whole "serialize everything into a base::Value and then parse it back out" > complexity. That would be a huge amount of work and, imho, a step backwards. What would be much more useful would be an ONC .mojom file and a data driven ONC base::DictionaryValue -> mojo library. Then the translation from ONC -> Arc could happen in the arc layer instead of in Chrome (which would just pass complete ONC dictionaries to Arc via mojo). If I was not completely swamped with other responsibilities I would try to drive that effort, but unfortunately my focus is elsewhere.
On 2016/05/18 at 15:35:48, stevenjb wrote: > https://codereview.chromium.org/1925083003/diff/260001/components/arc/net/arc... > File components/arc/net/arc_net_host_impl.cc (right): > > https://codereview.chromium.org/1925083003/diff/260001/components/arc/net/arc... > components/arc/net/arc_net_host_impl.cc:384: const base::DictionaryValue* dict) { > On 2016/05/18 05:32:45, dcheng wrote: > > I've expressed this in past reviews > > (https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc...), > > but I find this pretty unreadable. Can we encapsulate the ONC state into a class > > and provide a ToValue() for things that want the base::Value? This avoids the > > whole "serialize everything into a base::Value and then parse it back out" > > complexity. > > That would be a huge amount of work and, imho, a step backwards. > > What would be much more useful would be an ONC .mojom file and a data driven ONC base::DictionaryValue -> mojo library. Then the translation from ONC -> Arc could happen in the arc layer instead of in Chrome (which would just pass complete ONC dictionaries to Arc via mojo). > > If I was not completely swamped with other responsibilities I would try to drive that effort, but unfortunately my focus is elsewhere. Why would it be a huge step backwards? In C++, we should be representing the data with strong types rather than storing it as a collection of loosely typed key-values and then trying to coerce it to the right value later. It leads to way more code as well. A ToValue() function is trivial to write, and the resulting base::Value is trivial to pass around.
On 2016/05/18 17:32:41, dcheng wrote: > On 2016/05/18 at 15:35:48, stevenjb wrote: > > > https://codereview.chromium.org/1925083003/diff/260001/components/arc/net/arc... > > File components/arc/net/arc_net_host_impl.cc (right): > > > > > https://codereview.chromium.org/1925083003/diff/260001/components/arc/net/arc... > > components/arc/net/arc_net_host_impl.cc:384: const base::DictionaryValue* > dict) { > > On 2016/05/18 05:32:45, dcheng wrote: > > > I've expressed this in past reviews > > > > (https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc...), > > > but I find this pretty unreadable. Can we encapsulate the ONC state into a > class > > > and provide a ToValue() for things that want the base::Value? This avoids > the > > > whole "serialize everything into a base::Value and then parse it back out" > > > complexity. > > > > That would be a huge amount of work and, imho, a step backwards. > > > > What would be much more useful would be an ONC .mojom file and a data driven > ONC base::DictionaryValue -> mojo library. Then the translation from ONC -> Arc > could happen in the arc layer instead of in Chrome (which would just pass > complete ONC dictionaries to Arc via mojo). > > > > If I was not completely swamped with other responsibilities I would try to > drive that effort, but unfortunately my focus is elsewhere. > > Why would it be a huge step backwards? In C++, we should be representing the > data with strong types rather than storing it as a collection of loosely typed > key-values and then trying to coerce it to the right value later. It leads to > way more code as well. A ToValue() function is trivial to write, and the > resulting base::Value is trivial to pass around. Clearly we have different perspectives on this. As I mentioned, unfortunately I do not have the bandwidth to be heavily involved in the design. The only short answer I can provide is that with a complex spec like ONC, maintaining a C++ class to represent it can be labor intensive, and does not help with other languages like JS which we are now using primarily for the Chrome UI. Mojo with its autogenerated code is a much more expressive way to do this, and can be used in multiple languages. If we were developing ONC today we would almost certainly have defined it in mojo.
Hi guys, can we go ahead and push this CL as-is? It is blocking a couple of important new features. Thanks!
Sorry, forgot to publish my last round of feedback. Just nits, so lgtm. https://codereview.chromium.org/1925083003/diff/260001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1925083003/diff/260001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:419: } All non member / non public functions should be in the anonymous namespace. https://codereview.chromium.org/1925083003/diff/260001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:511: } I think this would be simpler / more clear if it returned a mojo type. (Would need to return a default value after NOTREACHED() but that is sometimes unavoidable).
https://codereview.chromium.org/1925083003/diff/260001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1925083003/diff/260001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:384: const base::DictionaryValue* dict) { On 2016/05/18 15:35:48, stevenjb wrote: > On 2016/05/18 05:32:45, dcheng wrote: > > I've expressed this in past reviews > > > (https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc...), > > but I find this pretty unreadable. Can we encapsulate the ONC state into a > class > > and provide a ToValue() for things that want the base::Value? This avoids the > > whole "serialize everything into a base::Value and then parse it back out" > > complexity. > > That would be a huge amount of work and, imho, a step backwards. > > What would be much more useful would be an ONC .mojom file and a data driven ONC > base::DictionaryValue -> mojo library. Then the translation from ONC -> Arc > could happen in the arc layer instead of in Chrome (which would just pass > complete ONC dictionaries to Arc via mojo). > > If I was not completely swamped with other responsibilities I would try to drive > that effort, but unfortunately my focus is elsewhere. Acknowledged. https://codereview.chromium.org/1925083003/diff/260001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:419: } On 2016/05/18 21:00:18, stevenjb wrote: > All non member / non public functions should be in the anonymous namespace. Done. https://codereview.chromium.org/1925083003/diff/260001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:511: } On 2016/05/18 21:00:18, stevenjb wrote: > I think this would be simpler / more clear if it returned a mojo type. (Would > need to return a default value after NOTREACHED() but that is sometimes > unavoidable). It is also setting mojo->wifi (and will someday set the equivalent fields for ethernet, cellular, etc.).
lgtm with nits https://codereview.chromium.org/1925083003/diff/280001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1925083003/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:55: std::string tmp; nit: value? https://codereview.chromium.org/1925083003/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:64: std::string tmp = GetStringFromOncDictionary(dict, onc::wifi::kSecurity, nit: type? https://codereview.chromium.org/1925083003/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:79: arc::mojom::WiFiPtr mojo = arc::mojom::WiFi::New(); nit: wifi? https://codereview.chromium.org/1925083003/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:101: mojo::Array<mojo::String> mojos = mojo::Array<mojo::String>::New(0); nit: strings? https://codereview.chromium.org/1925083003/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:115: mojo::Array<arc::mojom::IPConfigurationPtr> mojos = nit: configs? https://codereview.chromium.org/1925083003/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:120: arc::mojom::IPConfigurationPtr mojo = arc::mojom::IPConfiguration::New(); nit: configuration? https://codereview.chromium.org/1925083003/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:155: std::string tmp = GetStringFromOncDictionary( nit: connection_state? https://codereview.chromium.org/1925083003/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:171: std::string tmp = GetStringFromOncDictionary(dict, onc::network_config::kType, nit: type?
https://codereview.chromium.org/1925083003/diff/280001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1925083003/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:55: std::string tmp; On 2016/05/18 21:41:57, Luis Héctor Chávez (slow 5-23) wrote: > nit: value? Done. https://codereview.chromium.org/1925083003/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:64: std::string tmp = GetStringFromOncDictionary(dict, onc::wifi::kSecurity, On 2016/05/18 21:41:57, Luis Héctor Chávez (slow 5-23) wrote: > nit: type? Done. https://codereview.chromium.org/1925083003/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:79: arc::mojom::WiFiPtr mojo = arc::mojom::WiFi::New(); On 2016/05/18 21:41:57, Luis Héctor Chávez (slow 5-23) wrote: > nit: wifi? Done. https://codereview.chromium.org/1925083003/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:101: mojo::Array<mojo::String> mojos = mojo::Array<mojo::String>::New(0); On 2016/05/18 21:41:57, Luis Héctor Chávez (slow 5-23) wrote: > nit: strings? Done. https://codereview.chromium.org/1925083003/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:115: mojo::Array<arc::mojom::IPConfigurationPtr> mojos = On 2016/05/18 21:41:57, Luis Héctor Chávez (slow 5-23) wrote: > nit: configs? Done. https://codereview.chromium.org/1925083003/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:120: arc::mojom::IPConfigurationPtr mojo = arc::mojom::IPConfiguration::New(); On 2016/05/18 21:41:57, Luis Héctor Chávez (slow 5-23) wrote: > nit: configuration? Done. https://codereview.chromium.org/1925083003/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:155: std::string tmp = GetStringFromOncDictionary( On 2016/05/18 21:41:57, Luis Héctor Chávez (slow 5-23) wrote: > nit: connection_state? Done. https://codereview.chromium.org/1925083003/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:171: std::string tmp = GetStringFromOncDictionary(dict, onc::network_config::kType, On 2016/05/18 21:41:57, Luis Héctor Chávez (slow 5-23) wrote: > nit: type? Done.
The ONC translation is opaque and unmaintainable. It's impossible for me to verify correctness there, so I'm just going to have to assume it's correct. stevenjb: if you want some sort of generator, that would definitely improve the situation, but the internal representation should still be a C++ object with strong types. That way, a reader can actually look at a class definition and understand how the various bits and pieces all go together. Given the loosely-typed nature of base::Value, that's basically impossible as it stands. I just have one comment that needs to be addressed. https://codereview.chromium.org/1925083003/diff/300001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1925083003/diff/300001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:610: base::Bind(&DefaultNetworkSuccessCallback, base::Unretained(this)), Why is Unretained safe here?
https://codereview.chromium.org/1925083003/diff/300001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1925083003/diff/300001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:358: } Move these two to anon namespace. https://codereview.chromium.org/1925083003/diff/300001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:418: } Move these two to anon namespace. https://codereview.chromium.org/1925083003/diff/300001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:451: } Move these two to anon namespace. https://codereview.chromium.org/1925083003/diff/300001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:558: } Move these two to anon namespace. https://codereview.chromium.org/1925083003/diff/300001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:584: } See comment below. https://codereview.chromium.org/1925083003/diff/300001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:591: Move this to anon namespace. https://codereview.chromium.org/1925083003/diff/300001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:610: base::Bind(&DefaultNetworkSuccessCallback, base::Unretained(this)), On 2016/05/18 22:31:57, dcheng wrote: > Why is Unretained safe here? +1, I missed this (I was mostly focused on the NetworkHandler integration). This is not safe, you need to use a WeakPtrFactory.. Also DefaultNetworkSuccessCallback should be a member instead of taking |instance| as a parameter.
https://codereview.chromium.org/1925083003/diff/300001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1925083003/diff/300001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:358: } On 2016/05/18 23:01:17, stevenjb wrote: > Move these two to anon namespace. Done. https://codereview.chromium.org/1925083003/diff/300001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:418: } On 2016/05/18 23:01:17, stevenjb wrote: > Move these two to anon namespace. Done. https://codereview.chromium.org/1925083003/diff/300001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:451: } On 2016/05/18 23:01:17, stevenjb wrote: > Move these two to anon namespace. Done. https://codereview.chromium.org/1925083003/diff/300001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:558: } On 2016/05/18 23:01:17, stevenjb wrote: > Move these two to anon namespace. Done. https://codereview.chromium.org/1925083003/diff/300001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:584: } On 2016/05/18 23:01:17, stevenjb wrote: > See comment below. Done. https://codereview.chromium.org/1925083003/diff/300001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:591: On 2016/05/18 23:01:17, stevenjb wrote: > Move this to anon namespace. Done. https://codereview.chromium.org/1925083003/diff/300001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:610: base::Bind(&DefaultNetworkSuccessCallback, base::Unretained(this)), On 2016/05/18 22:31:57, dcheng wrote: > Why is Unretained safe here? Done.
Thanks. lgtm w/nit https://codereview.chromium.org/1925083003/diff/320001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.h (right): https://codereview.chromium.org/1925083003/diff/320001/components/arc/net/arc... components/arc/net/arc_net_host_impl.h:16: #include "base/values.h" Instead forward declare DictionaryValue
mojom lgtm
https://codereview.chromium.org/1925083003/diff/320001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.h (right): https://codereview.chromium.org/1925083003/diff/320001/components/arc/net/arc... components/arc/net/arc_net_host_impl.h:16: #include "base/values.h" On 2016/05/18 23:28:06, stevenjb wrote: > Instead forward declare DictionaryValue Done.
The CQ bit was checked by cernekee@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from abhishekbh@google.com, lhchavez@chromium.org, stevenjb@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1925083003/#ps340001 (title: "forward-declare base::DictionaryValue")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1925083003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1925083003/340001
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Notify ARC of default network changes Track the default logical service (i.e. VPN or physical interface) and notify ARC when it changes. Allow ARC to query the default service at startup time. This reports IP information, MAC address, wifi parameters, etc. BUG=b/26495433 ========== to ========== Notify ARC of default network changes Track the default logical service (i.e. VPN or physical interface) and notify ARC when it changes. Allow ARC to query the default service at startup time. This reports IP information, MAC address, wifi parameters, etc. BUG=b/26495433 Committed: https://crrev.com/69d6c93bea89bafef588b470c87f240577805f5c Cr-Commit-Position: refs/heads/master@{#394836} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/69d6c93bea89bafef588b470c87f240577805f5c Cr-Commit-Position: refs/heads/master@{#394836} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
