Merged Tether and cellular network types in System Tray.

chrome/browser/chromeos/tether/tether_service.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/tether/tether_service.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
@@ skipping 159 matching lines @@
                                       chromeos::NetworkStateHandler::
                                           TechnologyState::TECHNOLOGY_ENABLED) {
     is_enabled = true;
   } else {
     is_enabled = was_pref_enabled;
   }
 
   if (is_enabled != was_pref_enabled) {
     profile_->GetPrefs()->SetBoolean(prefs::kInstantTetheringEnabled,
                                      is_enabled);
-    UpdateTetherTechnologyState();
   }
+  UpdateTetherTechnologyState();
 }
 
 void TetherService::OnPrefsChanged() {
   UpdateTetherTechnologyState();
 }
 
 bool TetherService::HasSyncedTetherHosts() const {
   return !cryptauth_service_->GetCryptAuthDeviceManager()
               ->GetTetherHosts()
               .empty();
@@ skipping 15 matching lines @@
 
 chromeos::NetworkStateHandler::TechnologyState
 TetherService::GetTetherTechnologyState() {
   if (shut_down_ || suspended_ || session_manager_client_->IsScreenLocked() ||
       !HasSyncedTetherHosts()) {
     return chromeos::NetworkStateHandler::TechnologyState::
         TECHNOLOGY_UNAVAILABLE;
   } else if (!IsAllowedByPolicy()) {
     return chromeos::NetworkStateHandler::TechnologyState::
         TECHNOLOGY_PROHIBITED;
-  } else if (!IsBluetoothAvailable()) {
+  } else if (!IsBluetoothAvailable() || IsCellularAvailableButNotEnabled()) {

[khorimoto, 2017/05/30 22:35:16]

nit: Please add an explanation for why tether is left uninitialized when
cellular is available but not enabled.

[lesliewatkins, 2017/05/31 00:43:27]

Done.

[Kyle Horimoto, 2017/05/31 20:13:59]

Not actually done.

[lesliewatkins, 2017/05/31 22:06:28]

Done.

[Kyle Horimoto, 2017/05/31 22:18:48]

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.

[lesliewatkins, 2017/06/05 19:41:42]

Done.

     // TODO (hansberry): This unfortunately results in a weird UI state for

[khorimoto, 2017/05/30 22:35:16]

nit: Does this description still apply the same way now that we changed the if()
condition? Can you modify it accordingly?

[lesliewatkins, 2017/05/31 00:43:27]

Done.

[Kyle Horimoto, 2017/05/31 20:13:59]

Please explicitly refer to the code in question:

// TODO(hansberry): When !IsBluetoothAvailable(), the UI can get stuck in a
weird state...

[lesliewatkins, 2017/05/31 22:06:28]

Done.

[Kyle Horimoto, 2017/05/31 22:18:48]

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.

[lesliewatkins, 2017/06/05 19:41:42]

Done.

     // Settings where the toggle is clickable but immediately becomes disabled
     // after enabling it. Possible solution: grey out the toggle and tell the
     // user to turn Bluetooth on?
     return chromeos::NetworkStateHandler::TechnologyState::
         TECHNOLOGY_UNINITIALIZED;
   } else if (!IsEnabledbyPreference()) {
     return chromeos::NetworkStateHandler::TechnologyState::TECHNOLOGY_AVAILABLE;
   }
 
   return chromeos::NetworkStateHandler::TechnologyState::TECHNOLOGY_ENABLED;
 }
 
 void TetherService::OnBluetoothAdapterFetched(
     scoped_refptr<device::BluetoothAdapter> adapter) {
   adapter_ = adapter;
   adapter_->AddObserver(this);
   UpdateTetherTechnologyState();
 }
 
 bool TetherService::IsBluetoothAvailable() const {
   return adapter_.get() && adapter_->IsPresent() && adapter_->IsPowered();
 }
 
+bool TetherService::IsCellularAvailableButNotEnabled() const {
+  return (network_state_handler_->IsTechnologyAvailable(
+              chromeos::NetworkTypePattern::Cellular()) &&
+          !network_state_handler_->IsTechnologyEnabled(
+              chromeos::NetworkTypePattern::Cellular()));
+}
+
 bool TetherService::IsAllowedByPolicy() const {
   return profile_->GetPrefs()->GetBoolean(prefs::kInstantTetheringAllowed);
 }
 
 bool TetherService::IsEnabledbyPreference() const {
   return profile_->GetPrefs()->GetBoolean(prefs::kInstantTetheringEnabled);
 }