Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1751793002:
ARC: Remove error status setting in GetNetworks. (Closed)
|
Created:
4 years, 9 months ago by abhishekbh Modified:
4 years, 9 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionARC: Remove error status setting in GetNetworks.
This change removes the setting of the status code in GetNetworks. Its redundant, instead in error scenarios an empty list is given back. This change also updated GetNetworks to take in an enum instead of a boolean flag. The old version is deprecated moving forward but supported for existing older versions.
BUG=590963
BUG=b/26496701
Committed: https://crrev.com/96838748969e79cc41c364dbe386f3754741e1bb
Cr-Commit-Position: refs/heads/master@{#379117}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Moved caching logic to android. #Patch Set 3 : Added enum instead of booleans for GetNetworks. #
Total comments: 1
Messages
Total messages: 26 (7 generated)
PTAL.
https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... components/arc/net/arc_net_host_impl.cc:61: const GetNetworksCallback& callback) { nit: add DCHECK(thread_checker_.CalledOnValidThread()); here and all the rest of the methods (except where you expect a different thread to be used. In that case comment what thread you expect. https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... components/arc/net/arc_net_host_impl.cc:66: if (configured_only && visible_only) { Hmmm this is problematic. Can you substitute these two flags with a three-value enum? https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... components/arc/net/arc_net_host_impl.cc:75: visible_networks_lock_.Acquire(); This is the UI thred, so this is not allowed. How about you get rid of this lock and then if ScanCompleted arrives on a different thread than expected, post a task to the main thread? https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... File components/arc/net/arc_net_host_impl.h (right): https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... components/arc/net/arc_net_host_impl.h:50: void ScanCompleted(const chromeos::DeviceState* /*unused*/) override; Can you comment about what thread this is expected to be called as? https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... components/arc/net/arc_net_host_impl.h:71: mojo::Array<arc::WifiConfigurationPtr> cached_visible_networks_; I don't love the idea of a move-only collection being used as a cache and then cloned every time you want to send it over. Given that you still need to do copying, would it make sense to make it a std::vector<arc::WifiConfiguration>?
cernekee@chromium.org changed reviewers: + cernekee@chromium.org
Hmm, I am unclear on why this CL is needed? Is there a specific use case / call pattern that we are trying to optimize? I would prefer to do the caching on the Android side, since several apps may query the network list every time there is a scan complete broadcast intent.
On 2016/03/01 17:56:22, Kevin Cernekee wrote: > Hmm, I am unclear on why this CL is needed? > > Is there a specific use case / call pattern that we are trying to optimize? > > I would prefer to do the caching on the Android side, since several apps may > query the network list every time there is a scan complete broadcast intent. Moved the caching stuff to the Instance. And removed any error being set in this function.
On 2016/03/01 16:22:31, Luis Héctor Chávez wrote: > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > File components/arc/net/arc_net_host_impl.cc (right): > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > components/arc/net/arc_net_host_impl.cc:61: const GetNetworksCallback& callback) > { > nit: add DCHECK(thread_checker_.CalledOnValidThread()); here and all the rest of > the methods (except where you expect a different thread to be used. In that case > comment what thread you expect. > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > components/arc/net/arc_net_host_impl.cc:66: if (configured_only && visible_only) > { > Hmmm this is problematic. Can you substitute these two flags with a three-value > enum? > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > components/arc/net/arc_net_host_impl.cc:75: visible_networks_lock_.Acquire(); > This is the UI thred, so this is not allowed. > > How about you get rid of this lock and then if ScanCompleted arrives on a > different thread than expected, post a task to the main thread? > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > File components/arc/net/arc_net_host_impl.h (right): > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > components/arc/net/arc_net_host_impl.h:50: void ScanCompleted(const > chromeos::DeviceState* /*unused*/) override; > Can you comment about what thread this is expected to be called as? > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > components/arc/net/arc_net_host_impl.h:71: > mojo::Array<arc::WifiConfigurationPtr> cached_visible_networks_; > I don't love the idea of a move-only collection being used as a cache and then > cloned every time you want to send it over. Given that you still need to do > copying, would it make sense to make it a std::vector<arc::WifiConfiguration>? All these should be addressed as the caching logic is now moved to the Instance.
On 2016/03/02 03:08:03, abhishekbh wrote: > On 2016/03/01 16:22:31, Luis Héctor Chávez wrote: > > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > > File components/arc/net/arc_net_host_impl.cc (right): > > > > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > > components/arc/net/arc_net_host_impl.cc:61: const GetNetworksCallback& > callback) > > { > > nit: add DCHECK(thread_checker_.CalledOnValidThread()); here and all the rest > of > > the methods (except where you expect a different thread to be used. In that > case > > comment what thread you expect. > > > > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > > components/arc/net/arc_net_host_impl.cc:66: if (configured_only && > visible_only) > > { > > Hmmm this is problematic. Can you substitute these two flags with a > three-value > > enum? > > This comment was not addressed. > > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > > components/arc/net/arc_net_host_impl.cc:75: visible_networks_lock_.Acquire(); > > This is the UI thred, so this is not allowed. > > > > How about you get rid of this lock and then if ScanCompleted arrives on a > > different thread than expected, post a task to the main thread? > > > > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > > File components/arc/net/arc_net_host_impl.h (right): > > > > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > > components/arc/net/arc_net_host_impl.h:50: void ScanCompleted(const > > chromeos::DeviceState* /*unused*/) override; > > Can you comment about what thread this is expected to be called as? > > > > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > > components/arc/net/arc_net_host_impl.h:71: > > mojo::Array<arc::WifiConfigurationPtr> cached_visible_networks_; > > I don't love the idea of a move-only collection being used as a cache and then > > cloned every time you want to send it over. Given that you still need to do > > copying, would it make sense to make it a std::vector<arc::WifiConfiguration>? > > All these should be addressed as the caching logic is now moved to the Instance.
PTAL https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... components/arc/net/arc_net_host_impl.cc:66: if (configured_only && visible_only) { On 2016/03/01 16:22:31, Luis Héctor Chávez wrote: > Hmmm this is problematic. Can you substitute these two flags with a three-value > enum? I don't think that gets us anything for the following reasons - 1. With the enum I will have something like REQUEST_CONFIGURED_ONLY, REQUEST_VISIBLE_ONLY, REQUEST_INVALID. Essentially it's just an int being passed over which I would still have to check and I"ll end up doing the same thing. 2. This really isn't a true public interface. It is being used by the Instance, so for all intents and purposes this is "internal" to the system. And any gross problems in malformed args should ideally be caught during development and fixed on the Instance side since we know what we are sending down. This simple check should suffice for this case.
On 2016/03/02 19:16:31, abhishekbh wrote: > PTAL > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > File components/arc/net/arc_net_host_impl.cc (right): > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > components/arc/net/arc_net_host_impl.cc:66: if (configured_only && visible_only) > { > On 2016/03/01 16:22:31, Luis Héctor Chávez wrote: > > Hmmm this is problematic. Can you substitute these two flags with a > three-value > > enum? Err.. I meant *two*-value enum, not three. Sorry for any confusion. > > I don't think that gets us anything for the following reasons - > 1. With the enum I will have something like REQUEST_CONFIGURED_ONLY, > REQUEST_VISIBLE_ONLY, REQUEST_INVALID. Essentially it's just an int being passed > over which I would still have to check and I"ll end up doing the same thing. > > 2. This really isn't a true public interface. It is being used by the Instance, > so for all intents and purposes this is "internal" to the system. And any gross > problems in malformed args should ideally be caught during development and fixed > on the Instance side since we know what we are sending down. This simple check > should suffice for this case. Well, this gets us the benefit of not being to express an illegal state that has to be validated :) (assuming you don't define REQUEST_INVALID as being a valid state).
On 2016/03/02 19:18:59, Luis Héctor Chávez wrote: > On 2016/03/02 19:16:31, abhishekbh wrote: > > PTAL > > > > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > > File components/arc/net/arc_net_host_impl.cc (right): > > > > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > > components/arc/net/arc_net_host_impl.cc:66: if (configured_only && > visible_only) > > { > > On 2016/03/01 16:22:31, Luis Héctor Chávez wrote: > > > Hmmm this is problematic. Can you substitute these two flags with a > > three-value > > > enum? > > Err.. I meant *two*-value enum, not three. Sorry for any confusion. > > > > > I don't think that gets us anything for the following reasons - > > 1. With the enum I will have something like REQUEST_CONFIGURED_ONLY, > > REQUEST_VISIBLE_ONLY, REQUEST_INVALID. Essentially it's just an int being > passed > > over which I would still have to check and I"ll end up doing the same thing. > > > > 2. This really isn't a true public interface. It is being used by the > Instance, > > so for all intents and purposes this is "internal" to the system. And any > gross > > problems in malformed args should ideally be caught during development and > fixed > > on the Instance side since we know what we are sending down. This simple check > > should suffice for this case. > > Well, this gets us the benefit of not being to express an illegal state that has > to be validated :) (assuming you don't define REQUEST_INVALID as being a valid > state). In general you want n flags if they're orthogonal and all 2^n options are valid. These aren't.
Can we either update the summary to describe the new purpose of this CL, or drop the CL? Thanks.
Description was changed from ========== ARC: Implement a cache of visible WiFi networks. This change caches the list of visible WiFi networks whenever the Instance requests visible networks. Subsequent requests for visible networks are serviced from this cache. This cache is invalidated when a scan completed event is received by the Host. This change also overrides the callback for the ArcBridgeService closing. BUG=590963 BUG=b/26496701 ========== to ========== ARC: Remove error status setting in GetNetworks. This change removes the setting of the status code in GetNetworks. Its redundant, instead in error scenarios an empty list is given back. BUG=590963 BUG=b/26496701 ==========
On 2016/03/02 19:24:40, Luis Héctor Chávez wrote: > On 2016/03/02 19:18:59, Luis Héctor Chávez wrote: > > On 2016/03/02 19:16:31, abhishekbh wrote: > > > PTAL > > > > > > > > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > > > File components/arc/net/arc_net_host_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/1751793002/diff/1/components/arc/net/arc_net_... > > > components/arc/net/arc_net_host_impl.cc:66: if (configured_only && > > visible_only) > > > { > > > On 2016/03/01 16:22:31, Luis Héctor Chávez wrote: > > > > Hmmm this is problematic. Can you substitute these two flags with a > > > three-value > > > > enum? > > > > Err.. I meant *two*-value enum, not three. Sorry for any confusion. > > > > > > > > I don't think that gets us anything for the following reasons - > > > 1. With the enum I will have something like REQUEST_CONFIGURED_ONLY, > > > REQUEST_VISIBLE_ONLY, REQUEST_INVALID. Essentially it's just an int being > > passed > > > over which I would still have to check and I"ll end up doing the same thing. > > > > > > 2. This really isn't a true public interface. It is being used by the > > Instance, > > > so for all intents and purposes this is "internal" to the system. And any > > gross > > > problems in malformed args should ideally be caught during development and > > fixed > > > on the Instance side since we know what we are sending down. This simple > check > > > should suffice for this case. > > > > Well, this gets us the benefit of not being to express an illegal state that > has > > to be validated :) (assuming you don't define REQUEST_INVALID as being a valid > > state). > > In general you want n flags if they're orthogonal and all 2^n options are valid. > These aren't. Done.
On 2016/03/02 19:27:38, Kevin Cernekee wrote: > Can we either update the summary to describe the new purpose of this CL, or drop > the CL? > > Thanks. Done.
PTAL.
LGTM. Please update the description to reflect the API changes.
Description was changed from ========== ARC: Remove error status setting in GetNetworks. This change removes the setting of the status code in GetNetworks. Its redundant, instead in error scenarios an empty list is given back. BUG=590963 BUG=b/26496701 ========== to ========== ARC: Remove error status setting in GetNetworks. This change removes the setting of the status code in GetNetworks. Its redundant, instead in error scenarios an empty list is given back. This change also updated GetNetworks to take in an enum instead of a boolean flag. The old version is deprecated moving forward but supported for existing older versions. BUG=590963 BUG=b/26496701 ==========
lgtm https://codereview.chromium.org/1751793002/diff/40001/components/arc/net/arc_... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/1751793002/diff/40001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:84: } It might be slightly shorter to just write: bool configured_only = type == GetNetworksRequestType::CONFIGURED_ONLY; bool visible_only = type == GetNetworksRequestType::VISIBLE_ONLY;
lgtm
The CQ bit was checked by abhishekbh@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1751793002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1751793002/40001
Message was sent while issue was closed.
Description was changed from ========== ARC: Remove error status setting in GetNetworks. This change removes the setting of the status code in GetNetworks. Its redundant, instead in error scenarios an empty list is given back. This change also updated GetNetworks to take in an enum instead of a boolean flag. The old version is deprecated moving forward but supported for existing older versions. BUG=590963 BUG=b/26496701 ========== to ========== ARC: Remove error status setting in GetNetworks. This change removes the setting of the status code in GetNetworks. Its redundant, instead in error scenarios an empty list is given back. This change also updated GetNetworks to take in an enum instead of a boolean flag. The old version is deprecated moving forward but supported for existing older versions. BUG=590963 BUG=b/26496701 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== ARC: Remove error status setting in GetNetworks. This change removes the setting of the status code in GetNetworks. Its redundant, instead in error scenarios an empty list is given back. This change also updated GetNetworks to take in an enum instead of a boolean flag. The old version is deprecated moving forward but supported for existing older versions. BUG=590963 BUG=b/26496701 ========== to ========== ARC: Remove error status setting in GetNetworks. This change removes the setting of the status code in GetNetworks. Its redundant, instead in error scenarios an empty list is given back. This change also updated GetNetworks to take in an enum instead of a boolean flag. The old version is deprecated moving forward but supported for existing older versions. BUG=590963 BUG=b/26496701 Committed: https://crrev.com/96838748969e79cc41c364dbe386f3754741e1bb Cr-Commit-Position: refs/heads/master@{#379117} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/96838748969e79cc41c364dbe386f3754741e1bb Cr-Commit-Position: refs/heads/master@{#379117} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
