Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(3568)

Unified Diff: chrome/browser/chromeos/net/network_portal_detector.cc

Issue 11419070: Added detection timeouts and usage of Retry-After HTTP header. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src
Patch Set: Small fixes. Created 8 years, 1 month ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: chrome/browser/chromeos/net/network_portal_detector.cc
diff --git a/chrome/browser/chromeos/net/network_portal_detector.cc b/chrome/browser/chromeos/net/network_portal_detector.cc
index a70418bbfd120d80ea06ce1abe875c3fe0e5e9f9..54f2f13362bf22eeea3f2b5d82437b5d3136bd1f 100644
--- a/chrome/browser/chromeos/net/network_portal_detector.cc
+++ b/chrome/browser/chromeos/net/network_portal_detector.cc
@@ -7,6 +7,7 @@
#include "base/bind.h"
#include "base/command_line.h"
#include "base/logging.h"
+#include "base/message_loop.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/chromeos/cros/cros_library.h"
#include "chrome/common/chrome_switches.h"
@@ -20,6 +21,10 @@ namespace chromeos {
namespace {
+const int kMaxRequestAttempts = 3;
Nikita (slow) 2012/11/20 12:58:45 nit: Please add comments for these contants
ygorshenin1 2012/11/21 09:08:06 Done.
+const int kMinTimeBetweenAttemptsSec = 3;
+const int kRequestTimeoutSec = 10;
+
std::string CaptivePortalStateString(
NetworkPortalDetector::CaptivePortalState state) {
switch (state) {
@@ -44,7 +49,12 @@ NetworkPortalDetector* g_network_portal_detector = NULL;
NetworkPortalDetector::NetworkPortalDetector(
const scoped_refptr<net::URLRequestContextGetter>& request_context)
- : test_url_(CaptivePortalDetector::kDefaultURL) {
+ : test_url_(CaptivePortalDetector::kDefaultURL),
+ weak_ptr_factory_(this),
+ attempt_count_(0),
+ min_time_between_attempts_(
+ base::TimeDelta::FromSeconds(kMinTimeBetweenAttemptsSec)),
+ request_timeout_(base::TimeDelta::FromSeconds(kRequestTimeoutSec)) {
captive_portal_detector_.reset(
new CaptivePortalDetector(request_context));
}
@@ -64,6 +74,9 @@ void NetworkPortalDetector::Init() {
void NetworkPortalDetector::Shutdown() {
DCHECK(CalledOnValidThread());
+ detection_task_.Cancel();
+ detection_timeout_.Cancel();
+
captive_portal_detector_->Cancel();
captive_portal_detector_.reset();
observers_.Clear();
@@ -74,17 +87,21 @@ void NetworkPortalDetector::Shutdown() {
void NetworkPortalDetector::AddObserver(Observer* observer) {
DCHECK(CalledOnValidThread());
+
if (!observers_.HasObserver(observer))
observers_.AddObserver(observer);
}
void NetworkPortalDetector::RemoveObserver(Observer* observer) {
DCHECK(CalledOnValidThread());
+
observers_.RemoveObserver(observer);
}
NetworkPortalDetector::CaptivePortalState
NetworkPortalDetector::GetCaptivePortalState(const Network* network) {
+ DCHECK(CalledOnValidThread());
+
if (!network)
return CAPTIVE_PORTAL_STATE_UNKNOWN;
CaptivePortalStateMap::const_iterator it =
@@ -109,22 +126,27 @@ void NetworkPortalDetector::OnNetworkManagerChanged(NetworkLibrary* cros) {
if (active_network_id_ != new_active_network_id) {
active_network_id_ = new_active_network_id;
- if (IsCheckingForPortal()) {
- // Network is changed, so detection results will be incorrect.
- state_ = STATE_CHECKING_FOR_PORTAL_NETWORK_CHANGED;
- } else if (Network::IsConnectedState(connection_state)) {
- // Start captive portal detection, if possible.
- DetectCaptivePortal();
+ attempt_count_ = 0;
+ if (IsPortalCheckPending()) {
+ detection_task_.Cancel();
+ detection_timeout_.Cancel();
+ } else if (IsCheckingForPortal()) {
+ captive_portal_detector_->Cancel();
}
- } else if (!IsCheckingForPortal() &&
- Network::IsConnectedState(connection_state)) {
+ state_ = STATE_IDLE;
+ }
+ if (!IsCheckingForPortal() && !IsPortalCheckPending() &&
+ Network::IsConnectedState(connection_state) &&
+ attempt_count_ < kMaxRequestAttempts) {
DCHECK(active_network);
+
// Initiate Captive Portal detection only if network's captive
- // portal state is unknown (e.g. for freshly created networks) or offline.
+ // portal state is unknown (e.g. for freshly created networks) or
+ // offline.
CaptivePortalState state = GetCaptivePortalState(active_network);
if (state == CAPTIVE_PORTAL_STATE_UNKNOWN ||
state == CAPTIVE_PORTAL_STATE_OFFLINE) {
- DetectCaptivePortal();
+ DetectCaptivePortal(base::TimeDelta());
}
}
}
@@ -150,41 +172,88 @@ bool NetworkPortalDetector::IsEnabled() {
switches::kDisableChromeCaptivePortalDetector);
}
-void NetworkPortalDetector::DetectCaptivePortal() {
+void NetworkPortalDetector::DetectCaptivePortal(const base::TimeDelta& delay) {
+ DCHECK(!IsPortalCheckPending());
+ DCHECK(!IsCheckingForPortal());
+ DCHECK(attempt_count_ < kMaxRequestAttempts);
+
Nikita (slow) 2012/11/20 12:58:45 I think it makes sense to clear tasks here just in
ygorshenin1 2012/11/21 09:08:06 Done.
+ state_ = STATE_PORTAL_CHECK_PENDING;
+
+ next_attempt_delay_ = delay;
+ if (attempt_count_ > 0) {
+ base::TimeTicks now = GetCurrentTimeTicks();
+ base::TimeDelta elapsed_time;
+ if (now > attempt_start_time_)
+ elapsed_time = now - attempt_start_time_;
+ if (elapsed_time < min_time_between_attempts_ &&
+ min_time_between_attempts_ - elapsed_time > next_attempt_delay_) {
+ next_attempt_delay_ = min_time_between_attempts_ - elapsed_time;
+ }
+ }
+ detection_task_.Reset(
+ base::Bind(&NetworkPortalDetector::DetectCaptivePortalTask,
+ weak_ptr_factory_.GetWeakPtr()));
+ MessageLoop::current()->PostDelayedTask(FROM_HERE,
+ detection_task_.callback(),
+ next_attempt_delay_);
+}
+
+void NetworkPortalDetector::DetectCaptivePortalTask() {
+ DCHECK(IsPortalCheckPending());
+
state_ = STATE_CHECKING_FOR_PORTAL;
+
+ ++attempt_count_;
+ attempt_start_time_ = GetCurrentTimeTicks();
+
+ LOG(INFO) << "Portal detection starting attempt " << attempt_count_
Nikita (slow) 2012/11/20 12:58:45 Note that by default LOG(INFO) is not captured in
ygorshenin1 2012/11/21 09:08:06 Done.
+ << " of " << kMaxRequestAttempts;
+
captive_portal_detector_->DetectCaptivePortal(
test_url_,
base::Bind(&NetworkPortalDetector::OnPortalDetectionCompleted,
- base::Unretained(this)));
+ weak_ptr_factory_.GetWeakPtr()));
+ detection_timeout_.Reset(
+ base::Bind(&NetworkPortalDetector::PortalDetectionTimeout,
+ weak_ptr_factory_.GetWeakPtr()));
+ MessageLoop::current()->PostDelayedTask(FROM_HERE,
+ detection_timeout_.callback(),
+ request_timeout_);
+}
+
+void NetworkPortalDetector::PortalDetectionTimeout() {
+ DCHECK(CalledOnValidThread());
+ DCHECK(IsCheckingForPortal());
+
+ LOG(ERROR) << "Portal detection timeout";
+
+ captive_portal_detector_->Cancel();
+ CaptivePortalDetector::Results results;
+ results.result = captive_portal::RESULT_NO_RESPONSE;
+ OnPortalDetectionCompleted(results);
}
void NetworkPortalDetector::OnPortalDetectionCompleted(
const CaptivePortalDetector::Results& results) {
DCHECK(CalledOnValidThread());
-
DCHECK(IsCheckingForPortal());
+ LOG(ERROR) << "On portal detection completed";
Nikita (slow) 2012/11/20 12:58:45 LOG(INFO) or VLOG - see above.
ygorshenin1 2012/11/21 09:08:06 Done.
+
+ state_ = STATE_IDLE;
+ detection_timeout_.Cancel();
+
NetworkLibrary* cros = CrosLibrary::Get()->GetNetworkLibrary();
const Network* active_network = cros->active_network();
if (!active_network)
return;
- if (state_ == STATE_CHECKING_FOR_PORTAL_NETWORK_CHANGED) {
- // Can't use detection results, because network was changed. So
- // retry portal detection for the active network or, if possible.
- if (Network::IsConnectedState(active_network->connection_state()) &&
- GetCaptivePortalState(active_network) ==
- CAPTIVE_PORTAL_STATE_UNKNOWN) {
- DetectCaptivePortal();
- }
- return;
- }
-
- state_ = STATE_IDLE;
-
switch (results.result) {
case captive_portal::RESULT_NO_RESPONSE:
- SetCaptivePortalState(active_network, CAPTIVE_PORTAL_STATE_OFFLINE);
+ if (attempt_count_ >= kMaxRequestAttempts)
+ SetCaptivePortalState(active_network, CAPTIVE_PORTAL_STATE_OFFLINE);
+ else
+ DetectCaptivePortal(results.retry_after_delta);
break;
case captive_portal::RESULT_INTERNET_CONNECTED:
SetCaptivePortalState(active_network, CAPTIVE_PORTAL_STATE_ONLINE);
@@ -197,14 +266,18 @@ void NetworkPortalDetector::OnPortalDetectionCompleted(
}
}
+bool NetworkPortalDetector::IsPortalCheckPending() const {
+ return state_ == STATE_PORTAL_CHECK_PENDING;
+}
+
bool NetworkPortalDetector::IsCheckingForPortal() const {
- return state_ == STATE_CHECKING_FOR_PORTAL ||
- state_ == STATE_CHECKING_FOR_PORTAL_NETWORK_CHANGED;
+ return state_ == STATE_CHECKING_FOR_PORTAL;
}
void NetworkPortalDetector::SetCaptivePortalState(const Network* network,
CaptivePortalState state) {
DCHECK(network);
+
CaptivePortalStateMap::const_iterator it =
captive_portal_state_map_.find(network->unique_id());
if (it == captive_portal_state_map_.end() ||
@@ -222,4 +295,11 @@ void NetworkPortalDetector::NotifyPortalStateChanged(const Network* network,
FOR_EACH_OBSERVER(Observer, observers_, OnPortalStateChanged(network, state));
}
+base::TimeTicks NetworkPortalDetector::GetCurrentTimeTicks() const {
+ if (time_ticks_for_testing_.is_null())
+ return base::TimeTicks::Now();
+ else
+ return time_ticks_for_testing_;
+}
+
} // namespace chromeos

Powered by Google App Engine
This is Rietveld 408576698