Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1572483002:
Implement OnGetNetworks for net.mojom (Closed)
|
Created:
4 years, 11 months ago by Kevin Cernekee Modified:
4 years, 11 months ago CC:
Aaron Boodman, abarth-chromium, abhishekbh, ben+mojo_chromium.org, blundell+watchlist_chromium.org, cbentzel+watch_chromium.org, chromium-reviews, darin (slow to review), droger+watchlist_chromium.org, qsr+mojo_chromium.org, sdefresne+watchlist_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement OnGetNetworks for net.mojom
This allows ARC to request the list of visible wifi networks.
BUG=b:26369403
TEST=manual
Signed-off-by: Abhishek Bhardwaj <abhishekbh@chromium.org>;
Signed-off-by: Kevin Cernekee <cernekee@chromium.org>;
Committed: https://crrev.com/6ad6767dc0e6a62244abf243305bea594f94f882
Cr-Commit-Position: refs/heads/master@{#371421}
Patch Set 1 #Patch Set 2 : #
Total comments: 12
Patch Set 3 : #
Total comments: 38
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 6
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : Tweak gypi deps to try to fix trybot failure #Patch Set 10 : fix onc dep, take 2 #
Total comments: 8
Patch Set 11 : fix more review comments #
Total comments: 11
Patch Set 12 : rebase + incorporate code review feedback #
Total comments: 4
Patch Set 13 : add missing OnNetInstanceReady and CloseNetChannel calls #
Total comments: 7
Patch Set 14 : rebase + add checks on GetInteger / GetString #Patch Set 15 : remove unnecessary #include #
Total comments: 7
Patch Set 16 : add more DCHECKs #Patch Set 17 : remove weak_factory_ #
Messages
Total messages: 56 (10 generated)
cernekee@google.com changed reviewers: + cernekee@google.com, hidehiko@chromium.org, kenrb@chromium.org, lhchavez@chromium.org, stevenjb@chromium.org
https://codereview.chromium.org/1572483002/diff/20001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1572483002/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_service.h:96: // Called whenever the ARC net interface state changes. nit: Remove this header and the one below to make this a simple list. https://codereview.chromium.org/1572483002/diff/20001/components/arc/arc_serv... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/1572483002/diff/20001/components/arc/arc_serv... components/arc/arc_service_manager.cc:35: arc_net_host_(ArcNetHostImpl::CreateNetHost(arc_bridge_service_.get())), Since CreateNetHost is trivial, how about just newing it instead of adding a static function to keep consistency? https://codereview.chromium.org/1572483002/diff/20001/components/arc/arc_serv... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1572483002/diff/20001/components/arc/arc_serv... components/arc/arc_service_manager.h:47: scoped_ptr<arc::NetHost> arc_net_host_; Any reason you are not using scoped_ptr<ArcNetHostImpl> here? https://codereview.chromium.org/1572483002/diff/20001/components/arc/common/a... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/1572483002/diff/20001/components/arc/common/a... components/arc/common/arc_bridge.mojom:35: OnNetInstanceReady@110(NetInstance instance_ptr); You need to add [MinVersion=3] (or whatever is in ToT just before sending it to CQ + 1), and use @108 since IME is 110. https://codereview.chromium.org/1572483002/diff/20001/components/arc/common/n... File components/arc/common/net.mojom (right): https://codereview.chromium.org/1572483002/diff/20001/components/arc/common/n... components/arc/common/net.mojom:28: OnGetNetworks(int32 request_id, bool configured_only, bool visible_only); Is the plan to only send the networks to ARC after calling this method? Or will there be an observer that continuously sends an updated list? If you will do the former, it's probably better to change this function to GetNetworks(bool configured_only, bool visible_only) => (NetworkData data); and drop the TODO and the SendNetworks method below.
https://codereview.chromium.org/1572483002/diff/20001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1572483002/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_service.h:96: // Called whenever the ARC net interface state changes. On 2016/01/12 17:32:32, Luis Héctor Chávez wrote: > nit: Remove this header and the one below to make this a simple list. Not sure I follow. You're saying to remove the comments for OnNetInstanceReady() and OnPowerInstance*(), but leave the comments for everything else in the list? Or to delete all of these boilerplate comments from the class definition? https://codereview.chromium.org/1572483002/diff/20001/components/arc/arc_serv... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/1572483002/diff/20001/components/arc/arc_serv... components/arc/arc_service_manager.cc:35: arc_net_host_(ArcNetHostImpl::CreateNetHost(arc_bridge_service_.get())), On 2016/01/12 17:32:32, Luis Héctor Chávez wrote: > Since CreateNetHost is trivial, how about just newing it instead of adding a > static function to keep consistency? Done. https://codereview.chromium.org/1572483002/diff/20001/components/arc/common/a... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/1572483002/diff/20001/components/arc/common/a... components/arc/common/arc_bridge.mojom:35: OnNetInstanceReady@110(NetInstance instance_ptr); On 2016/01/12 17:32:32, Luis Héctor Chávez wrote: > You need to add [MinVersion=3] (or whatever is in ToT just before sending it to > CQ + 1), and use @108 since IME is 110. OK. What is IME? https://codereview.chromium.org/1572483002/diff/20001/components/arc/common/n... File components/arc/common/net.mojom (right): https://codereview.chromium.org/1572483002/diff/20001/components/arc/common/n... components/arc/common/net.mojom:28: OnGetNetworks(int32 request_id, bool configured_only, bool visible_only); On 2016/01/12 17:32:32, Luis Héctor Chávez wrote: > Is the plan to only send the networks to ARC after calling this method? Or will > there be an observer that continuously sends an updated list? > > If you will do the former, it's probably better to change this function to > > GetNetworks(bool configured_only, bool visible_only) => (NetworkData data); > > and drop the TODO and the SendNetworks method below. Not sure yet. There is an undocumented API on the other side that might require us to send periodic updates. Is there an example of how to use the "=>" syntax? I see RequestProcessList() and GetAuthCode() but neither one seems to be implemented in C++ yet.
https://codereview.chromium.org/1572483002/diff/20001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1572483002/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_service.h:96: // Called whenever the ARC net interface state changes. On 2016/01/12 20:14:47, cernekee wrote: > On 2016/01/12 17:32:32, Luis Héctor Chávez wrote: > > nit: Remove this header and the one below to make this a simple list. > > Not sure I follow. You're saying to remove the comments for > OnNetInstanceReady() and OnPowerInstance*(), but leave the comments for > everything else in the list? > > Or to delete all of these boilerplate comments from the class definition? Wait, the diff I was seeing was weird. The code is fine as-is. Disregard this comment. https://codereview.chromium.org/1572483002/diff/20001/components/arc/common/a... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/1572483002/diff/20001/components/arc/common/a... components/arc/common/arc_bridge.mojom:35: OnNetInstanceReady@110(NetInstance instance_ptr); On 2016/01/12 20:14:47, cernekee wrote: > On 2016/01/12 17:32:32, Luis Héctor Chávez wrote: > > You need to add [MinVersion=3] (or whatever is in ToT just before sending it > to > > CQ + 1), and use @108 since IME is 110. > > OK. What is IME? It's another interface for input. https://codereview.chromium.org/1572483002/diff/20001/components/arc/common/n... File components/arc/common/net.mojom (right): https://codereview.chromium.org/1572483002/diff/20001/components/arc/common/n... components/arc/common/net.mojom:28: OnGetNetworks(int32 request_id, bool configured_only, bool visible_only); On 2016/01/12 20:14:47, cernekee wrote: > On 2016/01/12 17:32:32, Luis Héctor Chávez wrote: > > Is the plan to only send the networks to ARC after calling this method? Or > will > > there be an observer that continuously sends an updated list? > > > > If you will do the former, it's probably better to change this function to > > > > GetNetworks(bool configured_only, bool visible_only) => (NetworkData data); > > > > and drop the TODO and the SendNetworks method below. > > Not sure yet. There is an undocumented API on the other side that might require > us to send periodic updates. > > Is there an example of how to use the "=>" syntax? I see RequestProcessList() > and GetAuthCode() but neither one seems to be implemented in C++ yet. the C++ code should look like void GetNetworks(bool configured_only, bool visible_only, const GetNetworksCallback& callback) { NetworkData data; // ... callback.Run(std::move(data)); }
https://codereview.chromium.org/1572483002/diff/40001/components/arc/common/n... File components/arc/common/net.mojom (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/common/n... components/arc/common/net.mojom:12: struct WifiConfiguration { We should document these properties. Do they correspond to ONC properties? Android properties? What are the units / ranges of frequency and signal_strength? What strings are valid for security? https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:52: NetworkDataPtr data) { align https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:68: chromeos::network_util::TranslateNetworkListToONC( 4 space indent https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:76: WifiConfigurationPtr wc = WifiConfiguration::New(); 2 space indent https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:84: wc->ssid = tmp; Name != SSID. Use kSSID or call the member name. (They are often the same but not always). https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... File components/arc/net/arc_net_host_impl.h (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.h:31: void GetNetworksFromChrome(bool configured_only, Unused?
abhishekbh@google.com changed reviewers: + abhishekbh@google.com
lgtm https://codereview.chromium.org/1572483002/diff/40001/components/arc/arc_serv... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:47: scoped_ptr<arc::NetHost> arc_net_host_; should this be ArcNetHostImpl ? https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... File components/arc/net/arc_net_host_impl.h (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.h:31: void GetNetworksFromChrome(bool configured_only, On 2016/01/12 22:14:04, stevenjb wrote: > Unused? There is another Android API that will ask for configured networks, that flag will be used then.
https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... File components/arc/net/arc_net_host_impl.h (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.h:31: void GetNetworksFromChrome(bool configured_only, On 2016/01/12 22:42:26, abhishekbh wrote: > On 2016/01/12 22:14:04, stevenjb wrote: > > Unused? > > There is another Android API that will ask for configured networks, that flag > will be used then. I mean the entire function does not exist in the C++ file.
https://codereview.chromium.org/1572483002/diff/40001/components/arc/arc_serv... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:47: scoped_ptr<arc::NetHost> arc_net_host_; On 2016/01/12 22:42:26, abhishekbh wrote: > should this be ArcNetHostImpl ? Per lhchavez we were able to bypass some of the indirection layers that the other calls are using, because we don't have to call into certain parts of Chrome. (Sorry but I'm a little sketchy on the details - Luis could provide a much better explanation than I can.) https://codereview.chromium.org/1572483002/diff/40001/components/arc/common/n... File components/arc/common/net.mojom (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/common/n... components/arc/common/net.mojom:12: struct WifiConfiguration { On 2016/01/12 22:14:04, stevenjb wrote: > We should document these properties. Do they correspond to ONC properties? > Android properties? What are the units / ranges of frequency and > signal_strength? What strings are valid for security? Done. https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:52: NetworkDataPtr data) { On 2016/01/12 22:14:04, stevenjb wrote: > align Done. https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:68: chromeos::network_util::TranslateNetworkListToONC( On 2016/01/12 22:14:04, stevenjb wrote: > 4 space indent Done. https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:76: WifiConfigurationPtr wc = WifiConfiguration::New(); On 2016/01/12 22:14:04, stevenjb wrote: > 2 space indent Done. https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:84: wc->ssid = tmp; On 2016/01/12 22:14:04, stevenjb wrote: > Name != SSID. Use kSSID or call the member name. (They are often the same but > not always). > OK, fixed. Does the string handling / casting / pointer stuff look sane to you? Is there a cleaner way to do this without the temp variable? https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... File components/arc/net/arc_net_host_impl.h (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.h:31: void GetNetworksFromChrome(bool configured_only, On 2016/01/12 22:14:04, stevenjb wrote: > Unused? Yes, left over from a previous iteration. Fixed.
https://codereview.chromium.org/1572483002/diff/40001/components/arc/arc_serv... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:47: scoped_ptr<arc::NetHost> arc_net_host_; On 2016/01/12 23:53:27, Kevin Cernekee wrote: > On 2016/01/12 22:42:26, abhishekbh wrote: > > should this be ArcNetHostImpl ? > > Per lhchavez we were able to bypass some of the indirection layers that the > other calls are using, because we don't have to call into certain parts of > Chrome. > > (Sorry but I'm a little sketchy on the details - Luis could provide a much > better explanation than I can.) Some of the other services require to use this indirection since they are instantiated elsewhere (typically in ChromeBrowserMain), but the ownership is then transferred to the ArcServiceManager. The reason they are instantiated there is that they depend on directories that code in components/arc/ is now allowed to depend on (like chrome/ or content/). The current net implementation does not fall into that category IIUC, so we should be able to use the concrete class here.
Quick walk through. Probably, you're forgetting to upload the latest PS? Also, could you run trybots? https://codereview.chromium.org/1572483002/diff/40001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_service.h:97: virtual void OnNetInstanceReady() {} Please follow the current code around here for this class. https://codereview.chromium.org/1572483002/diff/40001/components/arc/arc_serv... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:47: scoped_ptr<arc::NetHost> arc_net_host_; nit: here in arc:: namespace. https://codereview.chromium.org/1572483002/diff/40001/components/arc/common/a... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/common/a... components/arc/common/arc_bridge.mojom:35: [MinVersion=3] OnNetInstanceReady@108(NetInstance instance_ptr); Luis, please manage the @number. https://codereview.chromium.org/1572483002/diff/40001/components/arc/common/n... File components/arc/common/net.mojom (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/common/n... components/arc/common/net.mojom:12: struct WifiConfiguration { On 2016/01/12 23:53:27, Kevin Cernekee wrote: > On 2016/01/12 22:14:04, stevenjb wrote: > > We should document these properties. Do they correspond to ONC properties? > > Android properties? What are the units / ranges of frequency and > > signal_strength? What strings are valid for security? > > Done. Haven't updated yet? https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:39: DCHECK(origin_task_runner_->RunsTasksOnCurrentThread()); base::ThreadChecker instead? https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:44: DCHECK(origin_task_runner_->RunsTasksOnCurrentThread()); Ditto. https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:80: DCHECK(network_dict); Instead, you may want to check the return value of GetAsDictionary()?
Can you also run `git cl format` and `git cl lint` while we're at it? They'll probably come back clean, but just in case. https://codereview.chromium.org/1572483002/diff/40001/components/arc/common/a... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/common/a... components/arc/common/arc_bridge.mojom:35: [MinVersion=3] OnNetInstanceReady@108(NetInstance instance_ptr); On 2016/01/13 05:58:56, hidehiko wrote: > Luis, please manage the @number. This is the correct one.
https://codereview.chromium.org/1572483002/diff/40001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_service.h:97: virtual void OnNetInstanceReady() {} On 2016/01/13 05:58:56, hidehiko wrote: > Please follow the current code around here for this class. Could you please clarify what you mean by that? https://codereview.chromium.org/1572483002/diff/40001/components/arc/common/a... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/common/a... components/arc/common/arc_bridge.mojom:35: [MinVersion=3] OnNetInstanceReady@108(NetInstance instance_ptr); On 2016/01/13 16:43:10, Luis Héctor Chávez wrote: > On 2016/01/13 05:58:56, hidehiko wrote: > > Luis, please manage the @number. > > This is the correct one. Acknowledged. https://codereview.chromium.org/1572483002/diff/40001/components/arc/common/n... File components/arc/common/net.mojom (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/common/n... components/arc/common/net.mojom:12: struct WifiConfiguration { On 2016/01/13 05:58:56, hidehiko wrote: > On 2016/01/12 23:53:27, Kevin Cernekee wrote: > > On 2016/01/12 22:14:04, stevenjb wrote: > > > We should document these properties. Do they correspond to ONC properties? > > > Android properties? What are the units / ranges of frequency and > > > signal_strength? What strings are valid for security? > > > > Done. > > Haven't updated yet? I'm still working on the new PS as there were questions about the PS 3 feedback. https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:39: DCHECK(origin_task_runner_->RunsTasksOnCurrentThread()); On 2016/01/13 05:58:56, hidehiko wrote: > base::ThreadChecker instead? Done. https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:44: DCHECK(origin_task_runner_->RunsTasksOnCurrentThread()); On 2016/01/13 05:58:56, hidehiko wrote: > Ditto. Done. https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:80: DCHECK(network_dict); On 2016/01/13 05:58:56, hidehiko wrote: > Instead, you may want to check the return value of GetAsDictionary()? Done. https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:84: wc->ssid = tmp; On 2016/01/12 23:53:27, Kevin Cernekee wrote: > On 2016/01/12 22:14:04, stevenjb wrote: > > Name != SSID. Use kSSID or call the member name. (They are often the same but > > not always). > > > > OK, fixed. I take that back. I did not see where kSSID gets propagated from shill. I could translate the hex SSID, but I don't know if that is reliable? There are checks on the Chrome side which imply that it might not always be populated.
https://codereview.chromium.org/1572483002/diff/40001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_service.h:97: virtual void OnNetInstanceReady() {} On 2016/01/14 03:08:58, cernekee wrote: > On 2016/01/13 05:58:56, hidehiko wrote: > > Please follow the current code around here for this class. > > Could you please clarify what you mean by that? You need to support - "close"ing functions and the callback. - "version" related functions.
https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:80: DCHECK(network_dict); On 2016/01/14 03:08:58, cernekee wrote: > On 2016/01/13 05:58:56, hidehiko wrote: > > Instead, you may want to check the return value of GetAsDictionary()? > > Done. Actually, the DCHECK was fine (and preferred). It should not be possible for TranslateNetworkListToONC to include a nullptr in the returned ListValue. https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:84: wc->ssid = tmp; On 2016/01/14 03:08:58, cernekee wrote: > On 2016/01/12 23:53:27, Kevin Cernekee wrote: > > On 2016/01/12 22:14:04, stevenjb wrote: > > > Name != SSID. Use kSSID or call the member name. (They are often the same > but > > > not always). > > > > > > > OK, fixed. > > I take that back. I did not see where kSSID gets propagated from shill. > > I could translate the hex SSID, but I don't know if that is reliable? There are > checks on the Chrome side which imply that it might not always be populated. Apologies, I forgot that we actually process the Hex SSID to get as consistent / reliable a "name" as possible. This is fine, but we might want to add a comment to that effect. https://codereview.chromium.org/1572483002/diff/60001/components/arc/common/n... File components/arc/common/net.mojom (right): https://codereview.chromium.org/1572483002/diff/60001/components/arc/common/n... components/arc/common/net.mojom:13: // These correspond to various ONC properties returned by s/various// Also, maybe reference components/onc/docs/onc_spec.html https://codereview.chromium.org/1572483002/diff/60001/components/arc/net/arc_... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1572483002/diff/60001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:81: continue; DCHECK
https://codereview.chromium.org/1572483002/diff/40001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_service.h:97: virtual void OnNetInstanceReady() {} On 2016/01/14 04:06:32, hidehiko wrote: > On 2016/01/14 03:08:58, cernekee wrote: > > On 2016/01/13 05:58:56, hidehiko wrote: > > > Please follow the current code around here for this class. > > > > Could you please clarify what you mean by that? > > You need to support > - "close"ing functions and the callback. > - "version" related functions. Done. https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:80: DCHECK(network_dict); On 2016/01/14 18:18:54, stevenjb wrote: > On 2016/01/14 03:08:58, cernekee wrote: > > On 2016/01/13 05:58:56, hidehiko wrote: > > > Instead, you may want to check the return value of GetAsDictionary()? > > > > Done. > > Actually, the DCHECK was fine (and preferred). It should not be possible for > TranslateNetworkListToONC to include a nullptr in the returned ListValue. Done. https://codereview.chromium.org/1572483002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:84: wc->ssid = tmp; On 2016/01/14 18:18:54, stevenjb wrote: > On 2016/01/14 03:08:58, cernekee wrote: > > On 2016/01/12 23:53:27, Kevin Cernekee wrote: > > > On 2016/01/12 22:14:04, stevenjb wrote: > > > > Name != SSID. Use kSSID or call the member name. (They are often the same > > but > > > > not always). > > > > > > > > > > OK, fixed. > > > > I take that back. I did not see where kSSID gets propagated from shill. > > > > I could translate the hex SSID, but I don't know if that is reliable? There > are > > checks on the Chrome side which imply that it might not always be populated. > > Apologies, I forgot that we actually process the Hex SSID to get as consistent / > reliable a "name" as possible. This is fine, but we might want to add a comment > to that effect. Done. https://codereview.chromium.org/1572483002/diff/60001/components/arc/common/n... File components/arc/common/net.mojom (right): https://codereview.chromium.org/1572483002/diff/60001/components/arc/common/n... components/arc/common/net.mojom:13: // These correspond to various ONC properties returned by On 2016/01/14 18:18:54, stevenjb wrote: > s/various// > Also, maybe reference components/onc/docs/onc_spec.html Done. https://codereview.chromium.org/1572483002/diff/60001/components/arc/net/arc_... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1572483002/diff/60001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:81: continue; On 2016/01/14 18:18:54, stevenjb wrote: > DCHECK Done.
Again, please run trybots. https://codereview.chromium.org/1572483002/diff/80001/components/arc/arc_brid... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/1572483002/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:166: FOR_EACH_OBSERVER(Observer, observer_list(), OnNetInstanceReady()); This needs to be called after set_connection_error_handler() below (please see also the ToT code). https://codereview.chromium.org/1572483002/diff/80001/components/arc/common/a... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/1572483002/diff/80001/components/arc/common/a... components/arc/common/arc_bridge.mojom:35: [MinVersion=3] OnNetInstanceReady@108(NetInstance instance_ptr); Probably, MinVersion=4 or bigger. Please re-check before submitting. https://codereview.chromium.org/1572483002/diff/80001/components/arc/net/arc_... File components/arc/net/arc_net_host_impl.h (right): https://codereview.chromium.org/1572483002/diff/80001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.h:23: // Private implementation of ArcNetHost nit: '.' at the end.
https://codereview.chromium.org/1572483002/diff/80001/components/arc/arc_brid... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/1572483002/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:166: FOR_EACH_OBSERVER(Observer, observer_list(), OnNetInstanceReady()); On 2016/01/15 08:38:39, hidehiko wrote: > This needs to be called after set_connection_error_handler() below (please see > also the ToT code). Done. https://codereview.chromium.org/1572483002/diff/80001/components/arc/common/a... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/1572483002/diff/80001/components/arc/common/a... components/arc/common/arc_bridge.mojom:35: [MinVersion=3] OnNetInstanceReady@108(NetInstance instance_ptr); On 2016/01/15 08:38:39, hidehiko wrote: > Probably, MinVersion=4 or bigger. Please re-check before submitting. Done. https://codereview.chromium.org/1572483002/diff/80001/components/arc/net/arc_... File components/arc/net/arc_net_host_impl.h (right): https://codereview.chromium.org/1572483002/diff/80001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.h:23: // Private implementation of ArcNetHost On 2016/01/15 08:38:39, hidehiko wrote: > nit: '.' at the end. Done.
https://codereview.chromium.org/1572483002/diff/180001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1572483002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.h:57: scoped_ptr<arc::NetHost> arc_net_host_; This is going away soon, but ArcNetHostImpl? https://codereview.chromium.org/1572483002/diff/180001/components/arc/common/... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/1572483002/diff/180001/components/arc/common/... components/arc/common/arc_bridge.mojom:40: [MinVersion=4] OnNetInstanceReady@108(NetInstance instance_ptr); FYI: If https://codereview.chromium.org/1590723003/ lands first you'll need to bump to version 5. https://codereview.chromium.org/1572483002/diff/180001/components/arc/common/... File components/arc/common/net.mojom (right): https://codereview.chromium.org/1572483002/diff/180001/components/arc/common/... components/arc/common/net.mojom:31: GetNetworks(bool configured_only, bool visible_only) => (NetworkData data); Other interfaces have started doing it, so can you add an @0 ordinal to both GetNetworks and Init? https://codereview.chromium.org/1572483002/diff/180001/components/arc/common/... components/arc/common/net.mojom:34: // TODO(lhchavez): Migrate all request/response messages to Mojo. Your interface already uses Mojo-style request/response methods, so this comment is not needed :)
https://codereview.chromium.org/1572483002/diff/180001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1572483002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.h:57: scoped_ptr<arc::NetHost> arc_net_host_; On 2016/01/19 20:51:58, Luis Héctor Chávez wrote: > This is going away soon, but ArcNetHostImpl? Done. https://codereview.chromium.org/1572483002/diff/180001/components/arc/common/... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/1572483002/diff/180001/components/arc/common/... components/arc/common/arc_bridge.mojom:40: [MinVersion=4] OnNetInstanceReady@108(NetInstance instance_ptr); On 2016/01/19 20:51:58, Luis Héctor Chávez wrote: > FYI: If https://codereview.chromium.org/1590723003/ lands first you'll need to > bump to version 5. Done. https://codereview.chromium.org/1572483002/diff/180001/components/arc/common/... File components/arc/common/net.mojom (right): https://codereview.chromium.org/1572483002/diff/180001/components/arc/common/... components/arc/common/net.mojom:31: GetNetworks(bool configured_only, bool visible_only) => (NetworkData data); On 2016/01/19 20:51:58, Luis Héctor Chávez wrote: > Other interfaces have started doing it, so can you add an @0 ordinal to both > GetNetworks and Init? Done. https://codereview.chromium.org/1572483002/diff/180001/components/arc/common/... components/arc/common/net.mojom:34: // TODO(lhchavez): Migrate all request/response messages to Mojo. On 2016/01/19 20:51:58, Luis Héctor Chávez wrote: > Your interface already uses Mojo-style request/response methods, so this comment > is not needed :) Done.
https://codereview.chromium.org/1572483002/diff/200001/components/arc/BUILD.gn File components/arc/BUILD.gn (right): https://codereview.chromium.org/1572483002/diff/200001/components/arc/BUILD.g... components/arc/BUILD.gn:27: "net/arc_net_host_impl.cc", This has chromeos specific code in it, so it needs to be either named arc_net_host_chromeos or the chromeos specific code needs to be protected by an #ifdef. https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:15: #include "chromeos/network/onc/onc_utils.h" This should fail to compile on non chromeos platforms?
https://codereview.chromium.org/1572483002/diff/200001/components/arc/BUILD.gn File components/arc/BUILD.gn (right): https://codereview.chromium.org/1572483002/diff/200001/components/arc/BUILD.g... components/arc/BUILD.gn:27: "net/arc_net_host_impl.cc", On 2016/01/20 01:43:42, stevenjb wrote: > This has chromeos specific code in it, so it needs to be either named > arc_net_host_chromeos or the chromeos specific code needs to be protected by an > #ifdef. Is that true for other modules under components/arc/ as well?
Sorry, old habits. I forgot that this is arc specific, which presumably is always going to be chromeos only? Since there are already other chromeos dependencies in arc, this is fine then. lgtm On Tue, Jan 19, 2016 at 5:48 PM, <cernekee@google.com> wrote: > > > https://codereview.chromium.org/1572483002/diff/200001/components/arc/BUILD.gn > File components/arc/BUILD.gn (right): > > > https://codereview.chromium.org/1572483002/diff/200001/components/arc/BUILD.g... > components/arc/BUILD.gn:27: "net/arc_net_host_impl.cc", > On 2016/01/20 01:43:42, stevenjb wrote: > >> This has chromeos specific code in it, so it needs to be either named >> arc_net_host_chromeos or the chromeos specific code needs to be >> > protected by an > >> #ifdef. >> > > Is that true for other modules under components/arc/ as well? > > https://codereview.chromium.org/1572483002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
lhchavez@chromium.org changed reviewers: + dcheng@chromium.org
please rebase and make sure that the MinVersion in the .mojom file is up to date, but otherwise lgtm. dcheng@ PTAL the .mojom files.
https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:59: chromeos::network_util::TranslateNetworkListToONC( Can we just call GetNetworkListByType() directly? It seems like it'd be more straightforward (and type safe) than having to extract everything from base::Value. https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.h (right): https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.h:41: ArcBridgeService* arc_bridge_service_; Nit: ArcBridgeService* const
lhchavez@google.com changed reviewers: + lhchavez@google.com
https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.h (right): https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.h:41: ArcBridgeService* arc_bridge_service_; On 2016/01/22 07:17:43, dcheng wrote: > Nit: ArcBridgeService* const Actually, remove this completely since you need to inherit from ArcService, which already provides arc_bridge_service() for this. See other services in ToT for examples.
https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:15: #include "chromeos/network/onc/onc_utils.h" On 2016/01/20 01:43:42, stevenjb wrote: > This should fail to compile on non chromeos platforms? Acknowledged. https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:59: chromeos::network_util::TranslateNetworkListToONC( Not sure - I think abhishekbh and stevenjb made a decision early-on to stick with ONC interfaces. Possibly because ONC is our published API so it's less likely to break ARC++ if the underlying implementation changes. https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.h (right): https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.h:41: ArcBridgeService* arc_bridge_service_; On 2016/01/22 15:56:19, lhc(google) wrote: > On 2016/01/22 07:17:43, dcheng wrote: > > Nit: ArcBridgeService* const > > Actually, remove this completely since you need to inherit from ArcService, > which already provides arc_bridge_service() for this. See other services in ToT > for examples. Done.
https://codereview.chromium.org/1572483002/diff/220001/components/arc/arc_bri... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/1572483002/diff/220001/components/arc/arc_bri... components/arc/arc_bridge_service.cc:66: if (notifications_instance()) You need to add: if (net_instance()) observer->OnNetInstanceReady(); https://codereview.chromium.org/1572483002/diff/220001/components/arc/arc_bri... components/arc/arc_bridge_service.cc:406: CloseNotificationsChannel(); You need to add: CloseNetChannel();
https://codereview.chromium.org/1572483002/diff/220001/components/arc/arc_bri... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/1572483002/diff/220001/components/arc/arc_bri... components/arc/arc_bridge_service.cc:66: if (notifications_instance()) On 2016/01/22 20:47:12, lhc(google) wrote: > You need to add: > > if (net_instance()) > observer->OnNetInstanceReady(); Done. https://codereview.chromium.org/1572483002/diff/220001/components/arc/arc_bri... components/arc/arc_bridge_service.cc:406: CloseNotificationsChannel(); On 2016/01/22 20:47:12, lhc(google) wrote: > You need to add: > > CloseNetChannel(); Done.
https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:59: chromeos::network_util::TranslateNetworkListToONC( On 2016/01/22 19:58:41, cernekee wrote: > Not sure - I think abhishekbh and stevenjb made a decision early-on to stick > with ONC interfaces. Possibly because ONC is our published API so it's less > likely to break ARC++ if the underlying implementation changes. ONC is preferred for high level communication between processes. It is (reasonably) well documented and stable. It is definitely a better choice. Ideally we should be passing this ListValue directly to the caller, i.e. there ought to be a translation utility that can convert base::ListValue -> NetworkData.networks. (Does Mojo have a way to distinguish required properties from optional ones?). In the short term, this seems fine as-is. GetInteger and GetString do type checking so there shouldn't be any type safety issues here.
https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1572483002/diff/200001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:59: chromeos::network_util::TranslateNetworkListToONC( On 2016/01/22 at 21:08:28, stevenjb wrote: > On 2016/01/22 19:58:41, cernekee wrote: > > Not sure - I think abhishekbh and stevenjb made a decision early-on to stick > > with ONC interfaces. Possibly because ONC is our published API so it's less > > likely to break ARC++ if the underlying implementation changes. > > ONC is preferred for high level communication between processes. It is (reasonably) well documented and stable. It is definitely a better choice. > > Ideally we should be passing this ListValue directly to the caller, i.e. there ought to be a translation utility that can convert base::ListValue -> NetworkData.networks. (Does Mojo have a way to distinguish required properties from optional ones?). > > In the short term, this seems fine as-is. GetInteger and GetString do type checking so there shouldn't be any type safety issues here. They can fail though (for example, if the format of the underlying data ever changes), and there's no asserts that GetString() or GetInteger succeed. Obviously this should "never happen" but still...
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, stevenjb@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/1572483002/#ps240001 (title: "add missing OnNetInstanceReady and CloseNetChannel calls")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1572483002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1572483002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Hi guys, looks like we still need OWNERS approval for components/arc/common ?
FYI. https://codereview.chromium.org/1572483002/diff/240001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1572483002/diff/240001/components/arc/arc_ser... components/arc/arc_service_manager.h:13: #include "components/arc/common/net.mojom.h" nit: please remove this line.
On 2016/01/24 at 23:35:27, cernekee wrote: > Hi guys, looks like we still need OWNERS approval for components/arc/common ? I'm still waiting for a reply to my earlier comment about no asserts that GetString/GetInteger succeed and/or no tests that verify this.
Added some comments as to how the values should be checked. https://codereview.chromium.org/1572483002/diff/240001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1572483002/diff/240001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:79: wifi_dict->GetInteger(onc::wifi::kSignalStrength, &wc->signal_strength); Is there a way to specify default values in mojo? If so, frequency and signal_strength should default to 0 (indicating the service is disconnected, or these values have not been received yet), and the code above is OK. Otherwise both of these should look like: if (!wifi_dict->GetInteger(onc::wifi::kFrequency, &wc->frequency)) wc->frequency = 0; https://codereview.chromium.org/1572483002/diff/240001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:80: wifi_dict->GetString(onc::wifi::kSecurity, &tmp); This should never fail or return empty (no wifi security == "None"), so we could add: CHECK(!tmp.empty). https://codereview.chromium.org/1572483002/diff/240001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:82: wifi_dict->GetString(onc::wifi::kBSSID, &tmp); We can also CHECK this.
https://codereview.chromium.org/1572483002/diff/240001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1572483002/diff/240001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:79: wifi_dict->GetInteger(onc::wifi::kSignalStrength, &wc->signal_strength); On 2016/01/25 18:58:22, stevenjb wrote: > Is there a way to specify default values in mojo? If so, frequency and > signal_strength should default to 0 (indicating the service is disconnected, or > these values have not been received yet), and the code above is OK. Otherwise > both of these should look like: > > if (!wifi_dict->GetInteger(onc::wifi::kFrequency, &wc->frequency)) > wc->frequency = 0; Done. https://codereview.chromium.org/1572483002/diff/240001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:80: wifi_dict->GetString(onc::wifi::kSecurity, &tmp); On 2016/01/25 18:58:22, stevenjb wrote: > This should never fail or return empty (no wifi security == "None"), so we could > add: > CHECK(!tmp.empty). Done. https://codereview.chromium.org/1572483002/diff/240001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:82: wifi_dict->GetString(onc::wifi::kBSSID, &tmp); On 2016/01/25 18:58:22, stevenjb wrote: > We can also CHECK this. Done.
https://codereview.chromium.org/1572483002/diff/280001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1572483002/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:69: network_dict->GetString(onc::network_config::kName, &tmp); How come there's no DCHECK here? https://codereview.chromium.org/1572483002/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:83: CHECK(!tmp.empty()); DCHECK is fine here, since it should never happen. However, I think this should actually be: if (!wifi_dict->GetString(..., &tmp)) NOTREACHED(); DCHECK(!tmp.empty()); Otherwise |tmp| might already be non-empty from line 69. https://codereview.chromium.org/1572483002/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:87: CHECK(!tmp.empty()); Ditto. https://codereview.chromium.org/1572483002/diff/280001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.h (right): https://codereview.chromium.org/1572483002/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.h:46: base::WeakPtrFactory<ArcNetHostImpl> weak_factory_; This appears to be unused.
https://codereview.chromium.org/1572483002/diff/280001/components/arc/net/arc... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1572483002/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:69: network_dict->GetString(onc::network_config::kName, &tmp); On 2016/01/25 23:35:32, dcheng wrote: > How come there's no DCHECK here? Done. https://codereview.chromium.org/1572483002/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:83: CHECK(!tmp.empty()); On 2016/01/25 23:35:32, dcheng wrote: > DCHECK is fine here, since it should never happen. However, I think this should > actually be: > > if (!wifi_dict->GetString(..., &tmp)) > NOTREACHED(); > DCHECK(!tmp.empty()); > > Otherwise |tmp| might already be non-empty from line 69. Done. https://codereview.chromium.org/1572483002/diff/280001/components/arc/net/arc... components/arc/net/arc_net_host_impl.cc:87: CHECK(!tmp.empty()); On 2016/01/25 23:35:32, dcheng wrote: > Ditto. Done.
lgtm++
mojom changes lgtm, but please remove the weak_factory_ from ArcNetHostImpl. Just add it in a CL where it's actually used.
The CQ bit was checked by cernekee@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, abhishekbh@google.com, stevenjb@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1572483002/#ps310001 (title: "remove weak_factory_")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1572483002/310001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1572483002/310001
Message was sent while issue was closed.
Committed patchset #17 (id:310001)
Message was sent while issue was closed.
Description was changed from ========== Implement OnGetNetworks for net.mojom This allows ARC to request the list of visible wifi networks. BUG=b:26369403 TEST=manual Signed-off-by: Abhishek Bhardwaj <abhishekbh@chromium.org> Signed-off-by: Kevin Cernekee <cernekee@chromium.org> ========== to ========== Implement OnGetNetworks for net.mojom This allows ARC to request the list of visible wifi networks. BUG=b:26369403 TEST=manual Signed-off-by: Abhishek Bhardwaj <abhishekbh@chromium.org> Signed-off-by: Kevin Cernekee <cernekee@chromium.org> Committed: https://crrev.com/6ad6767dc0e6a62244abf243305bea594f94f882 Cr-Commit-Position: refs/heads/master@{#371421} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/6ad6767dc0e6a62244abf243305bea594f94f882 Cr-Commit-Position: refs/heads/master@{#371421} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
