Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
979183003:
Fix logic for default network notification (Closed)
|
Created:
5 years, 9 months ago by stevenjb Modified:
5 years, 9 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@issue_432404_default_network_0 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix logic for default network notification
BUG=432404
Committed: https://crrev.com/89c8b88b60db6b91a8cdbb42fd36aca7e098e101
Cr-Commit-Position: refs/heads/master@{#320670}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 11
Patch Set 4 : . #Patch Set 5 : Add test #
Total comments: 5
Patch Set 6 : More/better comments #Patch Set 7 : Rebase #
Messages
Total messages: 18 (6 generated)
stevenjb@chromium.org changed reviewers: + pneubeck@chromium.org, pstew@chromium.org
I'll update the CL desc tomorrow, but ptal at the logic if you can.
Are there any tests that need to be updated as a result of this change? https://codereview.chromium.org/979183003/diff/40001/chromeos/network/network... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/979183003/diff/40001/chromeos/network/network... chromeos/network/network_state_handler.cc:784: // connection state changes. Awesome. This might fix some page load attempts before the connection is fully up.
lgtm with comments https://codereview.chromium.org/979183003/diff/40001/chromeos/network/network... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/979183003/diff/40001/chromeos/network/network... chromeos/network/network_state_handler.cc:767: default_network_path_ = service_path; Not that it is affecting this CL, but just a reminder: You fix the notifications sent out by NetworkStateHandler, but the exposed state might still show a connecting default network. I.e. if someone happens to poll the network state at an unlucky moment, he will still see this weird state. https://codereview.chromium.org/979183003/diff/40001/chromeos/network/network... chromeos/network/network_state_handler.cc:775: NET_LOG(EVENT) "Default NetworkState not available: " missing '<<' after NET_LOG(EVENT) ? how can this compile? https://codereview.chromium.org/979183003/diff/40001/chromeos/network/network... chromeos/network/network_state_handler.cc:783: // Do not notify observers here, the notification will occur when the is it worth logging an error here if the state is neither Connected nor Connecting? (as you do in the OnNetworkConnectionStateChanged function below) https://codereview.chromium.org/979183003/diff/40001/chromeos/network/network... chromeos/network/network_state_handler.cc:892: if (notify_default) the log message should always use the prefix 'Default' if the default network is affected. Currently, you do that only if the default network is also connected. Note, that you can also not compare network-path() with default_network_path_ here anymore, since the latter is cleared in line 886.
https://codereview.chromium.org/979183003/diff/40001/chromeos/network/network... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/979183003/diff/40001/chromeos/network/network... chromeos/network/network_state_handler.cc:767: default_network_path_ = service_path; On 2015/03/12 09:57:48, pneubeck wrote: > Not that it is affecting this CL, but just a reminder: > > You fix the notifications sent out by NetworkStateHandler, but the exposed state > might still show a connecting default network. > I.e. if someone happens to poll the network state at an unlucky moment, he will > still see this weird state. Yes, that is true. I've limited the notifications here to reduce noise, and hopefully we will now end up always in a stable state, but the rest of the code should be able to handle an intermediate state with a non connected default network. Might be worth re-examining that though. Ideally Shill will never set this to a non connected network so the state update for the network will immediately follow, before any notifications are triggered (now). https://codereview.chromium.org/979183003/diff/40001/chromeos/network/network... chromeos/network/network_state_handler.cc:775: NET_LOG(EVENT) "Default NetworkState not available: " On 2015/03/12 09:57:48, pneubeck wrote: > missing '<<' after NET_LOG(EVENT) ? > > how can this compile? It doesn't. Last second change to use the same LOG format within the function on self review. Fixed. https://codereview.chromium.org/979183003/diff/40001/chromeos/network/network... chromeos/network/network_state_handler.cc:783: // Do not notify observers here, the notification will occur when the On 2015/03/12 09:57:48, pneubeck wrote: > is it worth logging an error here if the state is neither Connected nor > Connecting? (as you do in the OnNetworkConnectionStateChanged function below) Yes, done. https://codereview.chromium.org/979183003/diff/40001/chromeos/network/network... chromeos/network/network_state_handler.cc:784: // connection state changes. On 2015/03/11 23:29:03, Paul Stewart wrote: > Awesome. This might fix some page load attempts before the connection is fully > up. I hope so! https://codereview.chromium.org/979183003/diff/40001/chromeos/network/network... chromeos/network/network_state_handler.cc:892: if (notify_default) On 2015/03/12 09:57:48, pneubeck wrote: > the log message should always use the prefix 'Default' if the default network is > affected. Currently, you do that only if the default network is also connected. > > Note, that you can also not compare network-path() with default_network_path_ > here anymore, since the latter is cleared in line 886. My intention is to have the log match the *notification* (thus the NOTIFY: prefix). The logic is to avoid having two nearly identical log messages when we notify about the default network changing. We log DefaultNetworkServiceChanged on line 768.
On 2015/03/11 23:29:03, Paul Stewart wrote: > Are there any tests that need to be updated as a result of this change? > > https://codereview.chromium.org/979183003/diff/40001/chromeos/network/network... > File chromeos/network/network_state_handler.cc (right): > > https://codereview.chromium.org/979183003/diff/40001/chromeos/network/network... > chromeos/network/network_state_handler.cc:784: // connection state changes. > Awesome. This might fix some page load attempts before the connection is fully > up. Actually, now that you mention it, we can test this, but clearly don't. (We have a test, but it doesn't include this edge case). I will add a test that fails with the old logic and passes with the new logic. Done.
PTAL
still lgtm https://codereview.chromium.org/979183003/diff/40001/chromeos/network/network... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/979183003/diff/40001/chromeos/network/network... chromeos/network/network_state_handler.cc:892: if (notify_default) On 2015/03/12 16:42:28, stevenjb wrote: > On 2015/03/12 09:57:48, pneubeck wrote: > > the log message should always use the prefix 'Default' if the default network > is > > affected. Currently, you do that only if the default network is also > connected. > > > > Note, that you can also not compare network-path() with default_network_path_ > > here anymore, since the latter is cleared in line 886. > > My intention is to have the log match the *notification* (thus the NOTIFY: > prefix). The logic is to avoid having two nearly identical log messages when we > notify about the default network changing. > > We log DefaultNetworkServiceChanged on line 768. > ok, makes sense. https://codereview.chromium.org/979183003/diff/80001/chromeos/network/network... File chromeos/network/network_state_handler_unittest.cc (right): https://codereview.chromium.org/979183003/diff/80001/chromeos/network/network... chromeos/network/network_state_handler_unittest.cc:173: : device_test_(nullptr), nit: you could put all of these also to the declaration (since C++11) and drop the constructor. https://codereview.chromium.org/979183003/diff/80001/chromeos/network/network... chromeos/network/network_state_handler_unittest.cc:620: base::StringValue(shill::kStateIdle)); since both of these calls send a notification to NSH, it's not obvious to the reader how many updates are to expected and why. Maybe add a comment to the EXPECT_EQ(1u, ...). https://codereview.chromium.org/979183003/diff/80001/chromeos/network/network... chromeos/network/network_state_handler_unittest.cc:652: // state to Connected. The DefaultNetworkChange dnotification should not dnotification -> notification
https://codereview.chromium.org/979183003/diff/80001/chromeos/network/network... File chromeos/network/network_state_handler_unittest.cc (right): https://codereview.chromium.org/979183003/diff/80001/chromeos/network/network... chromeos/network/network_state_handler_unittest.cc:173: : device_test_(nullptr), On 2015/03/12 19:40:09, pneubeck wrote: > nit: you could put all of these also to the declaration (since C++11) and drop > the constructor. I'd rather do that consistently throughout these classes at some point, but noted. https://codereview.chromium.org/979183003/diff/80001/chromeos/network/network... chromeos/network/network_state_handler_unittest.cc:620: base::StringValue(shill::kStateIdle)); On 2015/03/12 19:40:09, pneubeck wrote: > since both of these calls send a notification to NSH, it's not obvious to the > reader how many updates are to expected and why. > Maybe add a comment to the EXPECT_EQ(1u, ...). Done.
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pneubeck@chromium.org Link to the patchset: https://codereview.chromium.org/979183003/#ps100001 (title: "More/better comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/979183003/100001
The CQ bit was unchecked by stevenjb@chromium.org
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pneubeck@chromium.org Link to the patchset: https://codereview.chromium.org/979183003/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/979183003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/89c8b88b60db6b91a8cdbb42fd36aca7e098e101 Cr-Commit-Position: refs/heads/master@{#320670} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
