Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2819303002:
Changed wifi arcs to mobile bars for Tether network. (Closed)
|
Created:
3 years, 8 months ago by lesliewatkins Modified:
3 years, 7 months ago CC:
chromium-reviews, jlklein+watch-tether_chromium.org, sadrul, tengs+watch-tether_chromium.org, hansberry+watch-tether_chromium.org, jhawkins+watch-tether_chromium.org, oshima+watch_chromium.org, kalyank, lesliewatkins+watch-tether_chromium.org, khorimoto+watch-tether_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionChanged wifi arcs to mobile bars for Tether network.
Added ability to disassociate Tether and Wi-Fi networks, and now Wi-Fi and Tether networks are associated as soon as the network becomes connectable.
Adding stevenjb@ for network_icon.cc.
BUG=672263
Review-Url: https://codereview.chromium.org/2819303002
Cr-Commit-Position: refs/heads/master@{#469430}
Committed: https://chromium.googlesource.com/chromium/src/+/b4f862467b0dce4e75b337f28fab0b7a5019f6ba
Patch Set 1 #
Total comments: 34
Patch Set 2 : khorimoto@ comments #
Total comments: 66
Patch Set 3 : khorimoto@ and hansberry@ comments #
Total comments: 24
Patch Set 4 : khorimoto@ comments #
Total comments: 13
Patch Set 5 : khorimoto@ comments #
Total comments: 46
Patch Set 6 : khorimoto@ comments #
Total comments: 20
Patch Set 7 : khorimoto@ hansberry@ stevenjb@ comments #
Total comments: 4
Patch Set 8 : stevenjb@ comments #Patch Set 9 : Fixed small typo preventing unit tests from building. #
Total comments: 1
Messages
Total messages: 44 (13 generated)
Description was changed from ========== Changed wifi arcs to mobile bars for Tether network. BUG=672263 ========== to ========== Changed wifi arcs to mobile bars for Tether network. Adding stevenjb@ for network_icon.cc. BUG=672263 ==========
lesliewatkins@chromium.org changed reviewers: + hansberry@chromium.org, khorimoto@chromium.org, stevenjb@chromium.org
https://codereview.chromium.org/2819303002/diff/1/ash/system/network/network_... File ash/system/network/network_icon.cc (right): https://codereview.chromium.org/2819303002/diff/1/ash/system/network/network_... ash/system/network/network_icon.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Please add unit tests for the changes you made in this file. https://codereview.chromium.org/2819303002/diff/1/ash/system/network/network_... ash/system/network/network_icon.cc:433: if (network->type() == shill::kTypeWifi && !network->tether_guid().empty()) Add a comment explaining what this means. https://codereview.chromium.org/2819303002/diff/1/ash/system/network/network_... ash/system/network/network_icon.cc:773: return GetBasicImage(false, icon_type, network_type); nit: Add /* parameter_name */ after "false" so that readers know what false represents. https://codereview.chromium.org/2819303002/diff/1/ash/system/network/network_... ash/system/network/network_icon.cc:775: if (!network->tether_guid().empty()) { Also add an explanatory comment here. https://codereview.chromium.org/2819303002/diff/1/ash/system/network/network_... ash/system/network/network_icon.cc:776: network_type = chromeos::kTypeTether; You do this check in ImageTypeForNetwork as well. Can you combine these to share code? https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... File chromeos/components/tether/fake_wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/fake_wifi_hotspot_connector.cc:17: : WifiHotspotConnector(network_state_handler, nullptr, nullptr) {} nit: Please add /* parameter_name */ comment. https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... File chromeos/components/tether/tether_connector_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/tether_connector_unittest.cc:215: /* void VerifyTetherAndWifiNetworkAssociation( Remove this if it is unused. https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... File chromeos/components/tether/wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector.cc:51: InvokeWifiConnectionCallback(std::string()); Now that you make the call to AssociateTetherNetworkStateWithWifiNetwork() in this class, you need to un-associate the networks when a connection attempt was underway when another connection attempt begins. You'll need to add a new function to NetworkStateHandler to do this. You also need to add a test for this situation. https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... File chromeos/components/tether/wifi_hotspot_connector.h (right): https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector.h:64: ActiveHost* active_host_; nit: Place injected classes in the same order as they appear in the constructor, and initialize them in the same order in the .cc file as well. https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... File chromeos/components/tether/wifi_hotspot_connector_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:126: fake_active_host_ = base::WrapUnique(new FakeActiveHost()); nit: Use base::MakeUnique<FakeActiveHost>(). https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:128: network_state_handler()->AddTetherNetworkState(kDeviceId, ""); You should be passing a tether network GUID, not a device ID. I realize that currently, we use the same value, but this will be changing in the future. Please adjust the variable name accordingly. https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:130: fake_active_host_->SetActiveHostConnecting(kDeviceId, kDeviceId); Also pass the GUID here as well. Don't use the same variable (and make the device ID and tether GUID values different). https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:228: void VerifyTetherAndWifiNetworkDisassociation(const std::string& wifi_guid) { nit: Rename this to VerifyTetherAndWifiNetworkNotAssociated(). https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:232: EXPECT_NE(fake_active_host_->GetTetherNetworkGuid(), Here, the tether GUID should be "". Assert that the GUID is empty(), not just that it isn't equal to another value. https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:257: std::unique_ptr<FakeActiveHost> fake_active_host_; Move this up to the protected section underneath the other test doubles. (beneath test_network_connect_).
https://codereview.chromium.org/2819303002/diff/1/ash/system/network/network_... File ash/system/network/network_icon.cc (right): https://codereview.chromium.org/2819303002/diff/1/ash/system/network/network_... ash/system/network/network_icon.cc:433: if (network->type() == shill::kTypeWifi && !network->tether_guid().empty()) On 2017/04/17 19:50:04, Kyle Horimoto wrote: > Add a comment explaining what this means. Done. https://codereview.chromium.org/2819303002/diff/1/ash/system/network/network_... ash/system/network/network_icon.cc:773: return GetBasicImage(false, icon_type, network_type); On 2017/04/17 19:50:04, Kyle Horimoto wrote: > nit: Add /* parameter_name */ after "false" so that readers know what false > represents. Done. https://codereview.chromium.org/2819303002/diff/1/ash/system/network/network_... ash/system/network/network_icon.cc:775: if (!network->tether_guid().empty()) { On 2017/04/17 19:50:03, Kyle Horimoto wrote: > Also add an explanatory comment here. Done. https://codereview.chromium.org/2819303002/diff/1/ash/system/network/network_... ash/system/network/network_icon.cc:776: network_type = chromeos::kTypeTether; On 2017/04/17 19:50:03, Kyle Horimoto wrote: > You do this check in ImageTypeForNetwork as well. Can you combine these to share > code? Done. https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... File chromeos/components/tether/fake_wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/fake_wifi_hotspot_connector.cc:17: : WifiHotspotConnector(network_state_handler, nullptr, nullptr) {} On 2017/04/17 19:50:04, Kyle Horimoto wrote: > nit: Please add /* parameter_name */ comment. Done. https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... File chromeos/components/tether/tether_connector_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/tether_connector_unittest.cc:215: /* void VerifyTetherAndWifiNetworkAssociation( On 2017/04/17 19:50:04, Kyle Horimoto wrote: > Remove this if it is unused. Done. https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... File chromeos/components/tether/wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector.cc:51: InvokeWifiConnectionCallback(std::string()); On 2017/04/17 19:50:04, Kyle Horimoto wrote: > Now that you make the call to AssociateTetherNetworkStateWithWifiNetwork() in > this class, you need to un-associate the networks when a connection attempt was > underway when another connection attempt begins. You'll need to add a new > function to NetworkStateHandler to do this. > > You also need to add a test for this situation. I think this is done using NetworkStateHandler::RemoveTetherNetworkState https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... File chromeos/components/tether/wifi_hotspot_connector.h (right): https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector.h:64: ActiveHost* active_host_; On 2017/04/17 19:50:04, Kyle Horimoto wrote: > nit: Place injected classes in the same order as they appear in the constructor, > and initialize them in the same order in the .cc file as well. Done. Initializing them in a different order gives a compiler warning. https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... File chromeos/components/tether/wifi_hotspot_connector_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:126: fake_active_host_ = base::WrapUnique(new FakeActiveHost()); On 2017/04/17 19:50:04, Kyle Horimoto wrote: > nit: Use base::MakeUnique<FakeActiveHost>(). Done. https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:128: network_state_handler()->AddTetherNetworkState(kDeviceId, ""); On 2017/04/17 19:50:04, Kyle Horimoto wrote: > You should be passing a tether network GUID, not a device ID. I realize that > currently, we use the same value, but this will be changing in the future. > Please adjust the variable name accordingly. Done. https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:130: fake_active_host_->SetActiveHostConnecting(kDeviceId, kDeviceId); On 2017/04/17 19:50:04, Kyle Horimoto wrote: > Also pass the GUID here as well. Don't use the same variable (and make the > device ID and tether GUID values different). Done. https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:228: void VerifyTetherAndWifiNetworkDisassociation(const std::string& wifi_guid) { On 2017/04/17 19:50:04, Kyle Horimoto wrote: > nit: Rename this to VerifyTetherAndWifiNetworkNotAssociated(). Done. https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:232: EXPECT_NE(fake_active_host_->GetTetherNetworkGuid(), On 2017/04/17 19:50:04, Kyle Horimoto wrote: > Here, the tether GUID should be "". Assert that the GUID is empty(), not just > that it isn't equal to another value. Done. https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:232: EXPECT_NE(fake_active_host_->GetTetherNetworkGuid(), On 2017/04/17 19:50:04, Kyle Horimoto wrote: > Here, the tether GUID should be "". Assert that the GUID is empty(), not just > that it isn't equal to another value. Done. https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:257: std::unique_ptr<FakeActiveHost> fake_active_host_; On 2017/04/17 19:50:04, Kyle Horimoto wrote: > Move this up to the protected section underneath the other test doubles. > (beneath test_network_connect_). Done.
https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... File ash/system/network/network_icon.cc (right): https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon.cc:5: #include <iostream> Remove. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon.cc:439: std::string NetworkTypeForIcon(const NetworkState* network) { nit: How about GetEffectiveNetworkType()? This doesn't actually return the network type, it returns the effective network type for the purpose of an icon. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon.cc:618: std::cout << network_type << std::endl; Remove test logs. Also, FYI, the preferred logging approach is to include "base/logging.h" and use LOG(ERROR). https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... File ash/system/network/network_icon.h (right): https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon.h:1: Remove. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... File ash/system/network/network_icon_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:21: // This (somewhat indirectly) tests that the correct icons are being generated Move the comments to the individual tests. Someone might add new tests later which are unrelated to these comments. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:34: IconType icon_type = ICON_TYPE_TRAY; Move instance field declarations below functions, and declare each one on its own line. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:39: this->test::AshTestBase::SetUp(); You don't need "this->"; same in TearDown(). https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:48: void PrintImage(gfx::Image image, int i = 0) { Remove this function - it's unused. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:68: TEST_F(NetworkIconTest, NetworkNotVisible) { nit: Be more descriptive in your test names. How about CompareImagesByNetworkType_NotVisible, CompareImagesByNetworkType_Connecting, CompareImagesByNetworkType_Connected. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:69: std::unique_ptr<chromeos::NetworkState> tether_network = You initialize these fields in every test. Initialize them and set appropriate properties in SetUp() instead so you don't have to repeat them in each test. The only things you'll need to set in each particular test are visibility and connection state. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:70: base::MakeUnique<chromeos::NetworkState>("tether"); Also, the service path which you pass to the constructor doesn't need to change from test to test. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:90: EXPECT_FALSE(gfx::test::AreImagesEqual(tether_image, wifi_image)); In each test, after you set properties on the networks, you create an ImageSkia, create an Image with the ImageSkia, and assert that the Cellular/Tether are the same and that the Wi-Fi is different. Move all that to a helper function and call it from each test. This will make each individual test easier to follow. https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... File chromeos/components/tether/fake_wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/fake_wifi_hotspot_connector.cc:18: nullptr /* NetworkConnect */, Use the variable name, not the type name: nullptr /* network_connect */, etc. https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... File chromeos/components/tether/initializer.cc (right): https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/initializer.cc:186: wifi_hotspot_connector_ = base::MakeUnique<WifiHotspotConnector>( Revert changes in this file when you no longer need to inject ActiveHost. See my comments later in this review. https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... File chromeos/components/tether/tether_connector_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/tether_connector_unittest.cc:370: VerifyTetherAndWifiNetworkAssociation( You forgot to remove the association code from TetherConnector. You should no longer be verifying the association occurs in this test. https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... File chromeos/components/tether/wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:83: // If the network is now connectable, initiate a connection to it. Move this comment to before the ConnectToNetworkId() call below. Replace the comment with something that describes why we are associating it ASAP (to give the user visual feedback that the connection is in progress as early as we can). https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:84: std::string tether_guid = active_host_->GetTetherNetworkGuid(); I apologize for not noticing this earlier, but this isn't correct 100% of the time. If a new connection is started while another connection attempt is in progress and the ActiveHost has already been updated by this point, active_host_->GetTetherNetworkGuid() may be a different value from the GUID associated with the network we are trying to connect to. Instead of passing the ActiveHost to the constructor of this class, pass the tether network GUID as a parameter to the ConnectToWifiHotspot() function when you call it from TetherConnector. You can use ActiveHost::GetTetherNetworkGuid() to get the GUID at that point. In ConnectToWifiHotspot(), you can store the GUID like we store ssid_ and password_. Then, when you reach this point (where you are making the association), associate the stored GUID and wifi_guid_. https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... File chromeos/components/tether/wifi_hotspot_connector_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:132: kTetherNetworkGuid, "", /* name */ Add the comment before the comma: "" /* name */, Also, don't add all these spaces before your comment. Just fit as many arguments as you can on each line. You can run "git cl format ." in your src/ directory to do this formatting for you. https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:230: fake_active_host_->GetTetherNetworkGuid()); Note: To get the tether GUID once you change from the ActiveHost approach, you'll need to add WifiHotspotConnectorTest as a friend class of WifiHotspotConnector to access a private function. https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:235: void VerifyTetherAndWifiNetworkNotAssociated(const std::string& wifi_guid) { You aren't verifying anything about a tether network, so just rename this function VerifyWifiNetworkNotAssociated(). https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:294: VerifyTetherAndWifiNetworkNotAssociated(wifi_guid); Also verify that the network associated with other_wifi_service_path_ isn't associated. To get the GUID, you'll need to call: network_state_handler()->GetNetworkState(other_wifi_service_path_)->guid(). https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:425: Please add one additional test which tests this situation: 1) Connection requested to device A. 2) Network for device A becomes connectable. Association occurs and connection attempt is started. 3) Connection requested to device B. The callback for device A is invoked with "". 4) Connection to device A's hotspot completes, but it is ignored. 5) Network for device B becomes connectable and eventually connected. Success callback is invoked. https://codereview.chromium.org/2819303002/diff/20001/chromeos/network/manage... File chromeos/network/managed_state.h (right): https://codereview.chromium.org/2819303002/diff/20001/chromeos/network/manage... chromeos/network/managed_state.h:14: #include "base/gtest_prod_util.h" Remove.
https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... File chromeos/components/tether/wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector.cc:51: InvokeWifiConnectionCallback(std::string()); On 2017/04/27 00:33:53, lesliewatkins wrote: > On 2017/04/17 19:50:04, Kyle Horimoto wrote: > > Now that you make the call to AssociateTetherNetworkStateWithWifiNetwork() in > > this class, you need to un-associate the networks when a connection attempt > was > > underway when another connection attempt begins. You'll need to add a new > > function to NetworkStateHandler to do this. > > > > You also need to add a test for this situation. > > I think this is done using NetworkStateHandler::RemoveTetherNetworkState Nope - that function removes the network entirely. We don't want to do that; we just want to disassociate it from a Wi-Fi network. You still need to do that as part of this CL.
https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... File ash/system/network/network_icon.cc (right): https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon.cc:782: return GetBasicImage(false /* not connected */, icon_type, network_type); just 'connected' https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... File ash/system/network/network_icon_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:25: // The icon for a tether network should be the same as one for a cellular nit: please refer to Tether networks in comments as "Tether" as opposed to "tether". https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:161: } // namespace network_icon there should be a newline between namespace closings https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... File chromeos/components/tether/tether_connector_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/tether_connector_unittest.cc:361: // The active host should now be connected Comments should end with periods (and in general should be complete sentences but that's not an issue here). https://codereview.chromium.org/2819303002/diff/20001/chromeos/network/manage... File chromeos/network/managed_state.h (right): https://codereview.chromium.org/2819303002/diff/20001/chromeos/network/manage... chromeos/network/managed_state.h:21: } nit: close namespaces with comments. e.g. '} // namespace ash'
https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... File ash/system/network/network_icon.cc (right): https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon.cc:5: #include <iostream> On 2017/04/27 01:28:07, Kyle Horimoto wrote: > Remove. Done. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon.cc:439: std::string NetworkTypeForIcon(const NetworkState* network) { On 2017/04/27 01:28:07, Kyle Horimoto wrote: > nit: How about GetEffectiveNetworkType()? This doesn't actually return the > network type, it returns the effective network type for the purpose of an icon. Done. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon.cc:618: std::cout << network_type << std::endl; On 2017/04/27 01:28:07, Kyle Horimoto wrote: > Remove test logs. > > Also, FYI, the preferred logging approach is to include "base/logging.h" and use > LOG(ERROR). Done. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon.cc:782: return GetBasicImage(false /* not connected */, icon_type, network_type); On 2017/04/27 16:32:26, Ryan Hansberry wrote: > just 'connected' Changed to is_connected. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... File ash/system/network/network_icon.h (right): https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon.h:1: On 2017/04/27 01:28:07, Kyle Horimoto wrote: > Remove. Done. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... File ash/system/network/network_icon_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:21: // This (somewhat indirectly) tests that the correct icons are being generated On 2017/04/27 01:28:07, Kyle Horimoto wrote: > Move the comments to the individual tests. Someone might add new tests later > which are unrelated to these comments. Done. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:25: // The icon for a tether network should be the same as one for a cellular On 2017/04/27 16:32:27, Ryan Hansberry wrote: > nit: please refer to Tether networks in comments as "Tether" as opposed to > "tether". Done. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:34: IconType icon_type = ICON_TYPE_TRAY; On 2017/04/27 01:28:07, Kyle Horimoto wrote: > Move instance field declarations below functions, and declare each one on its > own line. Done. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:39: this->test::AshTestBase::SetUp(); On 2017/04/27 01:28:07, Kyle Horimoto wrote: > You don't need "this->"; same in TearDown(). Done. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:48: void PrintImage(gfx::Image image, int i = 0) { On 2017/04/27 01:28:07, Kyle Horimoto wrote: > Remove this function - it's unused. Done. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:68: TEST_F(NetworkIconTest, NetworkNotVisible) { On 2017/04/27 01:28:07, Kyle Horimoto wrote: > nit: Be more descriptive in your test names. How about > CompareImagesByNetworkType_NotVisible, CompareImagesByNetworkType_Connecting, > CompareImagesByNetworkType_Connected. Done. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:69: std::unique_ptr<chromeos::NetworkState> tether_network = On 2017/04/27 01:28:07, Kyle Horimoto wrote: > You initialize these fields in every test. Initialize them and set appropriate > properties in SetUp() instead so you don't have to repeat them in each test. The > only things you'll need to set in each particular test are visibility and > connection state. I actually need to give each of them a unique network path. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:70: base::MakeUnique<chromeos::NetworkState>("tether"); On 2017/04/27 01:28:07, Kyle Horimoto wrote: > Also, the service path which you pass to the constructor doesn't need to change > from test to test. It does, because the path is what's used in the IconMap to either look up or create a new icon. See https://cs.chromium.org/chromium/src/ash/system/network/network_icon.cc?type=... https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:90: EXPECT_FALSE(gfx::test::AreImagesEqual(tether_image, wifi_image)); On 2017/04/27 01:28:07, Kyle Horimoto wrote: > In each test, after you set properties on the networks, you create an ImageSkia, > create an Image with the ImageSkia, and assert that the Cellular/Tether are the > same and that the Wi-Fi is different. Move all that to a helper function and > call it from each test. This will make each individual test easier to follow. Done. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:161: } // namespace network_icon On 2017/04/27 16:32:26, Ryan Hansberry wrote: > there should be a newline between namespace closings Done. https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... File chromeos/components/tether/fake_wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/fake_wifi_hotspot_connector.cc:18: nullptr /* NetworkConnect */, On 2017/04/27 01:28:07, Kyle Horimoto wrote: > Use the variable name, not the type name: nullptr /* network_connect */, etc. Done. https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... File chromeos/components/tether/initializer.cc (right): https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/initializer.cc:186: wifi_hotspot_connector_ = base::MakeUnique<WifiHotspotConnector>( On 2017/04/27 01:28:07, Kyle Horimoto wrote: > Revert changes in this file when you no longer need to inject ActiveHost. See my > comments later in this review. Done. https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... File chromeos/components/tether/tether_connector_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/tether_connector_unittest.cc:361: // The active host should now be connected On 2017/04/27 16:32:27, Ryan Hansberry wrote: > Comments should end with periods (and in general should be complete sentences > but that's not an issue here). Done. https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/tether_connector_unittest.cc:370: VerifyTetherAndWifiNetworkAssociation( On 2017/04/27 01:28:07, Kyle Horimoto wrote: > You forgot to remove the association code from TetherConnector. You should no > longer be verifying the association occurs in this test. Done. https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... File chromeos/components/tether/wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:83: // If the network is now connectable, initiate a connection to it. On 2017/04/27 01:28:07, Kyle Horimoto wrote: > Move this comment to before the ConnectToNetworkId() call below. > > Replace the comment with something that describes why we are associating it ASAP > (to give the user visual feedback that the connection is in progress as early as > we can). Done. https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:84: std::string tether_guid = active_host_->GetTetherNetworkGuid(); On 2017/04/27 01:28:07, Kyle Horimoto wrote: > I apologize for not noticing this earlier, but this isn't correct 100% of the > time. If a new connection is started while another connection attempt is in > progress and the ActiveHost has already been updated by this point, > active_host_->GetTetherNetworkGuid() may be a different value from the GUID > associated with the network we are trying to connect to. > > Instead of passing the ActiveHost to the constructor of this class, pass the > tether network GUID as a parameter to the ConnectToWifiHotspot() function when > you call it from TetherConnector. You can use ActiveHost::GetTetherNetworkGuid() > to get the GUID at that point. > > In ConnectToWifiHotspot(), you can store the GUID like we store ssid_ and > password_. Then, when you reach this point (where you are making the > association), associate the stored GUID and wifi_guid_. Done. https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... File chromeos/components/tether/wifi_hotspot_connector_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:132: kTetherNetworkGuid, "", /* name */ On 2017/04/27 01:28:08, Kyle Horimoto wrote: > Add the comment before the comma: "" /* name */, > > Also, don't add all these spaces before your comment. Just fit as many arguments > as you can on each line. You can run "git cl format ." in your src/ directory to > do this formatting for you. Done. I believe 'git cl format' is the culprit for all those spaces, because I didn't add them. Maybe that will be fixed when I put the comment in front of the comma. https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:230: fake_active_host_->GetTetherNetworkGuid()); On 2017/04/27 01:28:08, Kyle Horimoto wrote: > Note: To get the tether GUID once you change from the ActiveHost approach, > you'll need to add WifiHotspotConnectorTest as a friend class of > WifiHotspotConnector to access a private function. I ended up added a second parameter to VerifyTetherAndWifiNetworkAssociated and just used the Tether GUID const defined in the test class. https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:235: void VerifyTetherAndWifiNetworkNotAssociated(const std::string& wifi_guid) { On 2017/04/27 01:28:08, Kyle Horimoto wrote: > You aren't verifying anything about a tether network, so just rename this > function VerifyWifiNetworkNotAssociated(). Done. https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:294: VerifyTetherAndWifiNetworkNotAssociated(wifi_guid); On 2017/04/27 01:28:08, Kyle Horimoto wrote: > Also verify that the network associated with other_wifi_service_path_ isn't > associated. To get the GUID, you'll need to call: > > network_state_handler()->GetNetworkState(other_wifi_service_path_)->guid(). Done. https://codereview.chromium.org/2819303002/diff/20001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:425: On 2017/04/27 01:28:08, Kyle Horimoto wrote: > Please add one additional test which tests this situation: > > 1) Connection requested to device A. > 2) Network for device A becomes connectable. Association occurs and connection > attempt is started. > 3) Connection requested to device B. The callback for device A is invoked with > "". > 4) Connection to device A's hotspot completes, but it is ignored. > 5) Network for device B becomes connectable and eventually connected. Success > callback is invoked. Done. https://codereview.chromium.org/2819303002/diff/20001/chromeos/network/manage... File chromeos/network/managed_state.h (right): https://codereview.chromium.org/2819303002/diff/20001/chromeos/network/manage... chromeos/network/managed_state.h:14: #include "base/gtest_prod_util.h" On 2017/04/27 01:28:08, Kyle Horimoto wrote: > Remove. Done. https://codereview.chromium.org/2819303002/diff/20001/chromeos/network/manage... chromeos/network/managed_state.h:21: } On 2017/04/27 16:32:27, Ryan Hansberry wrote: > nit: close namespaces with comments. e.g. '} // namespace ash' Even for the little namespaces where you can easily see the opening and closing braces together? That seems inconsistent with the other namespaces, even in this file.
Now with changes to the code!
https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... File chromeos/components/tether/wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector.cc:51: InvokeWifiConnectionCallback(std::string()); On 2017/04/27 01:30:07, Kyle Horimoto wrote: > On 2017/04/27 00:33:53, lesliewatkins wrote: > > On 2017/04/17 19:50:04, Kyle Horimoto wrote: > > > Now that you make the call to AssociateTetherNetworkStateWithWifiNetwork() > in > > > this class, you need to un-associate the networks when a connection attempt > > was > > > underway when another connection attempt begins. You'll need to add a new > > > function to NetworkStateHandler to do this. > > > > > > You also need to add a test for this situation. > > > > I think this is done using NetworkStateHandler::RemoveTetherNetworkState > > Nope - that function removes the network entirely. We don't want to do that; we > just want to disassociate it from a Wi-Fi network. You still need to do that as > part of this CL. This still hasn't been addressed. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... File ash/system/network/network_icon.cc (right): https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon.cc:439: std::string NetworkTypeForIcon(const NetworkState* network) { On 2017/04/28 21:30:42, lesliewatkins wrote: > On 2017/04/27 01:28:07, Kyle Horimoto wrote: > > nit: How about GetEffectiveNetworkType()? This doesn't actually return the > > network type, it returns the effective network type for the purpose of an > icon. > > Done. Update the comment as well. "Returns the effective network type for |network|. Here, "effective network" means..." https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon.cc:782: return GetBasicImage(false /* not connected */, icon_type, network_type); On 2017/04/28 21:30:42, lesliewatkins wrote: > On 2017/04/27 16:32:26, Ryan Hansberry wrote: > > just 'connected' > > Changed to is_connected. You're supposed to match the name of the parameter you're passing. Please change to "connected". https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... File ash/system/network/network_icon_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:69: std::unique_ptr<chromeos::NetworkState> tether_network = On 2017/04/28 21:30:42, lesliewatkins wrote: > On 2017/04/27 01:28:07, Kyle Horimoto wrote: > > You initialize these fields in every test. Initialize them and set appropriate > > properties in SetUp() instead so you don't have to repeat them in each test. > The > > only things you'll need to set in each particular test are visibility and > > connection state. > > I actually need to give each of them a unique network path. See my comment below regarding PurgeNetworkIconCache(). Please declare these as instance fields on the test class and initialize them in SetUp(). Also, please use strings which correspond to what would be passed in the real implementation. For example: base::MakeUnique<chromeos::NetworkState>("tetherNetworkGuid"); or base::MakeUnique<chromeos::NetworkState>("wifiServicePath"); or base::MakeUnique<chromeos::NetworkState>("cellularServicePath"); https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:70: base::MakeUnique<chromeos::NetworkState>("tether"); On 2017/04/28 21:30:42, lesliewatkins wrote: > On 2017/04/27 01:28:07, Kyle Horimoto wrote: > > Also, the service path which you pass to the constructor doesn't need to > change > > from test to test. > > It does, because the path is what's used in the IconMap to either look up or > create a new icon. See > https://cs.chromium.org/chromium/src/ash/system/network/network_icon.cc?type=... Call PurgeNetworkIconCache() in Teardown() to avoid this. https://codereview.chromium.org/2819303002/diff/40001/ash/system/network/netw... File ash/system/network/network_icon_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/40001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:71: // This (somewhat indirectly) tests that the correct icons are being generated If something is indirect/not obvious, explain it. https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... File chromeos/components/tether/fake_wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/fake_wifi_hotspot_connector.cc:31: const std::string& guid, tether_network_guid https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... File chromeos/components/tether/fake_wifi_hotspot_connector.h (right): https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/fake_wifi_hotspot_connector.h:34: const std::string& guid, Use tether_network_guid. We need to be specific, since both types of GUIDs are used by tether. https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... File chromeos/components/tether/wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:39: const std::string& guid, tether_network_guid https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:88: << " is connectable. Tether network ID: \"" << guid_ Add something to the log that says that the networks were associated successfully. https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:97: // If the network is now connectable, associate it with a Tether network nit: Newline before this. https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... File chromeos/components/tether/wifi_hotspot_connector.h (right): https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.h:43: const std::string& guid, tether_network_guid https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.h:70: std::string guid_; tether_network_guid https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.h:71: std::string wifi_guid_; nit: I know you didn't write this code, but let's change this name to wifi_network_guid_ for consistency. Same in the test file. https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... File chromeos/components/tether/wifi_hotspot_connector_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:130: kTetherNetworkGuid, "" /* name */, "" /* carrier */, nit: Pass a name like "tetherNetworkName1" instead of just an empty string. Passing an empty string makes it seem like we are purposely passing a special-cased value, when we really aren't. Same with carrier. https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:352: std::string(kSsid), "", kTetherNetworkGuid, nit: Add /* password */ after "". https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:462: // The original connection attempt should have gotten a "" response. Move this up to right after the second ConnectToWifiHotspot() call. In general, you want to make verifications the earliest that you can. Also, after you address my comment about disassociating the networks, verify that here as well.
https://codereview.chromium.org/2819303002/diff/1/ash/system/network/network_... File ash/system/network/network_icon.cc (right): https://codereview.chromium.org/2819303002/diff/1/ash/system/network/network_... ash/system/network/network_icon.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/04/17 19:50:04, Kyle Horimoto wrote: > Please add unit tests for the changes you made in this file. Done. https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... File chromeos/components/tether/wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/1/chromeos/components/tether/... chromeos/components/tether/wifi_hotspot_connector.cc:51: InvokeWifiConnectionCallback(std::string()); On 2017/04/28 22:07:28, Kyle Horimoto wrote: > On 2017/04/27 01:30:07, Kyle Horimoto wrote: > > On 2017/04/27 00:33:53, lesliewatkins wrote: > > > On 2017/04/17 19:50:04, Kyle Horimoto wrote: > > > > Now that you make the call to AssociateTetherNetworkStateWithWifiNetwork() > > in > > > > this class, you need to un-associate the networks when a connection > attempt > > > was > > > > underway when another connection attempt begins. You'll need to add a new > > > > function to NetworkStateHandler to do this. > > > > > > > > You also need to add a test for this situation. > > > > > > I think this is done using NetworkStateHandler::RemoveTetherNetworkState > > > > Nope - that function removes the network entirely. We don't want to do that; > we > > just want to disassociate it from a Wi-Fi network. You still need to do that > as > > part of this CL. > > This still hasn't been addressed. Done. https://codereview.chromium.org/2819303002/diff/40001/ash/system/network/netw... File ash/system/network/network_icon_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/40001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:71: // This (somewhat indirectly) tests that the correct icons are being generated On 2017/04/28 22:07:28, Kyle Horimoto wrote: > If something is indirect/not obvious, explain it. Done. https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... File chromeos/components/tether/fake_wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/fake_wifi_hotspot_connector.cc:31: const std::string& guid, On 2017/04/28 22:07:28, Kyle Horimoto wrote: > tether_network_guid Done. https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... File chromeos/components/tether/fake_wifi_hotspot_connector.h (right): https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/fake_wifi_hotspot_connector.h:34: const std::string& guid, On 2017/04/28 22:07:28, Kyle Horimoto wrote: > Use tether_network_guid. We need to be specific, since both types of GUIDs are > used by tether. Done. https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... File chromeos/components/tether/wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:39: const std::string& guid, On 2017/04/28 22:07:28, Kyle Horimoto wrote: > tether_network_guid Done. https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:88: << " is connectable. Tether network ID: \"" << guid_ On 2017/04/28 22:07:28, Kyle Horimoto wrote: > Add something to the log that says that the networks were associated > successfully. Done. https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:97: // If the network is now connectable, associate it with a Tether network On 2017/04/28 22:07:28, Kyle Horimoto wrote: > nit: Newline before this. Done. https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... File chromeos/components/tether/wifi_hotspot_connector.h (right): https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.h:43: const std::string& guid, On 2017/04/28 22:07:28, Kyle Horimoto wrote: > tether_network_guid Done. https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.h:70: std::string guid_; On 2017/04/28 22:07:28, Kyle Horimoto wrote: > tether_network_guid Done. https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.h:71: std::string wifi_guid_; On 2017/04/28 22:07:28, Kyle Horimoto wrote: > nit: I know you didn't write this code, but let's change this name to > wifi_network_guid_ for consistency. Same in the test file. Done. https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... File chromeos/components/tether/wifi_hotspot_connector_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:130: kTetherNetworkGuid, "" /* name */, "" /* carrier */, On 2017/04/28 22:07:29, Kyle Horimoto wrote: > nit: Pass a name like "tetherNetworkName1" instead of just an empty string. > Passing an empty string makes it seem like we are purposely passing a > special-cased value, when we really aren't. Same with carrier. Done. https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:352: std::string(kSsid), "", kTetherNetworkGuid, On 2017/04/28 22:07:29, Kyle Horimoto wrote: > nit: Add /* password */ after "". Done. https://codereview.chromium.org/2819303002/diff/40001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:462: // The original connection attempt should have gotten a "" response. On 2017/04/28 22:07:29, Kyle Horimoto wrote: > Move this up to right after the second ConnectToWifiHotspot() call. In general, > you want to make verifications the earliest that you can. > > Also, after you address my comment about disassociating the networks, verify > that here as well. Done.
https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... File ash/system/network/network_icon_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:69: std::unique_ptr<chromeos::NetworkState> tether_network = On 2017/04/28 22:07:28, Kyle Horimoto wrote: > On 2017/04/28 21:30:42, lesliewatkins wrote: > > On 2017/04/27 01:28:07, Kyle Horimoto wrote: > > > You initialize these fields in every test. Initialize them and set > appropriate > > > properties in SetUp() instead so you don't have to repeat them in each test. > > The > > > only things you'll need to set in each particular test are visibility and > > > connection state. > > > > I actually need to give each of them a unique network path. > > See my comment below regarding PurgeNetworkIconCache(). Please declare these as > instance fields on the test class and initialize them in SetUp(). > > Also, please use strings which correspond to what would be passed in the real > implementation. > > For example: > base::MakeUnique<chromeos::NetworkState>("tetherNetworkGuid"); > > or > > base::MakeUnique<chromeos::NetworkState>("wifiServicePath"); > > or > > base::MakeUnique<chromeos::NetworkState>("cellularServicePath"); Still not addressed. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:70: base::MakeUnique<chromeos::NetworkState>("tether"); On 2017/04/28 22:07:28, Kyle Horimoto wrote: > On 2017/04/28 21:30:42, lesliewatkins wrote: > > On 2017/04/27 01:28:07, Kyle Horimoto wrote: > > > Also, the service path which you pass to the constructor doesn't need to > > change > > > from test to test. > > > > It does, because the path is what's used in the IconMap to either look up or > > create a new icon. See > > > https://cs.chromium.org/chromium/src/ash/system/network/network_icon.cc?type=... > > Call PurgeNetworkIconCache() in Teardown() to avoid this. Still not addressed. https://codereview.chromium.org/2819303002/diff/60001/ash/system/network/netw... File ash/system/network/network_icon.cc (right): https://codereview.chromium.org/2819303002/diff/60001/ash/system/network/netw... ash/system/network/network_icon.cc:776: std::string network_type = GetEffectiveNetworkType(network); nit: const std::string https://codereview.chromium.org/2819303002/diff/60001/ash/system/network/netw... File ash/system/network/network_icon_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/60001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:21: class NetworkIconTest : public test::AshTestBase { You need to add one more type to your test: a Wi-Fi network with a tether GUID. So there should be 4 total types: tether, cellular, Wi-Fi without tether GUID, and Wi-Fi with tether GUID. https://codereview.chromium.org/2819303002/diff/60001/chromeos/components/tet... File chromeos/components/tether/wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/60001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:36: void WifiHotspotConnector::ConnectToWifiHotspot( You forgot to add the disassociation here when necessary. Also add a test for this. https://codereview.chromium.org/2819303002/diff/60001/chromeos/network/networ... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/2819303002/diff/60001/chromeos/network/networ... chromeos/network/network_state_handler.cc:578: ManagedStateList::iterator iter = tether_network_list_.begin(); Use this: for (const auto& tether_network : tether_network_list_) https://codereview.chromium.org/2819303002/diff/60001/chromeos/network/networ... File chromeos/network/network_state_handler.h (right): https://codereview.chromium.org/2819303002/diff/60001/chromeos/network/networ... chromeos/network/network_state_handler.h:232: // Dissociate the Tether network specified by |guid| with its associated Disassociate https://codereview.chromium.org/2819303002/diff/60001/chromeos/network/networ... chromeos/network/network_state_handler.h:233: // Wi-Fi network. Return the iterator for that Tether network so that it Just return a success bool. The client can call RemoveTetherNetworkState() if it chooses to. https://codereview.chromium.org/2819303002/diff/60001/chromeos/network/networ... File chromeos/network/network_state_handler_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/60001/chromeos/network/networ... chromeos/network/network_state_handler_unittest.cc:843: wifi_network = network_state_handler_->GetNetworkStateFromGuid(kWifiGuid1); Also verify the Tether network changed as well.
https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... File ash/system/network/network_icon_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:69: std::unique_ptr<chromeos::NetworkState> tether_network = On 2017/05/01 17:09:26, Kyle Horimoto wrote: > On 2017/04/28 22:07:28, Kyle Horimoto wrote: > > On 2017/04/28 21:30:42, lesliewatkins wrote: > > > On 2017/04/27 01:28:07, Kyle Horimoto wrote: > > > > You initialize these fields in every test. Initialize them and set > > appropriate > > > > properties in SetUp() instead so you don't have to repeat them in each > test. > > > The > > > > only things you'll need to set in each particular test are visibility and > > > > connection state. > > > > > > I actually need to give each of them a unique network path. > > > > See my comment below regarding PurgeNetworkIconCache(). Please declare these > as > > instance fields on the test class and initialize them in SetUp(). > > > > Also, please use strings which correspond to what would be passed in the real > > implementation. > > > > For example: > > base::MakeUnique<chromeos::NetworkState>("tetherNetworkGuid"); > > > > or > > > > base::MakeUnique<chromeos::NetworkState>("wifiServicePath"); > > > > or > > > > base::MakeUnique<chromeos::NetworkState>("cellularServicePath"); > > Still not addressed. Done. https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:69: std::unique_ptr<chromeos::NetworkState> tether_network = On 2017/04/28 22:07:28, Kyle Horimoto wrote: > On 2017/04/28 21:30:42, lesliewatkins wrote: > > On 2017/04/27 01:28:07, Kyle Horimoto wrote: > > > You initialize these fields in every test. Initialize them and set > appropriate > > > properties in SetUp() instead so you don't have to repeat them in each test. > > The > > > only things you'll need to set in each particular test are visibility and > > > connection state. > > > > I actually need to give each of them a unique network path. > > See my comment below regarding PurgeNetworkIconCache(). Please declare these as > instance fields on the test class and initialize them in SetUp(). > > Also, please use strings which correspond to what would be passed in the real > implementation. > > For example: > base::MakeUnique<chromeos::NetworkState>("tetherNetworkGuid"); > > or > > base::MakeUnique<chromeos::NetworkState>("wifiServicePath"); > > or > > base::MakeUnique<chromeos::NetworkState>("cellularServicePath"); I'm not sure what the path would be for the Tether network, but the string "tetherNetworkGuid" is already used in this test as a stand-in for the guid. Edit: I just now realized that that string is used in a different test, but the string should still probably reflect the fact that the constructor parameter is a path and not a guid. How about "tetherNetworkPath"? https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:70: base::MakeUnique<chromeos::NetworkState>("tether"); On 2017/05/01 17:09:26, Kyle Horimoto wrote: > On 2017/04/28 22:07:28, Kyle Horimoto wrote: > > On 2017/04/28 21:30:42, lesliewatkins wrote: > > > On 2017/04/27 01:28:07, Kyle Horimoto wrote: > > > > Also, the service path which you pass to the constructor doesn't need to > > > change > > > > from test to test. > > > > > > It does, because the path is what's used in the IconMap to either look up or > > > create a new icon. See > > > > > > https://cs.chromium.org/chromium/src/ash/system/network/network_icon.cc?type=... > > > > Call PurgeNetworkIconCache() in Teardown() to avoid this. > > Still not addressed. Done. https://codereview.chromium.org/2819303002/diff/60001/ash/system/network/netw... File ash/system/network/network_icon.cc (right): https://codereview.chromium.org/2819303002/diff/60001/ash/system/network/netw... ash/system/network/network_icon.cc:776: std::string network_type = GetEffectiveNetworkType(network); On 2017/05/01 17:09:26, Kyle Horimoto wrote: > nit: const std::string Done. https://codereview.chromium.org/2819303002/diff/60001/ash/system/network/netw... File ash/system/network/network_icon_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/60001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:21: class NetworkIconTest : public test::AshTestBase { On 2017/05/01 17:09:26, Kyle Horimoto wrote: > You need to add one more type to your test: a Wi-Fi network with a tether GUID. > So there should be 4 total types: tether, cellular, Wi-Fi without tether GUID, > and Wi-Fi with tether GUID. Done. https://codereview.chromium.org/2819303002/diff/60001/chromeos/network/networ... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/2819303002/diff/60001/chromeos/network/networ... chromeos/network/network_state_handler.cc:578: ManagedStateList::iterator iter = tether_network_list_.begin(); On 2017/05/01 17:09:26, Kyle Horimoto wrote: > Use this: > > for (const auto& tether_network : tether_network_list_) I just reverted back to the old RemoveTetherNetworkState function and used GetModifiableNetworkStateFromGuid-- that seemed to be the most sensible way to deal with this. https://codereview.chromium.org/2819303002/diff/60001/chromeos/network/networ... File chromeos/network/network_state_handler.h (right): https://codereview.chromium.org/2819303002/diff/60001/chromeos/network/networ... chromeos/network/network_state_handler.h:232: // Dissociate the Tether network specified by |guid| with its associated On 2017/05/01 17:09:27, Kyle Horimoto wrote: > Disassociate Done. https://codereview.chromium.org/2819303002/diff/60001/chromeos/network/networ... chromeos/network/network_state_handler.h:233: // Wi-Fi network. Return the iterator for that Tether network so that it On 2017/05/01 17:09:27, Kyle Horimoto wrote: > Just return a success bool. The client can call RemoveTetherNetworkState() if it > chooses to. Done. https://codereview.chromium.org/2819303002/diff/60001/chromeos/network/networ... File chromeos/network/network_state_handler_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/60001/chromeos/network/networ... chromeos/network/network_state_handler_unittest.cc:843: wifi_network = network_state_handler_->GetNetworkStateFromGuid(kWifiGuid1); On 2017/05/01 17:09:27, Kyle Horimoto wrote: > Also verify the Tether network changed as well. Done.
Almost there! https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... File ash/system/network/network_icon_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/20001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:69: std::unique_ptr<chromeos::NetworkState> tether_network = On 2017/05/03 01:23:23, lesliewatkins wrote: > On 2017/04/28 22:07:28, Kyle Horimoto wrote: > > On 2017/04/28 21:30:42, lesliewatkins wrote: > > > On 2017/04/27 01:28:07, Kyle Horimoto wrote: > > > > You initialize these fields in every test. Initialize them and set > > appropriate > > > > properties in SetUp() instead so you don't have to repeat them in each > test. > > > The > > > > only things you'll need to set in each particular test are visibility and > > > > connection state. > > > > > > I actually need to give each of them a unique network path. > > > > See my comment below regarding PurgeNetworkIconCache(). Please declare these > as > > instance fields on the test class and initialize them in SetUp(). > > > > Also, please use strings which correspond to what would be passed in the real > > implementation. > > > > For example: > > base::MakeUnique<chromeos::NetworkState>("tetherNetworkGuid"); > > > > or > > > > base::MakeUnique<chromeos::NetworkState>("wifiServicePath"); > > > > or > > > > base::MakeUnique<chromeos::NetworkState>("cellularServicePath"); > > I'm not sure what the path would be for the Tether network, but the string > "tetherNetworkGuid" is already used in this test as a stand-in for the guid. > > Edit: I just now realized that that string is used in a different test, but the > string should still probably reflect the fact that the constructor parameter is > a path and not a guid. > > How about "tetherNetworkPath"? Sure, that's fine. Really, we pass the GUID as the network path since we don't have a real Shill network path, but I suppose that's an implementation detail of NetworkStateHandler, so clients shouldn't be expected to know that. https://codereview.chromium.org/2819303002/diff/80001/ash/system/network/netw... File ash/system/network/network_icon_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/80001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:30: tether_network = nit: tether_network = ... tether_network->set_type() tether_network->set_tether_guid() wifi_network = ... wifi_network->set_type() etc. https://codereview.chromium.org/2819303002/diff/80001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:77: std::unique_ptr<chromeos::NetworkState> tether_network, wifi_network, Declare one per line. https://codereview.chromium.org/2819303002/diff/80001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:78: cellular_network, wifi_tether_network; nit: For people unfamiliar with how Tether networks work, it isn't clear what wifi_tether_image is. Please add a comment explaining what this corresponds to. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... File chromeos/components/tether/fake_wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/fake_wifi_hotspot_connector.cc:31: const std::string& tether_network_guid, You should also save tether_network_guid like we do with the other parameters. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... File chromeos/components/tether/initializer.cc (right): https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/initializer.cc:192: wifi_hotspot_connector_ = base::MakeUnique<WifiHotspotConnector>( Delete this. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... File chromeos/components/tether/tether_connector.cc (right): https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/tether_connector.cc:129: ssid_copy, password_copy, active_host_->GetTetherNetworkGuid(), Now that you pass this here, modify the tests to assert that you passed the expected GUID. You'll need to use the change to FakeWifiHotspotConnector I suggested above. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... File chromeos/components/tether/wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:48: // disassociate that network with the Tether network and nit: "disassociates with" is incorrect grammatically. Should be "disassociates from". Also fix this below in your logs. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:49: // call the callback, passing an empty string to signal that the connection nit: Some of this fits on the previous line. Try to fit as many characters per line as you can without going over the 80-char limit. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:55: PA_LOG(INFO) << "Wifi network with ID " << wifi_network_guid_ Just make this one sentence: Wifi network with ID <id> successfully disassociated from Tether network with ID <id>. No need to repeat the Wi-Fi network ID again. Same with the error case. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:118: // If the network is now connectable, associate it with a Tether network Move this comment to above the AssociateTetherNetworkStateWithWifiNetwork() call. It's unrelated to the ConnectToNetworkId() call. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... File chromeos/components/tether/wifi_hotspot_connector_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:237: // EXPECT_TRUE(network_state->tether_guid().empty()); Use this, delete the line below. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:409: VerifyNetworkNotAssociated(wifi_guid1); You can move this to just below the "EXPECT_FALSE(service_path2.empty());" call. In general, try to verify things just after they happened (or didn't happen, in this case). https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:451: // Disassocciate first network from Tether network. nit: The way you word this makes it seem like the lines of code beneath this are the ones diassociating. Say something like "The first Tether and Wi-Fi networks should no longer be associated." https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... chromeos/network/network_state_handler.cc:592: nit: Remove changes to functions you did not edit. This makes "git blame" a lot easier to understand. https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... chromeos/network/network_state_handler.cc:616: NET_LOG(ERROR) << "Tether network with guid " << tether_network_guid nit: Make your log more descriptive to tell which function is failing. For example: "DisassociateTetherNetworkStateWithWifiNetwork(): Tether network with ID <id> not registered; could not remove association." https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... chromeos/network/network_state_handler.cc:617: << " doesn\'t exist.\n"; nit: You don't need to escape an apostrophe. Same below. https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... chromeos/network/network_state_handler.cc:630: You need to call NotifyNetworkListChanged() if at least one of the two has changed. You'll probably have to use two booleans to track this. https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... File chromeos/network/network_state_handler.h (right): https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... chromeos/network/network_state_handler.h:243: // Disassociates the Tether network specified by |guid| with its associated Update this comment to reflect the extra parameter and the changed parameter names. https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... chromeos/network/network_state_handler.h:245: bool DisassociateTetherNetworkStateWithWifiNetwork( Use From instead of With. https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... File chromeos/network/network_state_handler_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... chromeos/network/network_state_handler_unittest.cc:721: kTetherSignalStrength1, kTetherHasConnectedToHost2); Please revert this change. I purposely did not use this constant because I wanted to make it clear which parameter was different from the other default values. https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... chromeos/network/network_state_handler_unittest.cc:825: EXPECT_EQ(0u, test_observer_->network_list_changed_count()); nit: You don't need to verify this. It's assumed that before you do anything, it will be 0. https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... chromeos/network/network_state_handler_unittest.cc:859: After you make the NotifyNetworkListChanged() change, add this: EXPECT_EQ(4u, test_observer_->network_list_changed_count());
https://codereview.chromium.org/2819303002/diff/80001/ash/system/network/netw... File ash/system/network/network_icon_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/80001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:30: tether_network = On 2017/05/03 01:53:33, Kyle Horimoto wrote: > nit: > > tether_network = ... > tether_network->set_type() > tether_network->set_tether_guid() > > wifi_network = ... > wifi_network->set_type() > > etc. Done. https://codereview.chromium.org/2819303002/diff/80001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:77: std::unique_ptr<chromeos::NetworkState> tether_network, wifi_network, On 2017/05/03 01:53:33, Kyle Horimoto wrote: > Declare one per line. Done. https://codereview.chromium.org/2819303002/diff/80001/ash/system/network/netw... ash/system/network/network_icon_unittest.cc:78: cellular_network, wifi_tether_network; On 2017/05/03 01:53:33, Kyle Horimoto wrote: > nit: For people unfamiliar with how Tether networks work, it isn't clear what > wifi_tether_image is. Please add a comment explaining what this corresponds to. Done. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... File chromeos/components/tether/fake_wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/fake_wifi_hotspot_connector.cc:31: const std::string& tether_network_guid, On 2017/05/03 01:53:33, Kyle Horimoto wrote: > You should also save tether_network_guid like we do with the other parameters. Done. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... File chromeos/components/tether/initializer.cc (right): https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/initializer.cc:192: wifi_hotspot_connector_ = base::MakeUnique<WifiHotspotConnector>( On 2017/05/03 01:53:33, Kyle Horimoto wrote: > Delete this. Done. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... File chromeos/components/tether/wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:48: // disassociate that network with the Tether network and On 2017/05/03 01:53:33, Kyle Horimoto wrote: > nit: "disassociates with" is incorrect grammatically. Should be "disassociates > from". > > Also fix this below in your logs. Done. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:49: // call the callback, passing an empty string to signal that the connection On 2017/05/03 01:53:33, Kyle Horimoto wrote: > nit: Some of this fits on the previous line. Try to fit as many characters per > line as you can without going over the 80-char limit. Done. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:55: PA_LOG(INFO) << "Wifi network with ID " << wifi_network_guid_ On 2017/05/03 01:53:33, Kyle Horimoto wrote: > Just make this one sentence: > > Wifi network with ID <id> successfully disassociated from Tether network with ID > <id>. > > No need to repeat the Wi-Fi network ID again. Same with the error case. Done. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:118: // If the network is now connectable, associate it with a Tether network On 2017/05/03 01:53:33, Kyle Horimoto wrote: > Move this comment to above the AssociateTetherNetworkStateWithWifiNetwork() > call. It's unrelated to the ConnectToNetworkId() call. Done. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:118: // If the network is now connectable, associate it with a Tether network On 2017/05/03 01:53:33, Kyle Horimoto wrote: > Move this comment to above the AssociateTetherNetworkStateWithWifiNetwork() > call. It's unrelated to the ConnectToNetworkId() call. Done, but this is basically reverting back to: https://codereview.chromium.org/2819303002/patch/20001/30009 Is that your intention? https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... File chromeos/components/tether/wifi_hotspot_connector_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:237: // EXPECT_TRUE(network_state->tether_guid().empty()); On 2017/05/03 01:53:34, Kyle Horimoto wrote: > Use this, delete the line below. Done. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:409: VerifyNetworkNotAssociated(wifi_guid1); On 2017/05/03 01:53:33, Kyle Horimoto wrote: > You can move this to just below the "EXPECT_FALSE(service_path2.empty());" call. > In general, try to verify things just after they happened (or didn't happen, in > this case). Done. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector_unittest.cc:451: // Disassocciate first network from Tether network. On 2017/05/03 01:53:33, Kyle Horimoto wrote: > nit: The way you word this makes it seem like the lines of code beneath this are > the ones diassociating. Say something like "The first Tether and Wi-Fi networks > should no longer be associated." Done. https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... chromeos/network/network_state_handler.cc:592: On 2017/05/03 01:53:34, Kyle Horimoto wrote: > nit: Remove changes to functions you did not edit. This makes "git blame" a lot > easier to understand. Done. https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... chromeos/network/network_state_handler.cc:616: NET_LOG(ERROR) << "Tether network with guid " << tether_network_guid On 2017/05/03 01:53:34, Kyle Horimoto wrote: > nit: Make your log more descriptive to tell which function is failing. For > example: > > "DisassociateTetherNetworkStateWithWifiNetwork(): Tether network with ID <id> > not registered; could not remove association." Done. https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... chromeos/network/network_state_handler.cc:617: << " doesn\'t exist.\n"; On 2017/05/03 01:53:34, Kyle Horimoto wrote: > nit: You don't need to escape an apostrophe. Same below. Done. https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... chromeos/network/network_state_handler.cc:630: On 2017/05/03 01:53:34, Kyle Horimoto wrote: > You need to call NotifyNetworkListChanged() if at least one of the two has > changed. You'll probably have to use two booleans to track this. Done. https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... File chromeos/network/network_state_handler.h (right): https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... chromeos/network/network_state_handler.h:243: // Disassociates the Tether network specified by |guid| with its associated On 2017/05/03 01:53:34, Kyle Horimoto wrote: > Update this comment to reflect the extra parameter and the changed parameter > names. Done. https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... chromeos/network/network_state_handler.h:245: bool DisassociateTetherNetworkStateWithWifiNetwork( On 2017/05/03 01:53:34, Kyle Horimoto wrote: > Use From instead of With. Done. https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... File chromeos/network/network_state_handler_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... chromeos/network/network_state_handler_unittest.cc:721: kTetherSignalStrength1, kTetherHasConnectedToHost2); On 2017/05/03 01:53:34, Kyle Horimoto wrote: > Please revert this change. I purposely did not use this constant because I > wanted to make it clear which parameter was different from the other default > values. Done. https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... chromeos/network/network_state_handler_unittest.cc:825: EXPECT_EQ(0u, test_observer_->network_list_changed_count()); On 2017/05/03 01:53:34, Kyle Horimoto wrote: > nit: You don't need to verify this. It's assumed that before you do anything, it > will be 0. Done. https://codereview.chromium.org/2819303002/diff/80001/chromeos/network/networ... chromeos/network/network_state_handler_unittest.cc:859: On 2017/05/03 01:53:34, Kyle Horimoto wrote: > After you make the NotifyNetworkListChanged() change, add this: > > EXPECT_EQ(4u, test_observer_->network_list_changed_count()); Done.
LGTM with some nits. Congrats on completing your first Chromium code review! Please wait for hansberry@ and stevenjb@'s reviews before submitting. https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... File chromeos/components/tether/wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:118: // If the network is now connectable, associate it with a Tether network On 2017/05/03 22:00:24, lesliewatkins wrote: > On 2017/05/03 01:53:33, Kyle Horimoto wrote: > > Move this comment to above the AssociateTetherNetworkStateWithWifiNetwork() > > call. It's unrelated to the ConnectToNetworkId() call. > > Done, but this is basically reverting back to: > https://codereview.chromium.org/2819303002/patch/20001/30009 > > Is that your intention? It's not reverting back to that. You changed the wording of the comment (it no longer mentions connecting). Keep the comment about associating up there, and add back the comment about initiating the connection down here. https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... File ash/system/network/network_icon_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... ash/system/network/network_icon_unittest.cc:82: // a Tether network via its tether_guid nit: Don't refer to private types in a description. Just say "via a Tether network GUID". Also, end your sentence in a period. https://codereview.chromium.org/2819303002/diff/100001/chromeos/components/te... File chromeos/components/tether/tether_connector_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/100001/chromeos/components/te... chromeos/components/tether/tether_connector_unittest.cc:425: EXPECT_TRUE(fake_wifi_hotspot_connector_->most_recent_password().empty()); Also check that tether GUID is empty. https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/netwo... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/netwo... chromeos/network/network_state_handler.cc:618: << "network with ID " << tether_network_guid super nit: I usually append a << " " at the end of the line so that on the next line, I can start my text without a space in front. https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/netwo... chromeos/network/network_state_handler.cc:639: return success; You no longer need a success boolean. Just return tether_network_changed && wifi_network_changed.
https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... File ash/system/network/network_icon.cc (right): https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... ash/system/network/network_icon.cc:431: return BARS; {} around each clause when any statement or clause > 1 line. https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... ash/system/network/network_icon.cc:442: } I think this only makes for ICON_TYPE_TRAY? Otherwise this would show a mobile icon in the WiFi list which would be confusing. (We should be hiding tether networks in the WiFI list, but I'm not sure we should count on that?) https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/manag... File chromeos/network/managed_state.h (right): https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/manag... chromeos/network/managed_state.h:20: } } // namespace network_icon https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/manag... chromeos/network/managed_state.h:21: } } // namespace ash (We don't always do this for small/simple cases like 'namespace base' below, but when namespaces are nested or contain more than a few lines, it's always a good idea) https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/netwo... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/netwo... chromeos/network/network_state_handler.cc:606: const std::string& wifi_network_guid) { We shouldn't need wifi_network_guid here? This should always be tether_network->tether_guid(), right? (You can DCHECK that in the calling code).
lgtm! Please update the description of this CL -- at least also mention that you've changed when association occurs. Please also see nits. https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... File ash/system/network/network_icon.cc (right): https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... ash/system/network/network_icon.cc:757: icon = new NetworkIconImpl(network->path(), icon_type, network->type()); Why not use GetEffectiveNetworkType here?
Description was changed from ========== Changed wifi arcs to mobile bars for Tether network. Adding stevenjb@ for network_icon.cc. BUG=672263 ========== to ========== Changed wifi arcs to mobile bars for Tether network. Added ability to disassociate Tether and Wi-Fi networks, and now Wi-Fi and Tether networks are associated as soon as the network becomes connectable. Adding stevenjb@ for network_icon.cc. BUG=672263 ==========
https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... File chromeos/components/tether/wifi_hotspot_connector.cc (right): https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... chromeos/components/tether/wifi_hotspot_connector.cc:118: // If the network is now connectable, associate it with a Tether network On 2017/05/03 22:47:28, Kyle Horimoto wrote: > On 2017/05/03 22:00:24, lesliewatkins wrote: > > On 2017/05/03 01:53:33, Kyle Horimoto wrote: > > > Move this comment to above the AssociateTetherNetworkStateWithWifiNetwork() > > > call. It's unrelated to the ConnectToNetworkId() call. > > > > Done, but this is basically reverting back to: > > https://codereview.chromium.org/2819303002/patch/20001/30009 > > > > Is that your intention? > > It's not reverting back to that. You changed the wording of the comment (it no > longer mentions connecting). Keep the comment about associating up there, and > add back the comment about initiating the connection down here. Done. https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... File ash/system/network/network_icon.cc (right): https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... ash/system/network/network_icon.cc:431: return BARS; On 2017/05/03 23:02:21, stevenjb wrote: > {} around each clause when any statement or clause > 1 line. Done. https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... ash/system/network/network_icon.cc:442: } On 2017/05/03 23:02:21, stevenjb wrote: > I think this only makes for ICON_TYPE_TRAY? > > Otherwise this would show a mobile icon in the WiFi list which would be > confusing. (We should be hiding tether networks in the WiFI list, but I'm not > sure we should count on that?) This function now takes icon_type as an additional argument. Right now the unit tests only cover the ICON_TYPE_TRAY case. Should I write more? https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... ash/system/network/network_icon.cc:757: icon = new NetworkIconImpl(network->path(), icon_type, network->type()); On 2017/05/03 23:13:28, Ryan Hansberry wrote: > Why not use GetEffectiveNetworkType here? Done. https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... File ash/system/network/network_icon_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... ash/system/network/network_icon_unittest.cc:82: // a Tether network via its tether_guid On 2017/05/03 22:47:28, Kyle Horimoto wrote: > nit: Don't refer to private types in a description. Just say "via a Tether > network GUID". > > Also, end your sentence in a period. Done. https://codereview.chromium.org/2819303002/diff/100001/chromeos/components/te... File chromeos/components/tether/tether_connector_unittest.cc (right): https://codereview.chromium.org/2819303002/diff/100001/chromeos/components/te... chromeos/components/tether/tether_connector_unittest.cc:425: EXPECT_TRUE(fake_wifi_hotspot_connector_->most_recent_password().empty()); On 2017/05/03 22:47:28, Kyle Horimoto wrote: > Also check that tether GUID is empty. Done. https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/manag... File chromeos/network/managed_state.h (right): https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/manag... chromeos/network/managed_state.h:20: } On 2017/05/03 23:02:21, stevenjb wrote: > } // namespace network_icon Done. https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/manag... chromeos/network/managed_state.h:21: } On 2017/05/03 23:02:21, stevenjb wrote: > } // namespace ash > > (We don't always do this for small/simple cases like 'namespace base' below, but > when namespaces are nested or contain more than a few lines, it's always a good > idea) Done. https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/netwo... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/netwo... chromeos/network/network_state_handler.cc:606: const std::string& wifi_network_guid) { On 2017/05/03 23:02:21, stevenjb wrote: > We shouldn't need wifi_network_guid here? This should always be > tether_network->tether_guid(), right? (You can DCHECK that in the calling code). I didn't do the DCHECK part because I'm not exactly sure how, but I changed the function the way you suggested. https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/netwo... chromeos/network/network_state_handler.cc:618: << "network with ID " << tether_network_guid On 2017/05/03 22:47:28, Kyle Horimoto wrote: > super nit: I usually append a << " " at the end of the line so that on the next > line, I can start my text without a space in front. Done. https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/netwo... chromeos/network/network_state_handler.cc:639: return success; On 2017/05/03 22:47:28, Kyle Horimoto wrote: > You no longer need a success boolean. Just return tether_network_changed && > wifi_network_changed. Done.
On 2017/05/04 01:40:16, lesliewatkins wrote: > https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... > File chromeos/components/tether/wifi_hotspot_connector.cc (right): > > https://codereview.chromium.org/2819303002/diff/80001/chromeos/components/tet... > chromeos/components/tether/wifi_hotspot_connector.cc:118: // If the network is > now connectable, associate it with a Tether network > On 2017/05/03 22:47:28, Kyle Horimoto wrote: > > On 2017/05/03 22:00:24, lesliewatkins wrote: > > > On 2017/05/03 01:53:33, Kyle Horimoto wrote: > > > > Move this comment to above the > AssociateTetherNetworkStateWithWifiNetwork() > > > > call. It's unrelated to the ConnectToNetworkId() call. > > > > > > Done, but this is basically reverting back to: > > > https://codereview.chromium.org/2819303002/patch/20001/30009 > > > > > > Is that your intention? > > > > It's not reverting back to that. You changed the wording of the comment (it no > > longer mentions connecting). Keep the comment about associating up there, and > > add back the comment about initiating the connection down here. > > Done. > > https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... > File ash/system/network/network_icon.cc (right): > > https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... > ash/system/network/network_icon.cc:431: return BARS; > On 2017/05/03 23:02:21, stevenjb wrote: > > {} around each clause when any statement or clause > 1 line. > > Done. > > https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... > ash/system/network/network_icon.cc:442: } > On 2017/05/03 23:02:21, stevenjb wrote: > > I think this only makes for ICON_TYPE_TRAY? > > > > Otherwise this would show a mobile icon in the WiFi list which would be > > confusing. (We should be hiding tether networks in the WiFI list, but I'm not > > sure we should count on that?) > > This function now takes icon_type as an additional argument. > > Right now the unit tests only cover the ICON_TYPE_TRAY case. Should I write > more? > > https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... > ash/system/network/network_icon.cc:757: icon = new > NetworkIconImpl(network->path(), icon_type, network->type()); > On 2017/05/03 23:13:28, Ryan Hansberry wrote: > > Why not use GetEffectiveNetworkType here? > > Done. > > https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... > File ash/system/network/network_icon_unittest.cc (right): > > https://codereview.chromium.org/2819303002/diff/100001/ash/system/network/net... > ash/system/network/network_icon_unittest.cc:82: // a Tether network via its > tether_guid > On 2017/05/03 22:47:28, Kyle Horimoto wrote: > > nit: Don't refer to private types in a description. Just say "via a Tether > > network GUID". > > > > Also, end your sentence in a period. > > Done. > > https://codereview.chromium.org/2819303002/diff/100001/chromeos/components/te... > File chromeos/components/tether/tether_connector_unittest.cc (right): > > https://codereview.chromium.org/2819303002/diff/100001/chromeos/components/te... > chromeos/components/tether/tether_connector_unittest.cc:425: > EXPECT_TRUE(fake_wifi_hotspot_connector_->most_recent_password().empty()); > On 2017/05/03 22:47:28, Kyle Horimoto wrote: > > Also check that tether GUID is empty. > > Done. > > https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/manag... > File chromeos/network/managed_state.h (right): > > https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/manag... > chromeos/network/managed_state.h:20: } > On 2017/05/03 23:02:21, stevenjb wrote: > > } // namespace network_icon > > Done. > > https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/manag... > chromeos/network/managed_state.h:21: } > On 2017/05/03 23:02:21, stevenjb wrote: > > } // namespace ash > > > > (We don't always do this for small/simple cases like 'namespace base' below, > but > > when namespaces are nested or contain more than a few lines, it's always a > good > > idea) > > Done. > > https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/netwo... > File chromeos/network/network_state_handler.cc (right): > > https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/netwo... > chromeos/network/network_state_handler.cc:606: const std::string& > wifi_network_guid) { > On 2017/05/03 23:02:21, stevenjb wrote: > > We shouldn't need wifi_network_guid here? This should always be > > tether_network->tether_guid(), right? (You can DCHECK that in the calling > code). > > I didn't do the DCHECK part because I'm not exactly sure how, but I changed the > function the way you suggested. > > https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/netwo... > chromeos/network/network_state_handler.cc:618: << "network with ID " << > tether_network_guid > On 2017/05/03 22:47:28, Kyle Horimoto wrote: > > super nit: I usually append a << " " at the end of the line so that on the > next > > line, I can start my text without a space in front. > > Done. > > https://codereview.chromium.org/2819303002/diff/100001/chromeos/network/netwo... > chromeos/network/network_state_handler.cc:639: return success; > On 2017/05/03 22:47:28, Kyle Horimoto wrote: > > You no longer need a success boolean. Just return tether_network_changed && > > wifi_network_changed. > > Done. I don't see an updated patch set?
Sorry about that!
lgtm https://codereview.chromium.org/2819303002/diff/120001/chromeos/network/netwo... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/2819303002/diff/120001/chromeos/network/netwo... chromeos/network/network_state_handler.cc:613: "not registered; could not remove association."; What Kyle meant was: NET_LOG(ERROR) << "DisassociateTetherNetworkStateWithWifiNetwork(): Tether " << "network with ID " << tether_network_guid << " " << "not registered; could not remove association."; And personally I would put whatever text from the last line that fits on the second line there. https://codereview.chromium.org/2819303002/diff/120001/chromeos/network/netwo... chromeos/network/network_state_handler.cc:624: << " not registered; could not remove association."; nit: Same " " issue here (but as Kyle mentioned, it's a tiny nit, this is fine as-is, but consistency within the function at least is best).
The CQ bit was checked by lesliewatkins@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from khorimoto@chromium.org, hansberry@chromium.org Link to the patchset: https://codereview.chromium.org/2819303002/#ps120001 (title: "khorimoto@ hansberry@ stevenjb@ comments")
The CQ bit was unchecked by lesliewatkins@chromium.org
https://codereview.chromium.org/2819303002/diff/120001/chromeos/network/netwo... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/2819303002/diff/120001/chromeos/network/netwo... chromeos/network/network_state_handler.cc:613: "not registered; could not remove association."; On 2017/05/04 17:21:27, stevenjb wrote: > What Kyle meant was: > NET_LOG(ERROR) << "DisassociateTetherNetworkStateWithWifiNetwork(): Tether " > << "network with ID " << tether_network_guid << " " > << "not registered; could not remove association."; > > And personally I would put whatever text from the last line that fits on the > second line there. Done. https://codereview.chromium.org/2819303002/diff/120001/chromeos/network/netwo... chromeos/network/network_state_handler.cc:624: << " not registered; could not remove association."; On 2017/05/04 17:21:27, stevenjb wrote: > nit: Same " " issue here (but as Kyle mentioned, it's a tiny nit, this is fine > as-is, but consistency within the function at least is best). Done.
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/2819303002/#ps140001 (title: "stevenjb@ comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
For posterity... 'git cl format' doesn't seem to allow a string literal to be chained together with the << operator on the same line after a variable-- that's why it looked kind of funky when I tried to fix my logs to be compliant with Kyle's nit. (https://codereview.chromium.org/2819303002/diff/120001/chromeos/network/netwo...) I just reverted back to the old way since it seems pretty minor.
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_...)
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/2819303002/#ps160001 (title: "Fixed small typo preventing unit tests from building.")
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": 160001, "attempt_start_ts": 1493923086299960,
"parent_rev": "7533f415b244d51ff62f3216c524ad774ca5549e", "commit_rev":
"b4f862467b0dce4e75b337f28fab0b7a5019f6ba"}
Message was sent while issue was closed.
Description was changed from ========== Changed wifi arcs to mobile bars for Tether network. Added ability to disassociate Tether and Wi-Fi networks, and now Wi-Fi and Tether networks are associated as soon as the network becomes connectable. Adding stevenjb@ for network_icon.cc. BUG=672263 ========== to ========== Changed wifi arcs to mobile bars for Tether network. Added ability to disassociate Tether and Wi-Fi networks, and now Wi-Fi and Tether networks are associated as soon as the network becomes connectable. Adding stevenjb@ for network_icon.cc. BUG=672263 Review-Url: https://codereview.chromium.org/2819303002 Cr-Commit-Position: refs/heads/master@{#469430} Committed: https://chromium.googlesource.com/chromium/src/+/b4f862467b0dce4e75b337f28fab... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/b4f862467b0dce4e75b337f28fab...
Message was sent while issue was closed.
https://codereview.chromium.org/2819303002/diff/160001/ash/system/network/net... File ash/system/network/network_icon.cc (right): https://codereview.chromium.org/2819303002/diff/160001/ash/system/network/net... ash/system/network/network_icon.cc:620: image_ = GetBasicImage(false, icon_type, network_type); This will crash in GetBasicImage if network_type is VPN. It looks like this is intended to create a default image which will get updated elsewhere. Was there a reason to change this?
Message was sent while issue was closed.
On 2017/05/08 23:56:19, stevenjb wrote: > https://codereview.chromium.org/2819303002/diff/160001/ash/system/network/net... > File ash/system/network/network_icon.cc (right): > > https://codereview.chromium.org/2819303002/diff/160001/ash/system/network/net... > ash/system/network/network_icon.cc:620: image_ = GetBasicImage(false, icon_type, > network_type); > This will crash in GetBasicImage if network_type is VPN. It looks like this is > intended to create a default image which will get updated elsewhere. Was there a > reason to change this? This bug has been reported here: https://bugs.chromium.org/p/chromium/issues/detail?id=719681 |
