Fix auto-reload histograms.

Two bugs, and general state-machine grotesqueness, prevented auto-reload
histograms from working properly in production:

1. Success could only be reported in OnCommitLoad() if can_auto_reload_page_ was
true, but can_auto_reload_page_ would be set to false in OnStartLoad(), since
OnStartLoad() had no way of knowing that the non-error page load it saw starting
was actually part of an attempt to auto-reload the page. This has been patched
around by tracking is_auto_reloading in committed_error_page_info_; when the
auto reload timer fires, it sets is_auto_reloading to indicate to OnCommitLoad
that auto-reload was running. OnCommitLoad can then use is_auto_reloading and
the loading URL to decide whether the committing load represents a success or
failure for auto-reload.

2. Whenever a non-error load started, OnStartLoad() would call
CancelPendingFetches(), intending to prevent auto-reload from replacing the
starting load with its own; as a side-effect, CancelPendingFetches() would
assume that if there was an existing error page which looked auto-reloadable
that the cancel represented a failure of auto-reload and log a histogram entry.
Unfortunately, this logic would trigger at each reload attempt by auto-reload,
inflating the failure count with nonexistent failures. To prevent this
misbehavior, CancelPendingFetches() is now no longer responsible for logging
auto-reload failures; instead, failures are detected and logged in
OnCommitLoad() (for "user navigated elsewhere" failures), OnStop (for "user
pressed Stop to abort auto-reload" failures), and the NetErrorHelperCore
destructor (for "user closed the tab" failures).

To help clean up the code, statistics are now reported through
ReportAutoReloadSuccess() and ReportAutoReloadFailure().

There are new unit tests covering these cases:
1. Auto-reload succeeding on the first attempt;
2. Auto-reload succeeding on the second attempt;
3. The user manually stopping an auto-reload;
4. The user manually stopping a non-auto-reload load;
5. The user navigating to a different URL;

As part of this cleanup, simplify the state machine; the ambiguous "can_auto_reload_page_" flag is gone, replaced by an implicit state bit in whether the timer is running (or paused) or not.

BUG=

Review URL: https://codereview.chromium.org/259613003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@271007 0039d316-1c4b-4281-b951-d872f2087c98
parent 57ee4546
...@@ -116,7 +116,7 @@ void NetErrorHelper::DidStartProvisionalLoad() { ...@@ -116,7 +116,7 @@ void NetErrorHelper::DidStartProvisionalLoad() {
void NetErrorHelper::DidCommitProvisionalLoad(bool is_new_navigation) { void NetErrorHelper::DidCommitProvisionalLoad(bool is_new_navigation) {
blink::WebFrame* frame = render_frame()->GetWebFrame(); blink::WebFrame* frame = render_frame()->GetWebFrame();
core_.OnCommitLoad(GetFrameType(frame)); core_.OnCommitLoad(GetFrameType(frame), frame->document().url());
} }
void NetErrorHelper::DidFinishLoad() { void NetErrorHelper::DidFinishLoad() {
......
...@@ -130,7 +130,7 @@ class NetErrorHelperCore { ...@@ -130,7 +130,7 @@ class NetErrorHelperCore {
// These methods handle tracking the actual state of the page. // These methods handle tracking the actual state of the page.
void OnStartLoad(FrameType frame_type, PageType page_type); void OnStartLoad(FrameType frame_type, PageType page_type);
void OnCommitLoad(FrameType frame_type); void OnCommitLoad(FrameType frame_type, const GURL& url);
void OnFinishLoad(FrameType frame_type); void OnFinishLoad(FrameType frame_type);
void OnStop(); void OnStop();
...@@ -225,14 +225,29 @@ class NetErrorHelperCore { ...@@ -225,14 +225,29 @@ class NetErrorHelperCore {
NavigationCorrectionParams navigation_correction_params_; NavigationCorrectionParams navigation_correction_params_;
// True if auto-reload is enabled at all.
bool auto_reload_enabled_; bool auto_reload_enabled_;
// Timer used to wait for auto-reload attempts.
scoped_ptr<base::Timer> auto_reload_timer_; scoped_ptr<base::Timer> auto_reload_timer_;
// True if the auto-reload timer would be running but is waiting for an
// offline->online network transition.
bool auto_reload_paused_;
// True if there is an uncommitted-but-started load, error page or not. This
// is used to inhibit starting auto-reload when an error page finishes, in
// case this happens:
// Error page starts
// Error page commits
// Non-error page starts
// Error page finishes
bool uncommitted_load_started_;
// Is the browser online? // Is the browser online?
bool online_; bool online_;
int auto_reload_count_; int auto_reload_count_;
bool can_auto_reload_page_;
// This value is set only when a navigation has been initiated from // This value is set only when a navigation has been initiated from
// the error page. It is used to detect when such navigations result // the error page. It is used to detect when such navigations result
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment