Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2883283004:
Merged Tether and cellular network types in System Tray. (Closed)
|
Created:
3 years, 7 months ago by lesliewatkins Modified:
3 years, 6 months ago CC:
chromium-reviews, kalyank, sadrul, Jeremy Klein, James Hawkins Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMerged Tether and cellular network types in System Tray.
BUG=672263
Review-Url: https://codereview.chromium.org/2883283004
Cr-Commit-Position: refs/heads/master@{#478135}
Committed: https://chromium.googlesource.com/chromium/src/+/275810438db9694d4030c6b6390a0abe20618646
Patch Set 1 #
Total comments: 4
Patch Set 2 : Merged Tether and cellular network types in System Tray. #
Total comments: 24
Patch Set 3 : hansberry@ comments #
Total comments: 9
Patch Set 4 : hansberry@ comments #
Total comments: 31
Patch Set 5 : Rebased off of 2913103002 #Patch Set 6 : khorimoto@ and stevenjb@ comments #Patch Set 7 : khorimoto@ comments #Patch Set 8 : Merged Tether and cellular network types in System Tray. #Patch Set 9 : Merged Tether and cellular network types in System Tray. #Patch Set 10 : khorimoto@ and stevenjb@ comments #
Total comments: 2
Patch Set 11 : khorimoto@ comments #Patch Set 12 : Merged Tether and cellular network types in System Tray. #
Total comments: 7
Patch Set 13 : khorimoto@ comments #
Total comments: 4
Patch Set 14 : fixed failing TetherServiceTest #
Messages
Total messages: 41 (11 generated)
lesliewatkins@chromium.org changed reviewers: + hansberry@chromium.org, stevenjb@chromium.org
Please attach our tracking bug to this CL: crbug.com/672263 https://codereview.chromium.org/2883283004/diff/1/ash/system/network/network_... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2883283004/diff/1/ash/system/network/network_... ash/system/network/network_list.cc:224: handler->SetTechnologyEnabled(NetworkTypePattern::Tether(), is_on, This works fine in the case that the device has no cellular modem -- the first SetTechnologyEnabled call will be a no-op and the toggle UI will behave fine. However, I'm concerned about the case of the device having a cellular modem. After glancing at the mocks, it looks like only the toggle specific to Tether inside Settings should specifically enable/disable Tether. When the device does have a cellular modem, this toggle should only enable/disable the broad cellular settings section, and leave alone the Tether enabled/disabled user preference (sorry if I'm being unclear, please let me know if I need to reword this). I think the best way to account for both cases is to first check if a cellular modem exists (NetworkStateHandler::IsTechnologyAvailable) and then: 1. If no cellular modem exists, only set the Tether technology as enabled/disabled. 2. If a cellular modem exists, only set the cellular technology as enabled/disabled. We will now need to listen for cellular technology changes. Leslie, to accommodate #2, in TetherService::DeviceListChanged, please also listen for a change to the cellular device enabled/disabled event, as it already does for the Tether device. IMO, the Tether technology state should be 'TECHNOLOGY_UNINITIALIZED' when cellular is disabled. Make that change in this CL; it should be small. https://codereview.chromium.org/2883283004/diff/1/ash/system/network/network_... ash/system/network/network_list.cc:534: handler->IsTechnologyEnabled(NetworkTypePattern::Tether()), Given my comment above, please change this to: handler->IsTechnologyEnabled(NetworkTypePattern::Cellular()) || (!handler->IsTechnologyAvailable(NetworkTypePattern::Cellular()) && handler->IsTechnologyEnabled(NetworkTypePattern::Tether())) This accounts for the case of not having a cellular modem. https://codereview.chromium.org/2883283004/diff/1/ash/system/network/network_... ash/system/network/network_list.cc:554: new_guids->insert(new_cellular_guids->begin(), new_cellular_guids->end()); Why did you move this line? (previously below 'index += new_cellular_guids->size();')
https://codereview.chromium.org/2883283004/diff/1/ash/system/network/network_... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2883283004/diff/1/ash/system/network/network_... ash/system/network/network_list.cc:554: new_guids->insert(new_cellular_guids->begin(), new_cellular_guids->end()); On 2017/05/17 20:46:18, Ryan Hansberry wrote: > Why did you move this line? (previously below 'index += > new_cellular_guids->size();') An artifact of intermediate changes. It doesn't affect the functionality, but I moved it back.
Ping: Please attach our tracking bug to this CL: crbug.com/672263 https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:222: if (handler->IsTechnologyAvailable(NetworkTypePattern::Cellular())) { Awesome :). Please add a comment explaining this branch. https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:537: (!handler->IsTechnologyAvailable(NetworkTypePattern::Cellular()) && This is correct but might be confusing in the future. Can you please create a local is_enabled variable that is set before this in a similar branching way that you have in OnToggleToggled? e.g.: is_enabled = false if (handler->IsTechnologyAvailable(NetworkTypePattern::Cellular())) { ... } else { ... } https://codereview.chromium.org/2883283004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/tether/tether_service_unittest.cc (right): https://codereview.chromium.org/2883283004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service_unittest.cc:277: base::RunLoop().RunUntilIdle(); Is this necessary? https://codereview.chromium.org/2883283004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service_unittest.cc:336: network_state_handler()->SetTechnologyEnabled( Please break out these repeated SetTechnologyEnabled and RunLoop calls into a helper method. https://codereview.chromium.org/2883283004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service_unittest.cc:339: base::RunLoop().RunUntilIdle(); Are these RunLoop calls after calling on NetworkStateHandler necessary? https://codereview.chromium.org/2883283004/diff/20001/chromeos/dbus/fake_shil... File chromeos/dbus/fake_shill_manager_client.cc (right): https://codereview.chromium.org/2883283004/diff/20001/chromeos/dbus/fake_shil... chromeos/dbus/fake_shill_manager_client.cc:952: Please remove.
https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... File ash/system/network/network_info.h (right): https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... ash/system/network/network_info.h:22: enum class Type { UNKNOWN, WIFI, CELLULAR }; Let's change 'CELLULAR' to 'MOBILE' to reflect that it represents both Cellular and Tether. https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:222: if (handler->IsTechnologyAvailable(NetworkTypePattern::Cellular())) { On 2017/05/23 01:14:29, Ryan Hansberry wrote: > Awesome :). Please add a comment explaining this branch. Yes please. Specifically we should explain that when Cellular is available, we do not support Tether here. https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:449: network->Matches(NetworkTypePattern::Tether())) { So, we discussed possible changes to NetworkTypePattern before. Given the current direction I think we should go ahead and create NetworkTypePattern::Mobile to include Cellular and Tether. I think that would make this code slightly more clear. https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:537: (!handler->IsTechnologyAvailable(NetworkTypePattern::Cellular()) && On 2017/05/23 01:14:29, Ryan Hansberry wrote: > This is correct but might be confusing in the future. Can you please create a > local is_enabled variable that is set before this in a similar branching way > that you have in OnToggleToggled? e.g.: > > is_enabled = false > if (handler->IsTechnologyAvailable(NetworkTypePattern::Cellular())) { > ... > } else { > ... > } Or add an anonymous helper function: IsMobileTechnologyEnabled(). https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:724: pattern.Equals(NetworkTypePattern::Tether())) This could use Pattern::Mobile and then we won't need to add {} :) https://codereview.chromium.org/2883283004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/tether/tether_service_unittest.cc (right): https://codereview.chromium.org/2883283004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service_unittest.cc:277: base::RunLoop().RunUntilIdle(); On 2017/05/23 01:14:29, Ryan Hansberry wrote: > Is this necessary? Maybe add a comment. The NetworkHandler code is largely asyncronous, so if CreateTetherService calls into it at all to set state, we may need to run the message loop until all asynchronous calls have been competed. It might be better to just do this in CreateTetherService(). https://codereview.chromium.org/2883283004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service_unittest.cc:339: base::RunLoop().RunUntilIdle(); On 2017/05/23 01:14:29, Ryan Hansberry wrote: > Are these RunLoop calls after calling on NetworkStateHandler necessary? Se comment above. A SetTechnologyEnabled() wrapper that calls RunUntilIdle() would make sense to me.
https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:222: if (handler->IsTechnologyAvailable(NetworkTypePattern::Cellular())) { On 2017/05/23 01:14:29, Ryan Hansberry wrote: > Awesome :). Please add a comment explaining this branch. Done. https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:537: (!handler->IsTechnologyAvailable(NetworkTypePattern::Cellular()) && On 2017/05/23 01:14:29, Ryan Hansberry wrote: > This is correct but might be confusing in the future. Can you please create a > local is_enabled variable that is set before this in a similar branching way > that you have in OnToggleToggled? e.g.: > > is_enabled = false > if (handler->IsTechnologyAvailable(NetworkTypePattern::Cellular())) { > ... > } else { > ... > } I'm not sure what you mean by this, as there is no is_enabled variable in onToggleToggled, but I broke out is_enabled into a separate variable for better readability. Also, the updates to TetherService make this logic a little simpler, so hopefully it's less confusing now. https://codereview.chromium.org/2883283004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/tether/tether_service_unittest.cc (right): https://codereview.chromium.org/2883283004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service_unittest.cc:277: base::RunLoop().RunUntilIdle(); On 2017/05/23 01:14:29, Ryan Hansberry wrote: > Is this necessary? This one isn't-- it got added by accident! https://codereview.chromium.org/2883283004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service_unittest.cc:336: network_state_handler()->SetTechnologyEnabled( On 2017/05/23 01:14:29, Ryan Hansberry wrote: > Please break out these repeated SetTechnologyEnabled and RunLoop calls into a > helper method. Done. https://codereview.chromium.org/2883283004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service_unittest.cc:339: base::RunLoop().RunUntilIdle(); On 2017/05/23 01:14:29, Ryan Hansberry wrote: > Are these RunLoop calls after calling on NetworkStateHandler necessary? Yes, these are, because the call is still asynchronous. See https://cs.chromium.org/chromium/src/chromeos/network/network_state_handler.h... https://codereview.chromium.org/2883283004/diff/20001/chromeos/dbus/fake_shil... File chromeos/dbus/fake_shill_manager_client.cc (right): https://codereview.chromium.org/2883283004/diff/20001/chromeos/dbus/fake_shil... chromeos/dbus/fake_shill_manager_client.cc:952: On 2017/05/23 01:14:29, Ryan Hansberry wrote: > Please remove. Done.
Description was changed from ========== Merged Tether and cellular network types in System Tray. BUG= ========== to ========== Merged Tether and cellular network types in System Tray. BUG=672263 ==========
https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... File ash/system/network/network_info.h (right): https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... ash/system/network/network_info.h:22: enum class Type { UNKNOWN, WIFI, CELLULAR }; On 2017/05/23 22:11:07, stevenjb wrote: > Let's change 'CELLULAR' to 'MOBILE' to reflect that it represents both Cellular > and Tether. Ping. https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:449: network->Matches(NetworkTypePattern::Tether())) { On 2017/05/23 22:11:07, stevenjb wrote: > So, we discussed possible changes to NetworkTypePattern before. Given the > current direction I think we should go ahead and create > NetworkTypePattern::Mobile to include Cellular and Tether. I think that would > make this code slightly more clear. Ping. https://codereview.chromium.org/2883283004/diff/40001/ash/system/network/netw... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2883283004/diff/40001/ash/system/network/netw... ash/system/network/network_list.cc:194: // When cellular network technology is available on a device, Tethering is nit: Tether, not Tethering https://codereview.chromium.org/2883283004/diff/40001/ash/system/network/netw... ash/system/network/network_list.cc:195: // treated as a subset of Cellular technology. Otherwise, Tethering has to same nit here https://codereview.chromium.org/2883283004/diff/40001/ash/system/network/netw... ash/system/network/network_list.cc:510: handler->IsTechnologyEnabled(NetworkTypePattern::Cellular()) || This is unfortunately now incorrect (it was previously correct :( ). Based on my previous comment, is_enabled needs to be equal to handler->IsTechnologyEnabled(NetworkTypePattern::Cellular()) || (!handler->IsTechnologyAvailable(NetworkTypePattern::Cellular()) && handler->IsTechnologyEnabled(NetworkTypePattern::Tether())) My other previous comment about breaking this up was intended as something like (sorry that it was confusing): bool is_enabled = false; if (handler->IsTechnologyEnabled(NetworkTypePattern::Cellular()) is_enabled = true; else is_enabled = handler->IsTechnologyEnabled(NetworkTypePattern::Tether(); https://codereview.chromium.org/2883283004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/tether/tether_service.cc (right): https://codereview.chromium.org/2883283004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service.cc:181: UpdateTetherTechnologyState(); This is incorrect. Given our offline discussion earlier, I assume this was mistakenly uploaded -- you should expand the above if statement to check if cellular technology was changed.
https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... File ash/system/network/network_info.h (right): https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... ash/system/network/network_info.h:22: enum class Type { UNKNOWN, WIFI, CELLULAR }; On 2017/05/27 00:16:22, Ryan Hansberry wrote: > On 2017/05/23 22:11:07, stevenjb wrote: > > Let's change 'CELLULAR' to 'MOBILE' to reflect that it represents both > Cellular > > and Tether. > > Ping. Done. https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:449: network->Matches(NetworkTypePattern::Tether())) { On 2017/05/27 00:16:22, Ryan Hansberry wrote: > On 2017/05/23 22:11:07, stevenjb wrote: > > So, we discussed possible changes to NetworkTypePattern before. Given the > > current direction I think we should go ahead and create > > NetworkTypePattern::Mobile to include Cellular and Tether. I think that would > > make this code slightly more clear. > > Ping. Done. https://codereview.chromium.org/2883283004/diff/20001/ash/system/network/netw... ash/system/network/network_list.cc:724: pattern.Equals(NetworkTypePattern::Tether())) On 2017/05/23 22:11:07, stevenjb wrote: > This could use Pattern::Mobile and then we won't need to add {} :) Done. https://codereview.chromium.org/2883283004/diff/40001/ash/system/network/netw... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2883283004/diff/40001/ash/system/network/netw... ash/system/network/network_list.cc:194: // When cellular network technology is available on a device, Tethering is On 2017/05/27 00:16:22, Ryan Hansberry wrote: > nit: Tether, not Tethering Done. https://codereview.chromium.org/2883283004/diff/40001/ash/system/network/netw... ash/system/network/network_list.cc:195: // treated as a subset of Cellular technology. Otherwise, Tethering has to On 2017/05/27 00:16:22, Ryan Hansberry wrote: > same nit here Done. https://codereview.chromium.org/2883283004/diff/40001/ash/system/network/netw... ash/system/network/network_list.cc:510: handler->IsTechnologyEnabled(NetworkTypePattern::Cellular()) || On 2017/05/27 00:16:22, Ryan Hansberry wrote: > This is unfortunately now incorrect (it was previously correct :( ). > > Based on my previous comment, is_enabled needs to be equal to > > handler->IsTechnologyEnabled(NetworkTypePattern::Cellular()) || > (!handler->IsTechnologyAvailable(NetworkTypePattern::Cellular()) && > handler->IsTechnologyEnabled(NetworkTypePattern::Tether())) > > My other previous comment about breaking this up was intended as something like > (sorry that it was confusing): > > bool is_enabled = false; > if (handler->IsTechnologyEnabled(NetworkTypePattern::Cellular()) > is_enabled = true; > else > is_enabled = handler->IsTechnologyEnabled(NetworkTypePattern::Tether(); > I think it is correct, actually. Your if-statement is logically identical to the way the code is currently written. https://codereview.chromium.org/2883283004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/tether/tether_service.cc (right): https://codereview.chromium.org/2883283004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service.cc:181: UpdateTetherTechnologyState(); On 2017/05/27 00:16:22, Ryan Hansberry wrote: > This is incorrect. Given our offline discussion earlier, I assume this was > mistakenly uploaded -- you should expand the above if statement to check if > cellular technology was changed. Let's continue this discussion offline. It seems to work fine as written, but maybe I'm missing something.
lgtm :) https://codereview.chromium.org/2883283004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/tether/tether_service.cc (right): https://codereview.chromium.org/2883283004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service.cc:181: UpdateTetherTechnologyState(); On 2017/05/30 18:29:51, lesliewatkins wrote: > On 2017/05/27 00:16:22, Ryan Hansberry wrote: > > This is incorrect. Given our offline discussion earlier, I assume this was > > mistakenly uploaded -- you should expand the above if statement to check if > > cellular technology was changed. > > Let's continue this discussion offline. It seems to work fine as written, but > maybe I'm missing something. Oops, misread this. Looks good.
lesliewatkins@chromium.org changed reviewers: + khorimoto@chromium.org
khorimoto@google.com changed reviewers: + khorimoto@google.com
https://codereview.chromium.org/2883283004/diff/60001/ash/system/network/netw... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2883283004/diff/60001/ash/system/network/netw... ash/system/network/network_list.cc:181: class CellularHeaderRowView : public NetworkListView::SectionHeaderRowView { Isn't this called the Mobile section now? If so, you should update this class name accordingly. https://codereview.chromium.org/2883283004/diff/60001/ash/system/network/netw... ash/system/network/network_list.cc:194: // When cellular network technology is available on a device, Tether is This comment is a bit vague. Can you explain it more thoroughly? Something like: The mobile section contains both Cellular and Tether networks, though one or both of these subtypes may be unavailable. If the toggle has been changed and Cellular networks are available, set the enabled value accordingly. However, if Cellular networks are unavailable when the toggle's value is changed, the toggle should change the enabled value of Tether networks instead. https://codereview.chromium.org/2883283004/diff/60001/ash/system/network/netw... ash/system/network/network_list.cc:201: handler->SetTechnologyEnabled(NetworkTypePattern::Tether(), is_on, Above this line, you should DCHECK(handler->IsTechnologyAvailable(NetworkTypePattern::Tether())). https://codereview.chromium.org/2883283004/diff/60001/ash/system/network/netw... ash/system/network/network_list.cc:528: // TODO (hansberry): Audit existing usage of NonVirtual and consider changing We are using Mobile instead of NonVirtual now, so you can remove this TODO. https://codereview.chromium.org/2883283004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/tether/tether_service.cc (right): https://codereview.chromium.org/2883283004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service.cc:217: } else if (!IsBluetoothAvailable() || IsCellularAvailableButNotEnabled()) { nit: Please add an explanation for why tether is left uninitialized when cellular is available but not enabled. https://codereview.chromium.org/2883283004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service.cc:218: // TODO (hansberry): This unfortunately results in a weird UI state for nit: Does this description still apply the same way now that we changed the if() condition? Can you modify it accordingly? https://codereview.chromium.org/2883283004/diff/60001/chromeos/network/networ... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/2883283004/diff/60001/chromeos/network/networ... chromeos/network/network_state_handler.cc:427: IsTechnologyEnabled(NetworkTypePattern::Tether())) { I don't think this CL is the right place for this change. Theoretically, if tether is not enabled, we shouldn't have any networks in tether_network_list_ anyway. If we do want to make this change, we should also add a change which removes all tether networks from the list when the Tether DeviceState is changed from "enabled" to another value. stevenjb@, thoughts? :) https://codereview.chromium.org/2883283004/diff/60001/chromeos/network/networ... File chromeos/network/network_type_pattern.cc (right): https://codereview.chromium.org/2883283004/diff/60001/chromeos/network/networ... chromeos/network/network_type_pattern.cc:73: kNetworkTypeTether); Update unit test to reflect this change. Also, this type of change with an associated unit test change should usually be broken out into its own CL so that individual CLs can be more focused/granular.
https://codereview.chromium.org/2883283004/diff/60001/chromeos/network/networ... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/2883283004/diff/60001/chromeos/network/networ... chromeos/network/network_state_handler.cc:427: IsTechnologyEnabled(NetworkTypePattern::Tether())) { On 2017/05/30 22:35:16, khorimoto wrote: > I don't think this CL is the right place for this change. Theoretically, if > tether is not enabled, we shouldn't have any networks in tether_network_list_ > anyway. If we do want to make this change, we should also add a change which > removes all tether networks from the list when the Tether DeviceState is changed > from "enabled" to another value. > > stevenjb@, thoughts? :) I think changing the behavior in this CL is OK, but we shouldn't check IsTechnologyEnabled() here, that's an implementation detail of GetTetherNetworkList(), which should always return an empty result of Tether is disabled. https://codereview.chromium.org/2883283004/diff/60001/chromeos/network/networ... File chromeos/network/network_type_pattern.cc (right): https://codereview.chromium.org/2883283004/diff/60001/chromeos/network/networ... chromeos/network/network_type_pattern.cc:73: kNetworkTypeTether); On 2017/05/30 22:35:16, khorimoto wrote: > Update unit test to reflect this change. Also, this type of change with an > associated unit test change should usually be broken out into its own CL so that > individual CLs can be more focused/granular. +1, it would be good to make this a separate change, and to make sure we audit and call out any other uses of NetworkTypePattern::Mobile() in that CL.
https://codereview.chromium.org/2883283004/diff/60001/ash/system/network/netw... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2883283004/diff/60001/ash/system/network/netw... ash/system/network/network_list.cc:181: class CellularHeaderRowView : public NetworkListView::SectionHeaderRowView { On 2017/05/30 22:35:16, khorimoto wrote: > Isn't this called the Mobile section now? If so, you should update this class > name accordingly. Done. https://codereview.chromium.org/2883283004/diff/60001/ash/system/network/netw... ash/system/network/network_list.cc:194: // When cellular network technology is available on a device, Tether is On 2017/05/30 22:35:15, khorimoto wrote: > This comment is a bit vague. Can you explain it more thoroughly? Something like: > > The mobile section contains both Cellular and Tether networks, though one or > both of these subtypes may be unavailable. If the toggle has been changed and > Cellular networks are available, set the enabled value accordingly. However, if > Cellular networks are unavailable when the toggle's value is changed, the toggle > should change the enabled value of Tether networks instead. Done. https://codereview.chromium.org/2883283004/diff/60001/ash/system/network/netw... ash/system/network/network_list.cc:201: handler->SetTechnologyEnabled(NetworkTypePattern::Tether(), is_on, On 2017/05/30 22:35:16, khorimoto wrote: > Above this line, you should > DCHECK(handler->IsTechnologyAvailable(NetworkTypePattern::Tether())). Done. https://codereview.chromium.org/2883283004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/tether/tether_service.cc (right): https://codereview.chromium.org/2883283004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service.cc:217: } else if (!IsBluetoothAvailable() || IsCellularAvailableButNotEnabled()) { On 2017/05/30 22:35:16, khorimoto wrote: > nit: Please add an explanation for why tether is left uninitialized when > cellular is available but not enabled. Done. https://codereview.chromium.org/2883283004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service.cc:218: // TODO (hansberry): This unfortunately results in a weird UI state for On 2017/05/30 22:35:16, khorimoto wrote: > nit: Does this description still apply the same way now that we changed the if() > condition? Can you modify it accordingly? Done. https://codereview.chromium.org/2883283004/diff/60001/chromeos/network/networ... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/2883283004/diff/60001/chromeos/network/networ... chromeos/network/network_state_handler.cc:427: IsTechnologyEnabled(NetworkTypePattern::Tether())) { On 2017/05/30 22:35:16, khorimoto wrote: > I don't think this CL is the right place for this change. Theoretically, if > tether is not enabled, we shouldn't have any networks in tether_network_list_ > anyway. If we do want to make this change, we should also add a change which > removes all tether networks from the list when the Tether DeviceState is changed > from "enabled" to another value. > > stevenjb@, thoughts? :) If this change isn't implemented, then the Tether network list appears in the system tray even when Tether/Mobile is disabled. https://codereview.chromium.org/2883283004/diff/60001/chromeos/network/networ... File chromeos/network/network_type_pattern.cc (right): https://codereview.chromium.org/2883283004/diff/60001/chromeos/network/networ... chromeos/network/network_type_pattern.cc:73: kNetworkTypeTether); On 2017/05/30 22:35:16, khorimoto wrote: > Update unit test to reflect this change. Also, this type of change with an > associated unit test change should usually be broken out into its own CL so that > individual CLs can be more focused/granular. Okay, this is now its own CL-- 2913103002
https://codereview.chromium.org/2883283004/diff/60001/chromeos/network/networ... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/2883283004/diff/60001/chromeos/network/networ... chromeos/network/network_state_handler.cc:427: IsTechnologyEnabled(NetworkTypePattern::Tether())) { On 2017/05/30 23:23:14, stevenjb wrote: > On 2017/05/30 22:35:16, khorimoto wrote: > > I don't think this CL is the right place for this change. Theoretically, if > > tether is not enabled, we shouldn't have any networks in tether_network_list_ > > anyway. If we do want to make this change, we should also add a change which > > removes all tether networks from the list when the Tether DeviceState is > changed > > from "enabled" to another value. > > > > stevenjb@, thoughts? :) > > I think changing the behavior in this CL is OK, but we shouldn't check > IsTechnologyEnabled() here, that's an implementation detail of > GetTetherNetworkList(), which should always return an empty result of Tether is > disabled. Done.
https://codereview.chromium.org/2883283004/diff/60001/ash/system/network/netw... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2883283004/diff/60001/ash/system/network/netw... ash/system/network/network_list.cc:528: // TODO (hansberry): Audit existing usage of NonVirtual and consider changing On 2017/05/30 22:35:16, khorimoto wrote: > We are using Mobile instead of NonVirtual now, so you can remove this TODO. Done! Also closed the associated bug.
It doesn't look like your changes are uploaded properly. Can you please upload the correct changes?
On 2017/05/31 18:29:00, lesliewatkins wrote: Still hasn't been updated properly :/
Trying to fix the fallout from a rebase that went awry...
https://codereview.chromium.org/2883283004/diff/60001/ash/system/network/netw... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2883283004/diff/60001/ash/system/network/netw... ash/system/network/network_list.cc:194: // When cellular network technology is available on a device, Tether is On 2017/05/31 00:43:27, lesliewatkins wrote: > On 2017/05/30 22:35:15, khorimoto wrote: > > This comment is a bit vague. Can you explain it more thoroughly? Something > like: > > > > The mobile section contains both Cellular and Tether networks, though one or > > both of these subtypes may be unavailable. If the toggle has been changed and > > Cellular networks are available, set the enabled value accordingly. However, > if > > Cellular networks are unavailable when the toggle's value is changed, the > toggle > > should change the enabled value of Tether networks instead. > > Done. Not actually done. https://codereview.chromium.org/2883283004/diff/60001/ash/system/network/netw... ash/system/network/network_list.cc:201: handler->SetTechnologyEnabled(NetworkTypePattern::Tether(), is_on, On 2017/05/31 00:43:27, lesliewatkins wrote: > On 2017/05/30 22:35:16, khorimoto wrote: > > Above this line, you should > > DCHECK(handler->IsTechnologyAvailable(NetworkTypePattern::Tether())). > > Done. Not actually done. https://codereview.chromium.org/2883283004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/tether/tether_service.cc (right): https://codereview.chromium.org/2883283004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service.cc:217: } else if (!IsBluetoothAvailable() || IsCellularAvailableButNotEnabled()) { On 2017/05/31 00:43:27, lesliewatkins wrote: > On 2017/05/30 22:35:16, khorimoto wrote: > > nit: Please add an explanation for why tether is left uninitialized when > > cellular is available but not enabled. > > Done. Not actually done. https://codereview.chromium.org/2883283004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service.cc:218: // TODO (hansberry): This unfortunately results in a weird UI state for On 2017/05/31 00:43:27, lesliewatkins wrote: > On 2017/05/30 22:35:16, khorimoto wrote: > > nit: Does this description still apply the same way now that we changed the > if() > > condition? Can you modify it accordingly? > > Done. Please explicitly refer to the code in question: // TODO(hansberry): When !IsBluetoothAvailable(), the UI can get stuck in a weird state... https://codereview.chromium.org/2883283004/diff/180001/chromeos/network/netwo... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/2883283004/diff/180001/chromeos/network/netwo... chromeos/network/network_state_handler.cc:465: if (!IsTechnologyEnabled(NetworkTypePattern::Tether())) nit: Move the "int count" line below this early return.
https://codereview.chromium.org/2883283004/diff/60001/ash/system/network/netw... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2883283004/diff/60001/ash/system/network/netw... ash/system/network/network_list.cc:194: // When cellular network technology is available on a device, Tether is On 2017/05/31 20:13:59, Kyle Horimoto wrote: > On 2017/05/31 00:43:27, lesliewatkins wrote: > > On 2017/05/30 22:35:15, khorimoto wrote: > > > This comment is a bit vague. Can you explain it more thoroughly? Something > > like: > > > > > > The mobile section contains both Cellular and Tether networks, though one or > > > both of these subtypes may be unavailable. If the toggle has been changed > and > > > Cellular networks are available, set the enabled value accordingly. However, > > if > > > Cellular networks are unavailable when the toggle's value is changed, the > > toggle > > > should change the enabled value of Tether networks instead. > > > > Done. > > Not actually done. Done. https://codereview.chromium.org/2883283004/diff/60001/ash/system/network/netw... ash/system/network/network_list.cc:201: handler->SetTechnologyEnabled(NetworkTypePattern::Tether(), is_on, On 2017/05/31 20:13:58, Kyle Horimoto wrote: > On 2017/05/31 00:43:27, lesliewatkins wrote: > > On 2017/05/30 22:35:16, khorimoto wrote: > > > Above this line, you should > > > DCHECK(handler->IsTechnologyAvailable(NetworkTypePattern::Tether())). > > > > Done. > > Not actually done. Done. https://codereview.chromium.org/2883283004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/tether/tether_service.cc (right): https://codereview.chromium.org/2883283004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service.cc:217: } else if (!IsBluetoothAvailable() || IsCellularAvailableButNotEnabled()) { On 2017/05/31 20:13:59, Kyle Horimoto wrote: > On 2017/05/31 00:43:27, lesliewatkins wrote: > > On 2017/05/30 22:35:16, khorimoto wrote: > > > nit: Please add an explanation for why tether is left uninitialized when > > > cellular is available but not enabled. > > > > Done. > > Not actually done. Done. https://codereview.chromium.org/2883283004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service.cc:218: // TODO (hansberry): This unfortunately results in a weird UI state for On 2017/05/31 20:13:59, Kyle Horimoto wrote: > On 2017/05/31 00:43:27, lesliewatkins wrote: > > On 2017/05/30 22:35:16, khorimoto wrote: > > > nit: Does this description still apply the same way now that we changed the > > if() > > > condition? Can you modify it accordingly? > > > > Done. > > Please explicitly refer to the code in question: > > // TODO(hansberry): When !IsBluetoothAvailable(), the UI can get stuck in a > weird state... Done. https://codereview.chromium.org/2883283004/diff/180001/chromeos/network/netwo... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/2883283004/diff/180001/chromeos/network/netwo... chromeos/network/network_state_handler.cc:465: if (!IsTechnologyEnabled(NetworkTypePattern::Tether())) On 2017/05/31 20:13:59, Kyle Horimoto wrote: > nit: Move the "int count" line below this early return. Done.
lgtm https://codereview.chromium.org/2883283004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/tether/tether_service.cc (right): https://codereview.chromium.org/2883283004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service.cc:217: } else if (!IsBluetoothAvailable() || IsCellularAvailableButNotEnabled()) { On 2017/05/31 22:06:28, lesliewatkins wrote: > On 2017/05/31 20:13:59, Kyle Horimoto wrote: > > On 2017/05/31 00:43:27, lesliewatkins wrote: > > > On 2017/05/30 22:35:16, khorimoto wrote: > > > > nit: Please add an explanation for why tether is left uninitialized when > > > > cellular is available but not enabled. > > > > > > Done. > > > > Not actually done. > > Done. Can you move the comment into the if() block? It's currently above the code, which makes it seem like it applies to the previous if() block. https://codereview.chromium.org/2883283004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service.cc:218: // TODO (hansberry): This unfortunately results in a weird UI state for On 2017/05/31 22:06:28, lesliewatkins wrote: > On 2017/05/31 20:13:59, Kyle Horimoto wrote: > > On 2017/05/31 00:43:27, lesliewatkins wrote: > > > On 2017/05/30 22:35:16, khorimoto wrote: > > > > nit: Does this description still apply the same way now that we changed > the > > > if() > > > > condition? Can you modify it accordingly? > > > > > > Done. > > > > Please explicitly refer to the code in question: > > > > // TODO(hansberry): When !IsBluetoothAvailable(), the UI can get stuck in a > > weird state... > > Done. nit: Please change the wording so that the "When !IsBluetoothAvailable()" is at the beginning. Without this change, the comment is confusing because it appears to imply that the weird UI state will always occur at this point in the code.
https://codereview.chromium.org/2883283004/diff/220001/ash/system/network/net... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2883283004/diff/220001/ash/system/network/net... ash/system/network/network_list.cc:297: no_cellular_networks_view_(nullptr), These "cellular" views need to be renamed appropriately. https://codereview.chromium.org/2883283004/diff/220001/ash/system/network/net... ash/system/network/network_list.cc:391: if (network->Matches(NetworkTypePattern::Cellular())) Should be Mobile, right? https://codereview.chromium.org/2883283004/diff/220001/ash/system/network/net... ash/system/network/network_list.cc:507: // First add high-priority networks (not Wi-Fi nor cellular). mobile Also, while you're here, change "not" to "neither". https://codereview.chromium.org/2883283004/diff/220001/ash/system/network/net... ash/system/network/network_list.cc:523: // Cellular initializing. stevenjb@: Should this "initializing" section be changed/updated?
https://codereview.chromium.org/2883283004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/tether/tether_service.cc (right): https://codereview.chromium.org/2883283004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service.cc:217: } else if (!IsBluetoothAvailable() || IsCellularAvailableButNotEnabled()) { On 2017/05/31 22:18:48, Kyle Horimoto wrote: > On 2017/05/31 22:06:28, lesliewatkins wrote: > > On 2017/05/31 20:13:59, Kyle Horimoto wrote: > > > On 2017/05/31 00:43:27, lesliewatkins wrote: > > > > On 2017/05/30 22:35:16, khorimoto wrote: > > > > > nit: Please add an explanation for why tether is left uninitialized when > > > > > cellular is available but not enabled. > > > > > > > > Done. > > > > > > Not actually done. > > > > Done. > > Can you move the comment into the if() block? It's currently above the code, > which makes it seem like it applies to the previous if() block. Done. https://codereview.chromium.org/2883283004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/tether/tether_service.cc:218: // TODO (hansberry): This unfortunately results in a weird UI state for On 2017/05/31 22:18:48, Kyle Horimoto wrote: > On 2017/05/31 22:06:28, lesliewatkins wrote: > > On 2017/05/31 20:13:59, Kyle Horimoto wrote: > > > On 2017/05/31 00:43:27, lesliewatkins wrote: > > > > On 2017/05/30 22:35:16, khorimoto wrote: > > > > > nit: Does this description still apply the same way now that we changed > > the > > > > if() > > > > > condition? Can you modify it accordingly? > > > > > > > > Done. > > > > > > Please explicitly refer to the code in question: > > > > > > // TODO(hansberry): When !IsBluetoothAvailable(), the UI can get stuck in a > > > weird state... > > > > Done. > > nit: Please change the wording so that the "When !IsBluetoothAvailable()" is at > the beginning. Without this change, the comment is confusing because it appears > to imply that the weird UI state will always occur at this point in the code. Done. https://codereview.chromium.org/2883283004/diff/220001/ash/system/network/net... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2883283004/diff/220001/ash/system/network/net... ash/system/network/network_list.cc:297: no_cellular_networks_view_(nullptr), On 2017/06/05 17:49:30, Kyle Horimoto wrote: > These "cellular" views need to be renamed appropriately. Done. https://codereview.chromium.org/2883283004/diff/220001/ash/system/network/net... ash/system/network/network_list.cc:391: if (network->Matches(NetworkTypePattern::Cellular())) On 2017/06/05 17:49:30, Kyle Horimoto wrote: > Should be Mobile, right? I'm not sure. As written, all it will do is prioritize Cellular networks over Tether or WiMAX. stevenjb@? https://codereview.chromium.org/2883283004/diff/220001/ash/system/network/net... ash/system/network/network_list.cc:507: // First add high-priority networks (not Wi-Fi nor cellular). On 2017/06/05 17:49:30, Kyle Horimoto wrote: > mobile > > Also, while you're here, change "not" to "neither". Done.
Apologies for the late feedback. Let me know if you have any questions about my suggestions. https://codereview.chromium.org/2883283004/diff/240001/ash/system/network/net... File ash/system/network/network_list.cc (right): https://codereview.chromium.org/2883283004/diff/240001/ash/system/network/net... ash/system/network/network_list.cc:201: // Tether. Rather than putting this subtle logic in the UI code, lets move it to NetworkStateHandler::SetTechnlologyAvailable/Enabled and modify those to support NEtwrokTypePattern::Mobile. i.e. this code will just be: handler->SetTechnologyEnabled(NetworkTypePattern::Mobile(), is_on, chromeos::network_handler::ErrorCallback()); And the logic below would be in SetTechnologyEnabled() when the pattern is Mobile. https://codereview.chromium.org/2883283004/diff/240001/ash/system/network/net... ash/system/network/network_list.cc:513: handler->IsTechnologyAvailable(NetworkTypePattern::Tether())) { We should also make IsTechnologyAvailable work correctly for Mobile(). https://codereview.chromium.org/2883283004/diff/240001/ash/system/network/net... ash/system/network/network_list.cc:516: handler->IsTechnologyEnabled(NetworkTypePattern::Tether()); And IsTechnologyEnabled. https://codereview.chromium.org/2883283004/diff/240001/ash/system/network/net... ash/system/network/network_list.cc:518: index = UpdateSectionHeaderRow(NetworkTypePattern::Cellular(), is_enabled, NetworkTypePattern::Mobile() should work here.
On 2017/06/07 23:10:30, stevenjb wrote: > Apologies for the late feedback. Let me know if you have any questions about my > suggestions. > > https://codereview.chromium.org/2883283004/diff/240001/ash/system/network/net... > File ash/system/network/network_list.cc (right): > > https://codereview.chromium.org/2883283004/diff/240001/ash/system/network/net... > ash/system/network/network_list.cc:201: // Tether. > Rather than putting this subtle logic in the UI code, lets move it to > NetworkStateHandler::SetTechnlologyAvailable/Enabled and modify those to support > NEtwrokTypePattern::Mobile. > > i.e. this code will just be: > > handler->SetTechnologyEnabled(NetworkTypePattern::Mobile(), is_on, > chromeos::network_handler::ErrorCallback()); > > And the logic below would be in SetTechnologyEnabled() when the pattern is > Mobile. > > https://codereview.chromium.org/2883283004/diff/240001/ash/system/network/net... > ash/system/network/network_list.cc:513: > handler->IsTechnologyAvailable(NetworkTypePattern::Tether())) { > We should also make IsTechnologyAvailable work correctly for Mobile(). > > https://codereview.chromium.org/2883283004/diff/240001/ash/system/network/net... > ash/system/network/network_list.cc:516: > handler->IsTechnologyEnabled(NetworkTypePattern::Tether()); > And IsTechnologyEnabled. > > https://codereview.chromium.org/2883283004/diff/240001/ash/system/network/net... > ash/system/network/network_list.cc:518: index = > UpdateSectionHeaderRow(NetworkTypePattern::Cellular(), is_enabled, > NetworkTypePattern::Mobile() should work here. Oh, so I just remembered that Mobile includes WiMAX. Sigh. I think that probably eventually we will want to lump WiMAX together with Cellular/Tether (in practice WiMAX and Cellular are mutually exclusive), but for now.... leaving the logic in the UI as-is seems fine. So, LGTM as-is.
The CQ bit was checked by lesliewatkins@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hansberry@chromium.org, khorimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2883283004/#ps240001 (title: "khorimoto@ comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
hansberry@, I believe its your test that I modified so it would pass. Mind taking a look for me?
The CQ bit was checked by lesliewatkins@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, khorimoto@chromium.org, hansberry@chromium.org Link to the patchset: https://codereview.chromium.org/2883283004/#ps260001 (title: "fixed failing TetherServiceTest")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1496966052569920,
"parent_rev": "48e5d0e5f63a3faa02b3cdd19c9d87c9967cfa4b", "commit_rev":
"275810438db9694d4030c6b6390a0abe20618646"}
Message was sent while issue was closed.
Description was changed from ========== Merged Tether and cellular network types in System Tray. BUG=672263 ========== to ========== Merged Tether and cellular network types in System Tray. BUG=672263 Review-Url: https://codereview.chromium.org/2883283004 Cr-Commit-Position: refs/heads/master@{#478135} Committed: https://chromium.googlesource.com/chromium/src/+/275810438db9694d4030c6b6390a... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/275810438db9694d4030c6b6390a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
