Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
136203009:
Support auto-reload on errors. (Closed)
|
Created:
6 years, 10 months ago by Elly Fong-Jones Modified:
6 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Visibility:
Public. |
DescriptionWhen the auto-reload experiment is enabled ("enable-offline-auto-reload"), if a browser tab loads an error page, and the error seems transient, the renderer process starts attempting to automatically reload the tab's contents with exponential backoff.
To NetErrorHelperCore, add a new delegate method: ReloadPage(), which kicks off
a reload of the page in the observed frame. Add a new class MockableOneShotTimer
which provides a mockable interface to the standard base::OneShotTimer and is
used in NetErrorHelperCore for the autoreload timer. Add an implementation of
MockableOneShotTimer.
Add new member variables 'auto_reload_count_', 'auto_reload_pending_', and
'auto_reload_enabled_' to NetErrorHelperCore. The first tracks how many attempts
we have made to auto-reload; the second tracks whether we should be attempting
auto-reloads (i.e., the last loaded page was an error, and the user did not hit
'Stop' since that load); the last controls whether autoreload is enabled or not
and defaults to false.
The autoreload logic itself exists in
NetErrorHelperCore::{AutoReload,MaybeStartAutoReloadTimer,StartAutoReloadTimer}.
StartAutoReloadTimer sets a timer (auto_reload_timer_) which calls AutoReload
when it fires, and AutoReload starts a fetch of the error URL. The timer is
started with a successively larger interval at each successive call to
StartAutoReload() to implement exponential backoff. The auto-reload count is
reset (thus restarting the backoff) when we get a non-reloadable error like an
ERR_ABORTED or when a non-error page loads successfully. Any pending auto-reload
attempts stop (but the timer is not reset) if the user manually presses 'stop'
or a cross-process navigation starts (handled in NetErrorHelperCore::OnStop).
MaybeStartAutoReloadTimer is the real entry point for auto-reload; it starts
auto-reload if the last load was a reloadable error page, the browser is online,
and the user hasn't stopped the error page.
Also, extend the existing hook in
ChromeContentRendererClient::ShouldSuppressErrorPage to call
NetErrorHelper::ShouldSuppressErrorPage, which plumbs the request through to
NetErrorHelperCore.
Right now, if there are many tabs displaying error pages, the code in
NetworkStateChange() can cause a "reload storm", in which dozens or even
hundreds of auto-reload attempts start at the same time, possibly saturating the
network. Our current design for resolving this involves a ResourceThrottle in
the browser process to throttle individual tab loading, but design work is still
ongoing.
There are unit tests in NetErrorHelperCoreTest covering the various behaviors of
auto-reload, and helper code and a mock MockableOneShotTimer implementation
added to NetErrorHelperCoreTest. There is a browser test in
ErrorPageAutoReloadTest.
TODO: solve the "reload storm" issue.
TEST=unit,browser,trybot
BUG=308233
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257254
Patch Set 1 #
Total comments: 23
Patch Set 2 : Fixes #
Total comments: 90
Patch Set 3 : Update #Patch Set 4 : Add browser test. #Patch Set 5 : Clean up nits #
Total comments: 38
Patch Set 6 : Add UMA #
Total comments: 16
Patch Set 7 : Add experiment #
Total comments: 48
Patch Set 8 : More fixes #Patch Set 9 : More fixes #
Total comments: 58
Patch Set 10 : More fixes #
Total comments: 26
Patch Set 11 : Fixes #Patch Set 12 : Add MockableOneShotTimer tests. #
Total comments: 32
Patch Set 13 : Fixes #
Total comments: 10
Patch Set 14 : Rebase onto link doctor changes #Patch Set 15 : Handle comments on rev 13 #Patch Set 16 : placate clang #
Total comments: 32
Patch Set 17 : Pacify clang part 2 #Patch Set 18 : More testing. #
Total comments: 2
Patch Set 19 : More unit tests! #Patch Set 20 : Fix CCRC unit test crash #
Total comments: 1
Patch Set 21 : Handle another edge case #Patch Set 22 : Rebase? #
Total comments: 2
Patch Set 23 : DISALLOW_COPY_AND_ASSIGN NetErrorHelper #
Total comments: 13
Patch Set 24 : Last round of fixes #Patch Set 25 : Fix bogus include #
Total comments: 1
Patch Set 26 : MockableOneShotTimer -> base::Timer (now mockable!) #Messages
Total messages: 50 (0 generated)
PTAL? still a draft :)
The right bug number is 329618. The other behavior issue I'm a bit worried about is the reload storm that happens if: * You have a browser with a lot of tabs open. * You shut it down. * You start it up when you're offline, and get all sorts of error pages. * You go online in some fashion, and everyone and their brother hits the network at the same time to re-download the page. In some sense that's no worse than browser startup, but people at least understand what's going on at browser startup. I'm not sure what the right way to deal with this is, which maybe means it shouldn't be dealt with in this CL (though I want to have an idea of how we're going to deal with it before we enable auto-reload by default). Thoughts: * It's like browser startup; the user can deal. * We setup some kind of throttle. Maybe this work should also be done for browser startup? * We do a random delay after the NCN indicates we're online before we reload/start the exponential backoff. This is easy (because local), but doesn't scale well--at small numbers of tabs, we're randomly delaying the reload (but does this really matter?), and at large numbers, we probably won't have a large enough variance on the random delay, so we'll still get a storm. Thoughts? https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... File chrome/renderer/net/net_error_helper.cc (right): https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper.cc:216: // MockableOneShotTimer implementation in terms of base::OneShotTimer This feels like it should be in its own file, not in net_error_helper.cc, just so that other classes can use it if they want. https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper.cc:253: NetErrorHelperCore::MockableOneShotTimer* NetErrorHelper::NewMockableOneShotTimer() { nit: Line too long? https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:31: return error.reason != net::ERR_ABORTED; I believe we just want to reload on "offline like" errors, which I currently believe are: return (error == net::ERR_NAME_NOT_RESOLVED || error == net::ERR_INTERNET_DISCONNECTED || error == net::ERR_ADDRESS_UNREACHABLE || error == net::ERR_CONNECTION_TIMED_OUT); and based on the discussion with Paul J, I think we should add ERR_NETWORK_CHANGED. https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:129: auto_reload_enabled_(false), Why not attach this to the flag setup for this work in https://codereview.chromium.org/113133004/? https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:353: void NetErrorHelperCore::StartAutoReload() { We want the NCN indicating we're offline to override this; we should only do exponential backoff when the NCN says we're online.
Quick comments https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:225: if (IsReloadableError(error)) Don't want to auto reload POSTs. https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:225: if (IsReloadableError(error)) May make more sense to trigger this when the error page finishes loading (That is, OnDidFinishLoading, when there's a committed_error_page_info_, no link doctor load to start, and no pending_error_page_info_). https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:225: if (IsReloadableError(error)) We just try reloading eternally? Suppose shouldn't be too bad, but I wonder about the 1, 2, 4, 8, and 16 seconds reloads...Seems a bit too many. https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:346: if (!committed_error_page_info_) { Is it possible for this to trigger because it's taking the first error page too long to load? https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:353: void NetErrorHelperCore::StartAutoReload() { StartAutoReloadTimer? https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:354: int delay = 1 << auto_reload_count_; I suggest making the 1 a constant at the top of the file, and make make it in milliseconds. https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:363: return auto_reload_count_ != 0; This isn't correct in the OnStop() case.
https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:31: return error.reason != net::ERR_ABORTED; On 2014/01/30 20:14:47, rdsmith wrote: > I believe we just want to reload on "offline like" errors, which I currently > believe are: > > return (error == net::ERR_NAME_NOT_RESOLVED || > error == net::ERR_INTERNET_DISCONNECTED || > error == net::ERR_ADDRESS_UNREACHABLE || > error == net::ERR_CONNECTION_TIMED_OUT); > > and based on the discussion with Paul J, I think we should add > ERR_NETWORK_CHANGED. I'm not sure about those... ERR_CONNECTION_TIMED_OUT and ERR_ADDRESS_UNREACHABLE in particular. The latter you often see when upstream network devices are down, like your cable modem rebooting. Timed out you often see on random connection hiccups, or site hiccups. Also, if things are timing out, at least you're not really hammering the network trying to reload.
https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:31: return error.reason != net::ERR_ABORTED; On 2014/01/30 20:47:31, mmenke wrote: > On 2014/01/30 20:14:47, rdsmith wrote: > > I believe we just want to reload on "offline like" errors, which I currently > > believe are: > > > > return (error == net::ERR_NAME_NOT_RESOLVED || > > error == net::ERR_INTERNET_DISCONNECTED || > > error == net::ERR_ADDRESS_UNREACHABLE || > > error == net::ERR_CONNECTION_TIMED_OUT); > > > > and based on the discussion with Paul J, I think we should add > > ERR_NETWORK_CHANGED. > > I'm not sure about those... ERR_CONNECTION_TIMED_OUT and > ERR_ADDRESS_UNREACHABLE in particular. The latter you often see when upstream > network devices are down, like your cable modem rebooting. Timed out you often > see on random connection hiccups, or site hiccups. Also, if things are timing > out, at least you're not really hammering the network trying to reload. I'm confused; isn't modem rebooting and random connection hiccups exactly the situations in which we want to auto-reload with an exponential backoff?
On 2014/01/30 20:57:30, rdsmith wrote: > https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... > File chrome/renderer/net/net_error_helper_core.cc (right): > > https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... > chrome/renderer/net/net_error_helper_core.cc:31: return error.reason != > net::ERR_ABORTED; > On 2014/01/30 20:47:31, mmenke wrote: > > On 2014/01/30 20:14:47, rdsmith wrote: > > > I believe we just want to reload on "offline like" errors, which I currently > > > believe are: > > > > > > return (error == net::ERR_NAME_NOT_RESOLVED || > > > error == net::ERR_INTERNET_DISCONNECTED || > > > error == net::ERR_ADDRESS_UNREACHABLE || > > > error == net::ERR_CONNECTION_TIMED_OUT); > > > > > > and based on the discussion with Paul J, I think we should add > > > ERR_NETWORK_CHANGED. > > > > I'm not sure about those... ERR_CONNECTION_TIMED_OUT and > > ERR_ADDRESS_UNREACHABLE in particular. The latter you often see when upstream > > network devices are down, like your cable modem rebooting. Timed out you > often > > see on random connection hiccups, or site hiccups. Also, if things are timing > > out, at least you're not really hammering the network trying to reload. > > I'm confused; isn't modem rebooting and random connection hiccups exactly the > situations in which we want to auto-reload with an exponential backoff? Sorry...I misread your comment (I missed the whole invert the whole "offline errors" thing). Other errors that can indicate disconnection: ERR_TIMED_OUT, ERR_CONNECTION_RESET_BY_PEER. I did an experiment where I reloaded the instant we got certain errors, and recorded how often retrying worked, but unfortunately I don't still have that list. I think there may have been one other where reloading instantly often worked (Which is not what this is doing, but if reloading instantly works, reloading on a timer should as well).
On 2014/01/30 22:26:19, mmenke wrote: > On 2014/01/30 20:57:30, rdsmith wrote: > > > https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... > > File chrome/renderer/net/net_error_helper_core.cc (right): > > > > > https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... > > chrome/renderer/net/net_error_helper_core.cc:31: return error.reason != > > net::ERR_ABORTED; > > On 2014/01/30 20:47:31, mmenke wrote: > > > On 2014/01/30 20:14:47, rdsmith wrote: > > > > I believe we just want to reload on "offline like" errors, which I > currently > > > > believe are: > > > > > > > > return (error == net::ERR_NAME_NOT_RESOLVED || > > > > error == net::ERR_INTERNET_DISCONNECTED || > > > > error == net::ERR_ADDRESS_UNREACHABLE || > > > > error == net::ERR_CONNECTION_TIMED_OUT); > > > > > > > > and based on the discussion with Paul J, I think we should add > > > > ERR_NETWORK_CHANGED. > > > > > > I'm not sure about those... ERR_CONNECTION_TIMED_OUT and > > > ERR_ADDRESS_UNREACHABLE in particular. The latter you often see when > upstream > > > network devices are down, like your cable modem rebooting. Timed out you > > often > > > see on random connection hiccups, or site hiccups. Also, if things are > timing > > > out, at least you're not really hammering the network trying to reload. > > > > I'm confused; isn't modem rebooting and random connection hiccups exactly the > > situations in which we want to auto-reload with an exponential backoff? > > Sorry...I misread your comment (I missed the whole invert the whole "offline > errors" thing). > > Other errors that can indicate disconnection: ERR_TIMED_OUT, > ERR_CONNECTION_RESET_BY_PEER. I did an experiment where I reloaded the instant > we got certain errors, and recorded how often retrying worked, but unfortunately > I don't still have that list. I think there may have been one other where > reloading instantly often worked (Which is not what this is doing, but if > reloading instantly works, reloading on a timer should as well). I'm not certain that we don't want our initial reload delay to be 0. I think that would be a nice subject for experimentation after we land the basic change. And yeah, sounds like we should re-run the experiment you mention and see what we find from different errors.
On 2014/01/30 22:28:21, rdsmith wrote: > On 2014/01/30 22:26:19, mmenke wrote: > > On 2014/01/30 20:57:30, rdsmith wrote: > > > > > > https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... > > > File chrome/renderer/net/net_error_helper_core.cc (right): > > > > > > > > > https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... > > > chrome/renderer/net/net_error_helper_core.cc:31: return error.reason != > > > net::ERR_ABORTED; > > > On 2014/01/30 20:47:31, mmenke wrote: > > > > On 2014/01/30 20:14:47, rdsmith wrote: > > > > > I believe we just want to reload on "offline like" errors, which I > > currently > > > > > believe are: > > > > > > > > > > return (error == net::ERR_NAME_NOT_RESOLVED || > > > > > error == net::ERR_INTERNET_DISCONNECTED || > > > > > error == net::ERR_ADDRESS_UNREACHABLE || > > > > > error == net::ERR_CONNECTION_TIMED_OUT); > > > > > > > > > > and based on the discussion with Paul J, I think we should add > > > > > ERR_NETWORK_CHANGED. > > > > > > > > I'm not sure about those... ERR_CONNECTION_TIMED_OUT and > > > > ERR_ADDRESS_UNREACHABLE in particular. The latter you often see when > > upstream > > > > network devices are down, like your cable modem rebooting. Timed out you > > > often > > > > see on random connection hiccups, or site hiccups. Also, if things are > > timing > > > > out, at least you're not really hammering the network trying to reload. > > > > > > I'm confused; isn't modem rebooting and random connection hiccups exactly > the > > > situations in which we want to auto-reload with an exponential backoff? > > > > Sorry...I misread your comment (I missed the whole invert the whole "offline > > errors" thing). > > > > Other errors that can indicate disconnection: ERR_TIMED_OUT, > > ERR_CONNECTION_RESET_BY_PEER. I did an experiment where I reloaded the > instant > > we got certain errors, and recorded how often retrying worked, but > unfortunately > > I don't still have that list. I think there may have been one other where > > reloading instantly often worked (Which is not what this is doing, but if > > reloading instantly works, reloading on a timer should as well). > > I'm not certain that we don't want our initial reload delay to be 0. I think > that would be a nice subject for experimentation after we land the basic change. > > And yeah, sounds like we should re-run the experiment you mention and see what > we find from different errors. We could reload on everything at first on canary/dev, see what recovers, and then limit the list.
Also: What's the POST plan?
https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... File chrome/renderer/net/net_error_helper.cc (right): https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper.cc:216: // MockableOneShotTimer implementation in terms of base::OneShotTimer On 2014/01/30 20:14:47, rdsmith wrote: > This feels like it should be in its own file, not in net_error_helper.cc, just > so that other classes can use it if they want. Done. https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper.cc:253: NetErrorHelperCore::MockableOneShotTimer* NetErrorHelper::NewMockableOneShotTimer() { On 2014/01/30 20:14:47, rdsmith wrote: > nit: Line too long? Done. https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:129: auto_reload_enabled_(false), On 2014/01/30 20:14:47, rdsmith wrote: > Why not attach this to the flag setup for this work in > https://codereview.chromium.org/113133004/? Done. https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:225: if (IsReloadableError(error)) On 2014/01/30 20:45:07, mmenke wrote: > Don't want to auto reload POSTs. Done. https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:225: if (IsReloadableError(error)) On 2014/01/30 20:45:07, mmenke wrote: > May make more sense to trigger this when the error page finishes loading (That > is, OnDidFinishLoading, when there's a committed_error_page_info_, no link > doctor load to start, and no pending_error_page_info_). Done. https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:346: if (!committed_error_page_info_) { On 2014/01/30 20:45:07, mmenke wrote: > Is it possible for this to trigger because it's taking the first error page too > long to load? No. The timer is only restarted in GetErrorHTML. https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:353: void NetErrorHelperCore::StartAutoReload() { On 2014/01/30 20:45:07, mmenke wrote: > StartAutoReloadTimer? Done. https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:354: int delay = 1 << auto_reload_count_; On 2014/01/30 20:45:07, mmenke wrote: > I suggest making the 1 a constant at the top of the file, and make make it in > milliseconds. Moved it into a helper. https://codereview.chromium.org/136203009/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:363: return auto_reload_count_ != 0; On 2014/01/30 20:45:07, mmenke wrote: > This isn't correct in the OnStop() case. Yeah. Hm.
A lot of comments, but all very nitty. I'm happy with the overall architecture. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/chrome_c... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/chrome_c... chrome/renderer/chrome_content_renderer_client.cc:355: command_line->HasSwitch(switches::kEnableOfflineAutoReload); Don't you have to explicitly make sure this switch is passed to the renderer? I thought there was a filter somewhere. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/mock... File chrome/renderer/net/mockable_one_shot_timer.cc (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/mock... chrome/renderer/net/mockable_one_shot_timer.cc:17: const base::Closure& user_task) { DCHECK(!user_task.is_empty())? (Or is_null, whichever it is) https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/mock... File chrome/renderer/net/mockable_one_shot_timer.h (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/mock... chrome/renderer/net/mockable_one_shot_timer.h:17: const base::Closure& user_task); Maybe just task? "user" seems ambiguous. Same for the MockOneShotTimer. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/mock... chrome/renderer/net/mockable_one_shot_timer.h:20: private: nit: Linebreak before private. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/mock... chrome/renderer/net/mockable_one_shot_timer.h:21: void Fired(); Blank line between methofs and values. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/mock... chrome/renderer/net/mockable_one_shot_timer.h:23: base::Closure user_task_; DISALLOW_COPY_AND_ASSIGN + relevant header? https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper.h (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper.h:88: scoped_ptr<content::ResourceFetcher> auto_reload_page_fetcher_; Not used. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:22: base::TimeDelta GetAutoReloadBackoff(size_t reloads) { I suggest reload_count or num_reloads. I generally think using plurals for counters can be confusing. reload_count also matches the caller a bit more closely. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:22: base::TimeDelta GetAutoReloadBackoff(size_t reloads) { I suggest "GetAutoReloadTime" or "GetAutoReloadTimeWithBackoff", since it returns a time, not a "backoff". https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:39: bool IsReloadableError(const blink::WebURLError& error) { You should check the error domain, too, rather than assume it's net. It maybe also be "http" (There's also a DNS domain, but I don't think we ever pass a DNS error here, we always just pass those errors directly to the code to generate an error page) https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:162: auto_reload_timer_->Stop(); Can't these two lines just be replaced with "auto_reload_timer_.reset()"? https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:162: auto_reload_timer_->Stop(); auto_reload_count_ = 0;? https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:382: // start over nit: Start over. (Per style guide, comments should be complete sentences) https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core.h (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.h:115: virtual bool IsAutoReloading() const; Neither of these needs to be virtual. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.h:115: virtual bool IsAutoReloading() const; Suggest calling this IsAutoReloadingForTesting(). https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.h:150: int auto_reload_count_; Think this needs a comment - it's not the number of times we've reloaded, but the number of times we've started the timer. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.h:152: void AutoReload(); Methods should be declared before member variables. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.h:152: void AutoReload(); Suggest calling this "Reload" instead, since it doesn't directly do anything automatically. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:67: class MockOneShotTimer : public MockableOneShotTimer { Wonder if we should put this in its own file, and put them in a more accessible location. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:74: const base::Closure& user_task) { EXPECT_FALSE(running_) https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:88: virtual void Fire() { EXPECT_TRUE(running_) https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:95: bool running_; Style guide rather discourages public variables in classes. Just add accessors as needed. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:153: void SuccessLoad() { Maybe RunSuccessfulLoad()? DoSuccessfulLoad()? MockSuccessfulLoad()? Method names should generally be verbs. Same goes for ErrorLoad. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:198: auto_reloading_ = true; Having this and core()->IsAutoReloading is very confusing. I suggest calling this is_reloading_. It's also never reset, which seems weird. It's a pain, but maybe make all calls to core() go through the test fixture, and reset it on commit or something? Open to other ideas. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:198: auto_reloading_ = true; Suggest keeping track of how often this is called, and making sure there are no extra reloads. Could do the same for timers, but I think that's overkill. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1385: EXPECT_FALSE(auto_reloading()); EXPECT_FALSE(core().IsAutoReloading());? https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1403: EXPECT_FALSE(last_one_shot_timer()->IsRunning()); EXPECT_FALSE(core().IsAutoReloading());? (At the end of all of these, or true on the ones where it's running) https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1410: base::TimeDelta delay; Can declare this on the line where you set it. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1413: delay = last_one_shot_timer()->delay_; Maybe first_delay? https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1423: EXPECT_NE(last_one_shot_timer()->delay_, delay); EXPECT_GT? https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1426: TEST_F(NetErrorHelperCoreTest, AutoReloadStopsOnSuccess) { This seems to be a subset of AutoReloadSucceeds https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1448: core().OnStop(); This doesn't seem to test anything. auto_reloading() will still be true at the end of this test. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1458: EXPECT_EQ(last_one_shot_timer()->delay_, delay); Should make sure the original timer was stopped and a new one was started, or just make sure the second reload can be triggered, by triggering the time again.. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1461: TEST_F(NetErrorHelperCoreTest, AutoReloadStopsOnOtherLoadStart) { AutoReloadResetsCountOnSuccess assumes this test passes, so I suggest putting it first (Admittedly, tests are now run out of order on the bots, but still think it makes sense to roughly order tests in terms of which tests they are assuming pass). https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1491: } Other test suggestions: No auto reload on network state change after Stop(). Test in combination with a successful Link Doctor load (I...think we use it in this case?) Test in combination with Link Doctor failing to load (Again, I think we'll use it here, with the current code. If not, we probably should, but fine to worry about later, I'm reworking the Link Doctor code anyways). Test in combination with DNS probe results (Display DNS probe results, then reload, then get new DNS probe results, which we should display, rather than the original results.
High level comments: + Update the bug number as per c#2 + I'd like to document in the comments on this CL somewhere what we decided to do about the reload storm case. My memory (could you check me?) is that we decided: ++ We won't handle it in this CL. ++ We will explore it more and either handle it or determine that it's not that big a deal before we enable this feature. ++ We considered trying to hook in with the limiter used for browser start, but decided that that was just two different a situation (throttling loads coming from history), and tentatively decided that the right way to do this was a ResourceThrottle that only let a couple of tabs be loading at a time. + There are a couple of comments/questions about behavior scattered among the other comments; whatever you/we decide to do in that space, I'd like to make sure it's reflected in the CL description, probably top level (right after the initial title line). + How would you feel about a simple browser test for this behavior, just so we have something end-to-end? Thanks! https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/mock... File chrome/renderer/net/mockable_one_shot_timer.h (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/mock... chrome/renderer/net/mockable_one_shot_timer.h:11: class MockableOneShotTimer { It might be worthwhile tossing an email at whoever wrote OneShotTimer to see if they'd mine you making it mockable directly. That would just (sic?) take adding three "virtual" keywords, right? https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper.cc (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper.cc:73: core_.set_auto_reload_enabled(auto_reload_enabled); Why not as part of the constructor? https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper.cc:235: core_.NetworkStateChanged(enabled); Could you make NetErrorHelperCore a RenderProcessObserver directly? I think that wouldn't negatively affect testing any, and would mean you could avoid this delegation. (If Matt disagrees as to the cleanliness of this suggestion, he wins :-}.) https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper.h (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper.h:39: bool auto_reload_enabled); Given that there aren't any tests currently in this CL that test NetErrorHelper (as separate from NetErrorHelperCore), why don't we get rid of this argument and simply initialize NEHC from the value of the command line flag? https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:24: 5000, 30000, 60000, 300000, 600000, 1800000 I'd like to have some UMA as to when we succeed on this. We're almost certainly going to want to be tweaking these constants from data pulled in from the field, so let's plan for that. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:41: /* Why commented out? https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:185: if (frame_type == MAIN_FRAME && !committed_error_page_info_) { I find this cascade of conditionals confusing. Maybe separate out the two cases for clarity? (I.e. if (frame_type != MAIN_FRAME) return; if (!committed_error_page_info_) { auto_reload_count_ = 0; return; } https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:207: StartAutoReloadTimer(); Hmmm. My preference would be to only start the timer if we're online. (Also, if we're offline when we get the error and then go online, I'd rather not wait the five seconds to start the time.) https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:364: delegate_->ReloadPage(); Confirming, just because I'm not sure which of the CLs I've seen fly by have actually landed/are scheduled for landing: this reload will evaporate (i.e. not replace the error page, with the possible user annoyance involved there) if it produces another error page? https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:370: auto_reload_timer_.reset(delegate_->NewMockableOneShotTimer()); It looks to me as if OneShotTimer can be re-used (Stop() followed by Start()). Given that, I don't think the NEHC needs more than one OneShotTimer. Given that, I think it would be a cleaner interface to pass the timer into the constructor, and get rid of the extra delegate interface/method on NEH. WDYT? https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:381: if (online && auto_reload_timer_->IsRunning()) { Should we stop the timer if the change was to offline? https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:67: class MockOneShotTimer : public MockableOneShotTimer { On 2014/02/05 23:31:43, mmenke wrote: > Wonder if we should put this in its own file, and put them in a more accessible > location. The natural location is base, which is a pain to get things in. But agreed it would be a nice thing. Maybe a separate CL, just so debates over where to put it don't hold up this one? https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:88: virtual void Fire() { Doesn't need to be virtual. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1423: EXPECT_NE(last_one_shot_timer()->delay_, delay); Test to confirm we eventually level off (i.e. probing at the saturation code)? https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1440: EXPECT_FALSE(last_one_shot_timer()->IsRunning()); Should we confirm we do/don't (don't remember the behavior) reset the auto_reload_count_ here? https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1467: EXPECT_FALSE(last_one_shot_timer()->IsRunning()); Confirm the right thing has happened to the auto-reload count?
https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/chrome_c... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/chrome_c... chrome/renderer/chrome_content_renderer_client.cc:355: command_line->HasSwitch(switches::kEnableOfflineAutoReload); On 2014/02/05 23:31:43, mmenke wrote: > Don't you have to explicitly make sure this switch is passed to the renderer? I > thought there was a filter somewhere. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/mock... File chrome/renderer/net/mockable_one_shot_timer.cc (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/mock... chrome/renderer/net/mockable_one_shot_timer.cc:17: const base::Closure& user_task) { On 2014/02/05 23:31:43, mmenke wrote: > DCHECK(!user_task.is_empty())? (Or is_null, whichever it is) I don't think one can do that. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/mock... File chrome/renderer/net/mockable_one_shot_timer.h (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/mock... chrome/renderer/net/mockable_one_shot_timer.h:17: const base::Closure& user_task); On 2014/02/05 23:31:43, mmenke wrote: > Maybe just task? "user" seems ambiguous. Same for the MockOneShotTimer. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/mock... chrome/renderer/net/mockable_one_shot_timer.h:20: private: On 2014/02/05 23:31:43, mmenke wrote: > nit: Linebreak before private. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/mock... chrome/renderer/net/mockable_one_shot_timer.h:21: void Fired(); On 2014/02/05 23:31:43, mmenke wrote: > Blank line between methofs and values. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/mock... chrome/renderer/net/mockable_one_shot_timer.h:23: base::Closure user_task_; On 2014/02/05 23:31:43, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN + relevant header? Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper.h (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper.h:88: scoped_ptr<content::ResourceFetcher> auto_reload_page_fetcher_; On 2014/02/05 23:31:43, mmenke wrote: > Not used. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:22: base::TimeDelta GetAutoReloadBackoff(size_t reloads) { On 2014/02/05 23:31:43, mmenke wrote: > I suggest reload_count or num_reloads. I generally think using plurals for > counters can be confusing. reload_count also matches the caller a bit more > closely. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:22: base::TimeDelta GetAutoReloadBackoff(size_t reloads) { On 2014/02/05 23:31:43, mmenke wrote: > I suggest "GetAutoReloadTime" or "GetAutoReloadTimeWithBackoff", since it > returns a time, not a "backoff". Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:39: bool IsReloadableError(const blink::WebURLError& error) { On 2014/02/05 23:31:43, mmenke wrote: > You should check the error domain, too, rather than assume it's net. It maybe > also be "http" (There's also a DNS domain, but I don't think we ever pass a DNS > error here, we always just pass those errors directly to the code to generate an > error page) Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:41: /* On 2014/02/06 16:47:16, rdsmith wrote: > Why commented out? leftover from earlier, oops Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:162: auto_reload_timer_->Stop(); On 2014/02/05 23:31:43, mmenke wrote: > Can't these two lines just be replaced with "auto_reload_timer_.reset()"? Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:185: if (frame_type == MAIN_FRAME && !committed_error_page_info_) { On 2014/02/06 16:47:16, rdsmith wrote: > I find this cascade of conditionals confusing. Maybe separate out the two cases > for clarity? (I.e. > > if (frame_type != MAIN_FRAME) > return; > > > if (!committed_error_page_info_) { > auto_reload_count_ = 0; > return; > } > Aha, good catch. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:207: StartAutoReloadTimer(); On 2014/02/06 16:47:16, rdsmith wrote: > Hmmm. My preference would be to only start the timer if we're online. (Also, > if we're offline when we get the error and then go online, I'd rather not wait > the five seconds to start the time.) Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:370: auto_reload_timer_.reset(delegate_->NewMockableOneShotTimer()); On 2014/02/06 16:47:16, rdsmith wrote: > It looks to me as if OneShotTimer can be re-used (Stop() followed by Start()). > Given that, I don't think the NEHC needs more than one OneShotTimer. Given > that, I think it would be a cleaner interface to pass the timer into the > constructor, and get rid of the extra delegate interface/method on NEH. WDYT? Good idea, I like it. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:381: if (online && auto_reload_timer_->IsRunning()) { On 2014/02/06 16:47:16, rdsmith wrote: > Should we stop the timer if the change was to offline? Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:382: // start over On 2014/02/05 23:31:43, mmenke wrote: > nit: Start over. (Per style guide, comments should be complete sentences) Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core.h (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.h:115: virtual bool IsAutoReloading() const; On 2014/02/05 23:31:43, mmenke wrote: > Neither of these needs to be virtual. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.h:115: virtual bool IsAutoReloading() const; On 2014/02/05 23:31:43, mmenke wrote: > Suggest calling this IsAutoReloadingForTesting(). Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.h:150: int auto_reload_count_; On 2014/02/05 23:31:43, mmenke wrote: > Think this needs a comment - it's not the number of times we've reloaded, but > the number of times we've started the timer. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.h:152: void AutoReload(); On 2014/02/05 23:31:43, mmenke wrote: > Methods should be declared before member variables. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.h:152: void AutoReload(); On 2014/02/05 23:31:43, mmenke wrote: > Suggest calling this "Reload" instead, since it doesn't directly do anything > automatically. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:74: const base::Closure& user_task) { On 2014/02/05 23:31:43, mmenke wrote: > EXPECT_FALSE(running_) Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:88: virtual void Fire() { On 2014/02/05 23:31:43, mmenke wrote: > EXPECT_TRUE(running_) Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:88: virtual void Fire() { On 2014/02/06 16:47:16, rdsmith wrote: > Doesn't need to be virtual. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:95: bool running_; On 2014/02/05 23:31:43, mmenke wrote: > Style guide rather discourages public variables in classes. Just add accessors > as needed. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:153: void SuccessLoad() { On 2014/02/05 23:31:43, mmenke wrote: > Maybe RunSuccessfulLoad()? DoSuccessfulLoad()? MockSuccessfulLoad()? Method > names should generally be verbs. Same goes for ErrorLoad. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:198: auto_reloading_ = true; On 2014/02/05 23:31:43, mmenke wrote: > Suggest keeping track of how often this is called, and making sure there are no > extra reloads. > > Could do the same for timers, but I think that's overkill. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:198: auto_reloading_ = true; On 2014/02/05 23:31:43, mmenke wrote: > Having this and core()->IsAutoReloading is very confusing. I suggest calling > this is_reloading_. It's also never reset, which seems weird. It's a pain, but > maybe make all calls to core() go through the test fixture, and reset it on > commit or something? Open to other ideas. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1385: EXPECT_FALSE(auto_reloading()); On 2014/02/05 23:31:43, mmenke wrote: > EXPECT_FALSE(core().IsAutoReloading());? Removed that function altogether. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1403: EXPECT_FALSE(last_one_shot_timer()->IsRunning()); On 2014/02/05 23:31:43, mmenke wrote: > EXPECT_FALSE(core().IsAutoReloading());? (At the end of all of these, or true > on the ones where it's running) Removed it. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1410: base::TimeDelta delay; On 2014/02/05 23:31:43, mmenke wrote: > Can declare this on the line where you set it. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1413: delay = last_one_shot_timer()->delay_; On 2014/02/05 23:31:43, mmenke wrote: > Maybe first_delay? Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1426: TEST_F(NetErrorHelperCoreTest, AutoReloadStopsOnSuccess) { On 2014/02/05 23:31:43, mmenke wrote: > This seems to be a subset of AutoReloadSucceeds Haha, so it is. Removed. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1448: core().OnStop(); On 2014/02/05 23:31:43, mmenke wrote: > This doesn't seem to test anything. auto_reloading() will still be true at the > end of this test. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1458: EXPECT_EQ(last_one_shot_timer()->delay_, delay); On 2014/02/05 23:31:43, mmenke wrote: > Should make sure the original timer was stopped and a new one was started, or > just make sure the second reload can be triggered, by triggering the time > again.. Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1461: TEST_F(NetErrorHelperCoreTest, AutoReloadStopsOnOtherLoadStart) { On 2014/02/05 23:31:43, mmenke wrote: > AutoReloadResetsCountOnSuccess assumes this test passes, so I suggest putting it > first (Admittedly, tests are now run out of order on the bots, but still think > it makes sense to roughly order tests in terms of which tests they are assuming > pass). Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1467: EXPECT_FALSE(last_one_shot_timer()->IsRunning()); On 2014/02/06 16:47:16, rdsmith wrote: > Confirm the right thing has happened to the auto-reload count? Done. https://codereview.chromium.org/136203009/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1491: } On 2014/02/05 23:31:43, mmenke wrote: > Other test suggestions: > > No auto reload on network state change after Stop(). > > Test in combination with a successful Link Doctor load (I...think we use it in > this case?) > > Test in combination with Link Doctor failing to load (Again, I think we'll use > it here, with the current code. If not, we probably should, but fine to worry > about later, I'm reworking the Link Doctor code anyways). > > Test in combination with DNS probe results (Display DNS probe results, then > reload, then get new DNS probe results, which we should display, rather than the > original results. Done.
Initial comments on the new test in errorpage_browsertest.cc. https://codereview.chromium.org/136203009/diff/220001/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/136203009/diff/220001/base/timer/timer.h#newc... base/timer/timer.h:85: virtual bool IsRunning() const { I'd be a little surprised if this didn't get pushback, since anything going into base does. I'd suggest you line up a base reviewer now and get preliminary feedback about this change--you don't need to pull them into this CL, but at least toss someone an email checking whether they're likely to allow this. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:73: // for testing. Should it be declared const in this class as well, then? and OVERRIDE? https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:414: base::Bind(&ErrorPageAutoReloadTest::RemoveFilters)); No objection, but you probably don't need this, since the process is being torn down. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:418: void NavigateToURLAndWaitForTitle(const GURL& url, I wonder if this is being implemented in enough different places that we should consolidate in a test utils class or something. (No action on your part necessary unless you feel really inspired.) https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:432: static std::string test_schema() { return "http"; } Why not use const char * constants? https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:444: protocol_handler_ = new TestProtocolHandler(test_url(), 2); I'd make the failure threshold configurable, either by the test (best) or by a constant somewhere. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:448: scoped_thing(protocol_handler_); I'd be inclined to just stuff the construction of the scoped pointer into the argument to AddHostnameProtocolHandler, just so you can avoid the temporary variable (and having to name it :-}). You can use PassAs<net::URLRequestJobFactory::ProtocolHandler> to do the cast. Also note that while the comment is true in spirit, because protocol_handler_ is a static variable, it's not really relevant that URLRequestFilter will outlive the class instance, and classes themselves live forever :-} (well, until the process is killed). https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:459: static TestProtocolHandler* protocol_handler_; Why static? It seems like this could simply be a member variable on the class? https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:460: nit: blank line. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:466: NavigateToURLAndWaitForTitle(test_url(), "Test One", 3); Are you going to (or have you and I haven't noticed) do something to short-circuit the timeouts for the browser test?
Quick comments, I'll get to the rest tomorrow, or later today. https://codereview.chromium.org/136203009/diff/220001/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/136203009/diff/220001/base/timer/timer.h#newc... base/timer/timer.h:85: virtual bool IsRunning() const { You don't seem to be making use of these changes anywhere...Are you? https://codereview.chromium.org/136203009/diff/220001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/136203009/diff/220001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1639: command_line->AppendSwitch(switches::kEnableOfflineAutoReload); These are usually done with field trials. Also worry about the effect of this on browser tests. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:38: using net::TestJobInterceptor; nit: Alphabetize these two. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:40: class TestProtocolHandler : public net::URLRequestJobFactory::ProtocolHandler { Think this should have a comment, and maybe a more informative name (FailFirst[N]RequestsProtocolHandler, maybe?) https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:44: failure_threshold_(failure_threshold) {} I think "threshold" is ambiguous. Maybe "requests_to_fail"? https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:49: net::NetworkDelegate* network_delegate) const { nit: OVERRIDE https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:68: int threshold() const { return failure_threshold_; } nit: Don't think this one is needed? https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:403: class ErrorPageAutoReloadTest : public InProcessBrowserTest { Need to update the command line to enable auto-reload in SetUpCommandLine. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:451: test_schema(), test_domain(), scoped_thing.Pass()); Think AddUrlProtocolHandler is simpler, and avoids any weirdness due to favicon requests. Those requests may make this test flaky, as-is. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:469: protocol_handler()->threshold() + 1); Should comment on thread safety here.
https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer.cc (right): https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.cc:4: // MockableOneShotTimer implementation in terms of base::OneShotTimer nit: Comments like this should generally go above class definitions (Also generally shouldn't put comments above all headers - they've very easy to miss there). https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.cc:22: base::Unretained(this))); nit: Think you can merge these two lines. https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer.h (right): https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.h:10: #include <base/timer/timer.h> These should all use quotes. https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.h:20: virtual bool IsRunning() const; Maybe a comment that these all mirror base::OneShotTimer? https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper.cc (right): https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper.cc:73: scoped_ptr<MockableOneShotTimer>(new MockableOneShotTimer())) { nit: Fix indent. https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper.cc:74: core_.set_auto_reload_enabled(auto_reload_enabled); Can you just make this a constructor parameter? https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper.cc:235: bool NetErrorHelper::ShouldSuppressErrorPage(const GURL& url) { Definition order should match declaration order https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.h (right): https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.h:70: explicit NetErrorHelperCore(Delegate* delegate, explicit no longer needed.
I *think* I got everything. PTAL? https://codereview.chromium.org/136203009/diff/220001/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/136203009/diff/220001/base/timer/timer.h#newc... base/timer/timer.h:85: virtual bool IsRunning() const { On 2014/02/13 20:42:17, mmenke wrote: > You don't seem to be making use of these changes anywhere...Are you? Yeah, I did this while I was experimenting and forgot to revert it. Done. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:38: using net::TestJobInterceptor; On 2014/02/13 20:42:17, mmenke wrote: > nit: Alphabetize these two. Done. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:40: class TestProtocolHandler : public net::URLRequestJobFactory::ProtocolHandler { On 2014/02/13 20:42:17, mmenke wrote: > Think this should have a comment, and maybe a more informative name > (FailFirst[N]RequestsProtocolHandler, maybe?) Done. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:44: failure_threshold_(failure_threshold) {} On 2014/02/13 20:42:17, mmenke wrote: > I think "threshold" is ambiguous. Maybe "requests_to_fail"? Done. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:49: net::NetworkDelegate* network_delegate) const { On 2014/02/13 20:42:17, mmenke wrote: > nit: OVERRIDE Done. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:68: int threshold() const { return failure_threshold_; } On 2014/02/13 20:42:17, mmenke wrote: > nit: Don't think this one is needed? It is used in the tests below. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:73: // for testing. On 2014/02/13 18:47:40, rdsmith wrote: > Should it be declared const in this class as well, then? and OVERRIDE? It is declared const here. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:414: base::Bind(&ErrorPageAutoReloadTest::RemoveFilters)); On 2014/02/13 18:47:40, rdsmith wrote: > No objection, but you probably don't need this, since the process is being torn > down. Done. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:418: void NavigateToURLAndWaitForTitle(const GURL& url, On 2014/02/13 18:47:40, rdsmith wrote: > I wonder if this is being implemented in enough different places that we should > consolidate in a test utils class or something. (No action on your part > necessary unless you feel really inspired.) Done. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:432: static std::string test_schema() { return "http"; } On 2014/02/13 18:47:40, rdsmith wrote: > Why not use const char * constants? Done. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:444: protocol_handler_ = new TestProtocolHandler(test_url(), 2); On 2014/02/13 18:47:40, rdsmith wrote: > I'd make the failure threshold configurable, either by the test (best) or by a > constant somewhere. Done. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:448: scoped_thing(protocol_handler_); On 2014/02/13 18:47:40, rdsmith wrote: > I'd be inclined to just stuff the construction of the scoped pointer into the > argument to AddHostnameProtocolHandler, just so you can avoid the temporary > variable (and having to name it :-}). You can use > PassAs<net::URLRequestJobFactory::ProtocolHandler> to do the cast. > > Also note that while the comment is true in spirit, because protocol_handler_ is > a static variable, it's not really relevant that URLRequestFilter will outlive > the class instance, and classes themselves live forever :-} (well, until the > process is killed). Done. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:451: test_schema(), test_domain(), scoped_thing.Pass()); On 2014/02/13 20:42:17, mmenke wrote: > Think AddUrlProtocolHandler is simpler, and avoids any weirdness due to favicon > requests. Those requests may make this test flaky, as-is. I rejiggered this entire class so the handler is installed in the protocol handler itself, mirroring how the other test handlers work. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:459: static TestProtocolHandler* protocol_handler_; On 2014/02/13 18:47:40, rdsmith wrote: > Why static? It seems like this could simply be a member variable on the class? These are all de-staticed now. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:460: On 2014/02/13 18:47:40, rdsmith wrote: > nit: blank line. Done. https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:466: NavigateToURLAndWaitForTitle(test_url(), "Test One", 3); On 2014/02/13 18:47:40, rdsmith wrote: > Are you going to (or have you and I haven't noticed) do something to > short-circuit the timeouts for the browser test? Hm. Currently they aren't short-circuited because the first couple are short enough to not be bothersome, but maybe I should do that... https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:469: protocol_handler()->threshold() + 1); On 2014/02/13 20:42:17, mmenke wrote: > Should comment on thread safety here. Done. https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer.cc (right): https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.cc:4: // MockableOneShotTimer implementation in terms of base::OneShotTimer On 2014/02/14 17:56:10, mmenke wrote: > nit: Comments like this should generally go above class definitions (Also > generally shouldn't put comments above all headers - they've very easy to miss > there). Done. https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.cc:22: base::Unretained(this))); On 2014/02/14 17:56:10, mmenke wrote: > nit: Think you can merge these two lines. Done. https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer.h (right): https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.h:10: #include <base/timer/timer.h> On 2014/02/14 17:56:10, mmenke wrote: > These should all use quotes. Done. https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.h:20: virtual bool IsRunning() const; On 2014/02/14 17:56:10, mmenke wrote: > Maybe a comment that these all mirror base::OneShotTimer? Done. https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper.cc (right): https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper.cc:73: scoped_ptr<MockableOneShotTimer>(new MockableOneShotTimer())) { On 2014/02/14 17:56:10, mmenke wrote: > nit: Fix indent. Done. https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper.cc:74: core_.set_auto_reload_enabled(auto_reload_enabled); On 2014/02/14 17:56:10, mmenke wrote: > Can you just make this a constructor parameter? I did it this way instead so that the test code that constructs these objects only has to specify this if it cares about it, and because I hate proliferating constructor parameters. https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper.cc:235: bool NetErrorHelper::ShouldSuppressErrorPage(const GURL& url) { On 2014/02/14 17:56:10, mmenke wrote: > Definition order should match declaration order Done. https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.h (right): https://codereview.chromium.org/136203009/diff/290001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.h:70: explicit NetErrorHelperCore(Delegate* delegate, On 2014/02/14 17:56:10, mmenke wrote: > explicit no longer needed. Done.
https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:45: class FailFirstNRequestsProtocolHandler This should be in an anonymous namespace below. https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:54: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); Should include base/logging.h for DCHECK https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:55: scoped_ptr<URLRequestJobFactory::ProtocolHandler> scoped_handler(this); Should also probably include the header for scoped_ptr, though we do get it from the methods that take them in as arguments. https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:64: return NULL; This shouldn't be needed. Maybe a DCHECK_EQ instead? https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:84: GURL url_; maybe const? https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:435: protected: nit: I don't think protected really matters for test classes. Also seems arbitrary to make InstallProtocolHandler public and everything else protected. https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:468: IN_PROC_BROWSER_TEST_F(ErrorPageAutoReloadTest, AutoReload) { If we could do it non-racily, I'd love to have a test that checks auto-reload in conjunction with the Link Doctor. However, since we reload on a timer, and Link Doctor involves a double-commit, I don't think that can currently be done. We could allow a variable number of commits, but that has flake written all over it. https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:475: protocol_handler()->requests_to_fail() + 1); nit: Both of these pairs are in the wrong order. Expected should be before actual. https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:475: protocol_handler()->requests_to_fail() + 1); nit: I think this test is clearer if you have: "const int kRequestsToFail = 2;" And then use that instead of 2, 3, and requests_to_fail() (Which you can then get rid of). https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:475: protocol_handler()->requests_to_fail() + 1); On 2014/02/25 19:19:52, Elly Jones wrote: > On 2014/02/13 20:42:17, mmenke wrote: > > Should comment on thread safety here. > > Done. I don't see a comment about it. I just think it's non-obvious that these are updated on one thread and read on another, and depend on requests being created before navigation completes for correctness. https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:978: return true; This isn't right. There's only one ContentRendererClient per renderer process, not per RenderView. You should use NetErrorHelper::Get(render_view->GetMainRenderFrame()) https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:438: MaybeStartAutoReloadTimer(); Use braces for multi-line if blocks, or put the comment before the if, like you do in ShouldSuppressErrorPage (May want to go the latter route, here and above, just to be consistent, but I don't have strong feelings on it). https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:92: task_.Run(); optional: Out of paranoia, could reset the task here, though would have to store it in a temporary for safety. Then could check if it !task_.is_null() wherever you check running_ (Could optionally get rid of running_ as well) https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:144: protected: Don't think there's any point of using protected in test fixtures. https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1388: core().set_auto_reload_enabled(false); Should this be in the test fixture? Not exactly a huge concern, just seems like one less thing to worry about. https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1470: EXPECT_EQ(0, core().auto_reload_count()); Think this test could use a couple more auto_reload_count checks. A couple of the above tests use it as a proxy for checking delay without starting another request, and it seems like one should explicitly test both together. This seems the best test for that. https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1496: TEST_F(NetErrorHelperCoreTest, AutoReloadStopsOnOffline) { I know it's a lot of tests for this, but I'd like a test of the full online->offline->online transition. Could just add another online transition at the end of this one, or make another test just like it.
The UMA looks good; thanks for adding it. As noted above, could you make the first paragraph of the issue description be a description of the chrome behavior change that this CL implements? The implementation notes are great, but the first thing people will want to know is what it does. This is the last review I'll be able to do on this CL for a couple of weeks, by which time it'll hopefully be committed. I hear by hand my stamp over to Matt. Matt: Could you read through these comments, and drive whichever ones you agree with with Elly? Thanks! https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/136203009/diff/220001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:73: // for testing. On 2014/02/25 19:19:52, Elly Jones wrote: > On 2014/02/13 18:47:40, rdsmith wrote: > > Should it be declared const in this class as well, then? and OVERRIDE? > > It is declared const here. Huh. Weird. I have no idea what I meant by that comment. https://codereview.chromium.org/136203009/diff/360001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/136203009/diff/360001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1989: "enable-offline-auto-reload", This is the second definition of this flag; I created the first one up above for this purpose. There should (must? :-}) be only one. https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:978: return true; On 2014/02/25 22:09:52, mmenke wrote: > This isn't right. There's only one ContentRendererClient per renderer process, > not per RenderView. > > You should use NetErrorHelper::Get(render_view->GetMainRenderFrame()) +1. https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper.cc (right): https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper.cc:74: core_.set_auto_reload_enabled(auto_reload_enabled); See earlier comment; why not directly into the constructor? https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper.h (right): https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper.h:39: bool auto_reload_enabled); I had a series of questions on how auto_reload_enabled is plumbed into NetErrorHelper core that you never responded to, and that (at least based on this comment) it looks like you didn't evaluate. I think they were: * Why pass it into NetErrorHelper instead of NEH getting it direct from the command line? I don't believe we have any NEH level tests, so this is just extra verbiage in the API. * Why use a setter on NEHC rather than pass it in in the constructor? * Why not make NEHC a RenderProcessObserver instead of mapping that through NEH? I don't think it decreases testability any, and it decreases code cruft. (Check this with Matt, and if he doesn't like it, he wins.) https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:213: HISTOGRAM_COUNTS("Net.AutoReload.CountAtSuccess", auto_reload_count_); Suggestion: I'd be inclined to move this histogram up to be with the other ones so that we can reason about them together. My belief is that we'll usually succeed after commit, if the original error resulted in a pre-commit failure, and that for both code simplicity and histogram consistency reasons we're better off taking the small inaccuracy of moving the histogram above. You could put another one in to detect the Commit/Finish distinction if you'd like. https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1519: } Suggestion: A test that confirms that we eventually level off in our exponential backoff timing. https://codereview.chromium.org/136203009/diff/360001/content/test/net/url_re... File content/test/net/url_request_mock_http_job.cc (right): https://codereview.chromium.org/136203009/diff/360001/content/test/net/url_re... content/test/net/url_request_mock_http_job.cc:127: } Why?
https://codereview.chromium.org/136203009/diff/360001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/136203009/diff/360001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1989: "enable-offline-auto-reload", On 2014/02/26 00:07:38, rdsmith wrote: > This is the second definition of this flag; I created the first one up above for > this purpose. There should (must? :-}) be only one. Done. https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:45: class FailFirstNRequestsProtocolHandler On 2014/02/25 22:09:52, mmenke wrote: > This should be in an anonymous namespace below. Done. https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:54: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); On 2014/02/25 22:09:52, mmenke wrote: > Should include base/logging.h for DCHECK Done. https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:55: scoped_ptr<URLRequestJobFactory::ProtocolHandler> scoped_handler(this); On 2014/02/25 22:09:52, mmenke wrote: > Should also probably include the header for scoped_ptr, though we do get it from > the methods that take them in as arguments. Done. https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:64: return NULL; On 2014/02/25 22:09:52, mmenke wrote: > This shouldn't be needed. Maybe a DCHECK_EQ instead? Done. https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:84: GURL url_; On 2014/02/25 22:09:52, mmenke wrote: > maybe const? Done. https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:435: protected: On 2014/02/25 22:09:52, mmenke wrote: > nit: I don't think protected really matters for test classes. Also seems > arbitrary to make InstallProtocolHandler public and everything else protected. Done. https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:468: IN_PROC_BROWSER_TEST_F(ErrorPageAutoReloadTest, AutoReload) { On 2014/02/25 22:09:52, mmenke wrote: > If we could do it non-racily, I'd love to have a test that checks auto-reload in > conjunction with the Link Doctor. However, since we reload on a timer, and Link > Doctor involves a double-commit, I don't think that can currently be done. We > could allow a variable number of commits, but that has flake written all over > it. Yeah. I thought about this, but my mind recoiled, so I decided not to do it. https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:475: protocol_handler()->requests_to_fail() + 1); On 2014/02/25 22:09:52, mmenke wrote: > nit: Both of these pairs are in the wrong order. Expected should be before > actual. Done. https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:475: protocol_handler()->requests_to_fail() + 1); On 2014/02/25 22:09:52, mmenke wrote: > nit: I think this test is clearer if you have: > > "const int kRequestsToFail = 2;" > > And then use that instead of 2, 3, and requests_to_fail() (Which you can then > get rid of). Done. https://codereview.chromium.org/136203009/diff/360001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:475: protocol_handler()->requests_to_fail() + 1); On 2014/02/25 22:09:52, mmenke wrote: > On 2014/02/25 19:19:52, Elly Jones wrote: > > On 2014/02/13 20:42:17, mmenke wrote: > > > Should comment on thread safety here. > > > > Done. > > I don't see a comment about it. I just think it's non-obvious that these are > updated on one thread and read on another, and depend on requests being created > before navigation completes for correctness. Done. https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:978: return true; On 2014/02/25 22:09:52, mmenke wrote: > This isn't right. There's only one ContentRendererClient per renderer process, > not per RenderView. > > You should use NetErrorHelper::Get(render_view->GetMainRenderFrame()) Done. https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:978: return true; On 2014/02/26 00:07:38, rdsmith wrote: > On 2014/02/25 22:09:52, mmenke wrote: > > This isn't right. There's only one ContentRendererClient per renderer > process, > > not per RenderView. > > > > You should use NetErrorHelper::Get(render_view->GetMainRenderFrame()) > > +1. Done. https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper.cc (right): https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper.cc:74: core_.set_auto_reload_enabled(auto_reload_enabled); On 2014/02/26 00:07:38, rdsmith wrote: > See earlier comment; why not directly into the constructor? so test fixtures can construct the NetErrorHelperCore and individual tests can enable/disable autoreload as needed. https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper.h (right): https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper.h:39: bool auto_reload_enabled); On 2014/02/26 00:07:38, rdsmith wrote: > I had a series of questions on how auto_reload_enabled is plumbed into > NetErrorHelper core that you never responded to, and that (at least based on > this comment) it looks like you didn't evaluate. I think they were: > > * Why pass it into NetErrorHelper instead of NEH getting it direct from the > command line? I don't believe we have any NEH level tests, so this is just > extra verbiage in the API. Done. > * Why use a setter on NEHC rather than pass it in in the constructor? That way it's toggle-able by individual tests after the NEHC constructor is already run. > * Why not make NEHC a RenderProcessObserver instead of mapping that through NEH? > I don't think it decreases testability any, and it decreases code cruft. > (Check this with Matt, and if he doesn't like it, he wins.) I talked to Matt and he nacked this. In particular, NEHC is intended to have *no* external dependencies; making it a RenderProcessObserver entails constructing a mock RenderProcess for it to observe when unit-testing it. Now that you mention it, I don't see how NetErrorHelper actually gets attached as an observer of the RenderThread... https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:213: HISTOGRAM_COUNTS("Net.AutoReload.CountAtSuccess", auto_reload_count_); On 2014/02/26 00:07:38, rdsmith wrote: > Suggestion: I'd be inclined to move this histogram up to be with the other ones > so that we can reason about them together. My belief is that we'll usually > succeed after commit, if the original error resulted in a pre-commit failure, > and that for both code simplicity and histogram consistency reasons we're better > off taking the small inaccuracy of moving the histogram above. You could put > another one in to detect the Commit/Finish distinction if you'd like. Done. https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:438: MaybeStartAutoReloadTimer(); On 2014/02/25 22:09:52, mmenke wrote: > Use braces for multi-line if blocks, or put the comment before the if, like you > do in ShouldSuppressErrorPage (May want to go the latter route, here and above, > just to be consistent, but I don't have strong feelings on it). Done. https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:92: task_.Run(); On 2014/02/25 22:09:52, mmenke wrote: > optional: Out of paranoia, could reset the task here, though would have to > store it in a temporary for safety. Then could check if it !task_.is_null() > wherever you check running_ (Could optionally get rid of running_ as well) I don't understand why we'd want to do this, I guess - the task is going to be set every time Start() is called anyway. https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:144: protected: On 2014/02/25 22:09:52, mmenke wrote: > Don't think there's any point of using protected in test fixtures. Done. https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1388: core().set_auto_reload_enabled(false); On 2014/02/25 22:09:52, mmenke wrote: > Should this be in the test fixture? Not exactly a huge concern, just seems like > one less thing to worry about. Done. https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1470: EXPECT_EQ(0, core().auto_reload_count()); On 2014/02/25 22:09:52, mmenke wrote: > Think this test could use a couple more auto_reload_count checks. A couple of > the above tests use it as a proxy for checking delay without starting another > request, and it seems like one should explicitly test both together. This seems > the best test for that. Done. https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1496: TEST_F(NetErrorHelperCoreTest, AutoReloadStopsOnOffline) { On 2014/02/25 22:09:52, mmenke wrote: > I know it's a lot of tests for this, but I'd like a test of the full > online->offline->online transition. Could just add another online transition at > the end of this one, or make another test just like it. Done. https://codereview.chromium.org/136203009/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1519: } On 2014/02/26 00:07:38, rdsmith wrote: > Suggestion: A test that confirms that we eventually level off in our exponential > backoff timing. Done. https://codereview.chromium.org/136203009/diff/360001/content/test/net/url_re... File content/test/net/url_request_mock_http_job.cc (right): https://codereview.chromium.org/136203009/diff/360001/content/test/net/url_re... content/test/net/url_request_mock_http_job.cc:127: } On 2014/02/26 00:07:38, rdsmith wrote: > Why? Mistakes were made.
A bunch of nits. My only real concern is fragility in the logic about when we're running the timer/doing reloads and when we're not. https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:48: using net::TestJobInterceptor; Not used. https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:68: scoped_handler.Pass()); nit: url_, should appear on next line. https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:74: DCHECK_EQ(request->url(), url_); nit: Think url_ should go first, since that's closer to the the "expected" value than the "actual" value. https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:80: net::ERR_CONNECTION_RESET); May want to comment somewhere that this error won't bring up the LinkDoctor. https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:95: // for testing. nit: Capitalize "these" https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:504: virtual void SetUpCommandLine(CommandLine* command_line) { OVERRIDE https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:510: requests_to_fail); nit: Move "url" to next line. https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:517: base::Unretained(this))); nit: Align base with the & https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:547: } This is no longer called anywhere, so should get rid of it. (I rather like the idea of cleaning up like this, but almost no other tests do it, and should probably be consistent). https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:560: // racey. nit: Don't use "we" in comments, due to ambiguity. Can just use "we read them" -> "they are read" or somesuch. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:992: && NetErrorHelper::Get(render_frame)->ShouldSuppressErrorPage(url)) Optional: I think it's cleaner not to have the main frame check here. I don't think it's the content renderer client's responsibility to know that the NetErrorHelper only suppresses reloads for the main frame. Think that responsibility actually belongs to the NetErrorHelperCore, which is where all the other main frame checks live. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:993: return true; nit: Use braces when either part of an if takes more than one line. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.h (right): https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.h:19: class NetErrorHelper; no longer needed. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer.cc (right): https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.cc:11: task_.Run(); Per comment in the unit test file, I think we should also make a copy of task in a local, and reset the member variable it before running it. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer.h (right): https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.h:15: virtual ~MockableOneShotTimer(); optional: May want a blank line after the destructor (Seems more common than not). https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper.cc (right): https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper.cc:7: #include <string> nit: Blank line after standard C++ header includes. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:192: auto_reload_pending_) { Think this should be lined up with committed https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:194: HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtSuccess", To upload the histograms, you need to use the UMA_blah macros. These just update about:histograms (Why anyone would want that, I'm not sure). https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:213: auto_reload_pending_ = false; Hmm...Would it make sense to move these to ErrorPageInfo? Seems like they're associated with a particular error page, much like everything else. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:227: } else if (auto_reload_enabled_) { We don't want to do this if there's a pending load, right? Should also test this. The simplest solution would be to move these into ErrorPageInfo, and then set auto_reload_enabled_ to false for the committed load, if it's not finished loading (This is, admittedly, a little weird and not a perfect solution). https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:396: return false; I think having auto_reload_pending_ for a non-reloadable error code, or a failed post, is very misleading. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:414: base::Unretained(this))); Believe this should be lined up with the &. Or on the previous line, if it fits. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:426: // TODO(rdsmith): prevent the reload storm. Method level comments generally go with declarations, not definitions. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:435: // If we just got online, maybe start auto-reloading again. nit: --we https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:443: return false; May want to merged this check with the next...Think it's easier to mentally process related checks together. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:445: // If auto_reload_timer_ is still running, this error page isn't from an auto nit: |auto_reload_timer_| (Not required by Google style, but it's used elsewhere in this file, and should be consistent within a file) https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:457: return false; The logic here seems correct in most cases (Except, perhaps, when we autoreload and get an exciting new error, which I don't think is an issue), but all these checks make me worry about fragility. May want to think about making it more robust, but fine for now. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.h (right): https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.h:160: bool auto_reload_pending_; Think this variable is confusing. Maybe can_auto_reload_page_? https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:92: task_.Run(); On 2014/03/03 19:31:07, Elly Jones wrote: > On 2014/02/25 22:09:52, mmenke wrote: > > optional: Out of paranoia, could reset the task here, though would have to > > store it in a temporary for safety. Then could check if it !task_.is_null() > > wherever you check running_ (Could optionally get rid of running_ as well) > > I don't understand why we'd want to do this, I guess - the task is going to be > set every time Start() is called anyway. Suppose the task calls Start. It would overwrite task_, any any parameters it owns... So if we had: task_.Start(..., base::Bind(&Foo::Foo, url)); And then: void Foo::Foo(const url& url) { // Can safely access url here. timer_.Start(...); // Can't safely access url here, since we just deleted the task that owned it. }
https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:48: using net::TestJobInterceptor; On 2014/03/04 17:26:25, mmenke wrote: > Not used. Done. https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:68: scoped_handler.Pass()); On 2014/03/04 17:26:25, mmenke wrote: > nit: url_, should appear on next line. Done. https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:74: DCHECK_EQ(request->url(), url_); On 2014/03/04 17:26:25, mmenke wrote: > nit: Think url_ should go first, since that's closer to the the "expected" > value than the "actual" value. Done. https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:80: net::ERR_CONNECTION_RESET); On 2014/03/04 17:26:25, mmenke wrote: > May want to comment somewhere that this error won't bring up the LinkDoctor. Done. https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:95: // for testing. On 2014/03/04 17:26:25, mmenke wrote: > nit: Capitalize "these" Done. https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:504: virtual void SetUpCommandLine(CommandLine* command_line) { On 2014/03/04 17:26:25, mmenke wrote: > OVERRIDE Done. https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:510: requests_to_fail); On 2014/03/04 17:26:25, mmenke wrote: > nit: Move "url" to next line. Done. https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:517: base::Unretained(this))); On 2014/03/04 17:26:25, mmenke wrote: > nit: Align base with the & Done. https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:547: } On 2014/03/04 17:26:25, mmenke wrote: > This is no longer called anywhere, so should get rid of it. (I rather like the > idea of cleaning up like this, but almost no other tests do it, and should > probably be consistent). Done. https://codereview.chromium.org/136203009/diff/400001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:560: // racey. On 2014/03/04 17:26:25, mmenke wrote: > nit: Don't use "we" in comments, due to ambiguity. Can just use "we read them" > -> "they are read" or somesuch. Done. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:992: && NetErrorHelper::Get(render_frame)->ShouldSuppressErrorPage(url)) On 2014/03/04 17:26:25, mmenke wrote: > Optional: I think it's cleaner not to have the main frame check here. I don't > think it's the content renderer client's responsibility to know that the > NetErrorHelper only suppresses reloads for the main frame. Think that > responsibility actually belongs to the NetErrorHelperCore, which is where all > the other main frame checks live. Done. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:993: return true; On 2014/03/04 17:26:25, mmenke wrote: > nit: Use braces when either part of an if takes more than one line. Done. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.h (right): https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.h:19: class NetErrorHelper; On 2014/03/04 17:26:25, mmenke wrote: > no longer needed. Done. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer.cc (right): https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.cc:11: task_.Run(); On 2014/03/04 17:26:25, mmenke wrote: > Per comment in the unit test file, I think we should also make a copy of task in > a local, and reset the member variable it before running it. Done. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer.h (right): https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.h:15: virtual ~MockableOneShotTimer(); On 2014/03/04 17:26:25, mmenke wrote: > optional: May want a blank line after the destructor (Seems more common than > not). Done. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper.cc (right): https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper.cc:7: #include <string> On 2014/03/04 17:26:25, mmenke wrote: > nit: Blank line after standard C++ header includes. Done. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:192: auto_reload_pending_) { On 2014/03/04 17:26:25, mmenke wrote: > Think this should be lined up with committed Done. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:194: HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtSuccess", On 2014/03/04 17:26:25, mmenke wrote: > To upload the histograms, you need to use the UMA_blah macros. These just > update about:histograms (Why anyone would want that, I'm not sure). Done. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:213: auto_reload_pending_ = false; On 2014/03/04 17:26:25, mmenke wrote: > Hmm...Would it make sense to move these to ErrorPageInfo? Seems like they're > associated with a particular error page, much like everything else. Done. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:227: } else if (auto_reload_enabled_) { On 2014/03/04 17:26:25, mmenke wrote: > We don't want to do this if there's a pending load, right? Should also test > this. > > The simplest solution would be to move these into ErrorPageInfo, and then set > auto_reload_enabled_ to false for the committed load, if it's not finished > loading (This is, admittedly, a little weird and not a perfect solution). Done. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:396: return false; On 2014/03/04 17:26:25, mmenke wrote: > I think having auto_reload_pending_ for a non-reloadable error code, or a failed > post, is very misleading. Done. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:414: base::Unretained(this))); On 2014/03/04 17:26:25, mmenke wrote: > Believe this should be lined up with the &. Or on the previous line, if it > fits. Done. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:426: // TODO(rdsmith): prevent the reload storm. On 2014/03/04 17:26:25, mmenke wrote: > Method level comments generally go with declarations, not definitions. Done. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:435: // If we just got online, maybe start auto-reloading again. On 2014/03/04 17:26:25, mmenke wrote: > nit: --we Done. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:443: return false; On 2014/03/04 17:26:25, mmenke wrote: > May want to merged this check with the next...Think it's easier to mentally > process related checks together. They are now sadly more separate. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:445: // If auto_reload_timer_ is still running, this error page isn't from an auto On 2014/03/04 17:26:25, mmenke wrote: > nit: |auto_reload_timer_| (Not required by Google style, but it's used > elsewhere in this file, and should be consistent within a file) Done. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:457: return false; On 2014/03/04 17:26:25, mmenke wrote: > The logic here seems correct in most cases (Except, perhaps, when we autoreload > and get an exciting new error, which I don't think is an issue), but all these > checks make me worry about fragility. May want to think about making it more > robust, but fine for now. Done. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.h (right): https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.h:160: bool auto_reload_pending_; On 2014/03/04 17:26:25, mmenke wrote: > Think this variable is confusing. Maybe can_auto_reload_page_? Done. https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/136203009/diff/400001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:92: task_.Run(); On 2014/03/04 17:26:25, mmenke wrote: > On 2014/03/03 19:31:07, Elly Jones wrote: > > On 2014/02/25 22:09:52, mmenke wrote: > > > optional: Out of paranoia, could reset the task here, though would have to > > > store it in a temporary for safety. Then could check if it !task_.is_null() > > > wherever you check running_ (Could optionally get rid of running_ as well) > > > > I don't understand why we'd want to do this, I guess - the task is going to be > > set every time Start() is called anyway. > > Suppose the task calls Start. It would overwrite task_, any any parameters it > owns... So if we had: > > task_.Start(..., base::Bind(&Foo::Foo, url)); > > And then: > > void Foo::Foo(const url& url) { > // Can safely access url here. > timer_.Start(...); > // Can't safely access url here, since we just deleted the task that owned it. > } Done.
Elly: Just a heads up that I've landed my Link Doctor CL (For the second time...). Probably best not to sync until it's made it through a cycle on the buildbots.
I expect this will be the last round (May give some more nits, but expect to sign off on it next time) https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer.cc (right): https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.cc:16: task_.Run(); temp_task.Run(); There were no unit tests to check this...Should we have a unit test or two for the real timer? https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:156: committed_error_page_info_->can_auto_reload_page = false; Should probably do this for pending_error_page_info_, too. https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:168: committed_error_page_info_->can_auto_reload_page) { CancelPendingFetches() sets can_auto_reload_page to false now. https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:205: net::GetAllErrorCodesForUma()); nit: +braces https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:230: !committed_error_page_info_->was_failed_post) { Optional: May want to just pass "committed_error_page_info_" in to IsReloadableError, and have it check was_failed_post, too. Just suggest this because you're duplicating the pair of checks here and below. https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:396: return false; optional: Suggest just merging these two into one if. https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:408: void NetErrorHelperCore::StartAutoReloadTimer() { optional: DCHECK(committed_error_page_info_)? We'll crash anyways, so just for documentation purposes. Could also DCHECK can_auto_reload. https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:445: return false; optional: May want to merge this with the above check. https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:92: task_.Run(); Suggest doing the re-entrancy protection here, too.
https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:991: if (NetErrorHelper::Get(render_frame)->ShouldSuppressErrorPage(url)) Need to pass in the render_frame, and have the ErrorHelper check if it's the main frame, an pass that down to the ErrorHelperCode (A bit weird, but that's needed for isolation of dependencies and error logic) https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1387: TEST_F(NetErrorHelperCoreTest, AutoReloadDisabled) { One more thing: None of these check ShouldSuppressErrorPage
https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:991: if (NetErrorHelper::Get(render_frame)->ShouldSuppressErrorPage(url)) On 2014/03/07 15:56:44, mmenke wrote: > Need to pass in the render_frame, and have the ErrorHelper check if it's the > main frame, an pass that down to the ErrorHelperCode (A bit weird, but that's > needed for isolation of dependencies and error logic) The NetErrorHelper already knows what RenderFrame it's observing though? Changed the plumbing further inside NetErrorHelper, at any rate. https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer.cc (right): https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.cc:16: task_.Run(); On 2014/03/07 15:46:17, mmenke wrote: > temp_task.Run(); > > There were no unit tests to check this...Should we have a unit test or two for > the real timer? Fixed. I don't see how one could write a unit test for this that wouldn't be extremely flaky - maybe a variable with a side-effect-ful destructor? https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:156: committed_error_page_info_->can_auto_reload_page = false; On 2014/03/07 15:46:17, mmenke wrote: > Should probably do this for pending_error_page_info_, too. Done. https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:168: committed_error_page_info_->can_auto_reload_page) { On 2014/03/07 15:46:17, mmenke wrote: > CancelPendingFetches() sets can_auto_reload_page to false now. Done. https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:205: net::GetAllErrorCodesForUma()); On 2014/03/07 15:46:17, mmenke wrote: > nit: +braces Done. https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:230: !committed_error_page_info_->was_failed_post) { On 2014/03/07 15:46:17, mmenke wrote: > Optional: May want to just pass "committed_error_page_info_" in to > IsReloadableError, and have it check was_failed_post, too. Just suggest this > because you're duplicating the pair of checks here and below. Done. https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:396: return false; On 2014/03/07 15:46:17, mmenke wrote: > optional: Suggest just merging these two into one if. Done. https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:408: void NetErrorHelperCore::StartAutoReloadTimer() { On 2014/03/07 15:46:17, mmenke wrote: > optional: DCHECK(committed_error_page_info_)? We'll crash anyways, so just for > documentation purposes. > > Could also DCHECK can_auto_reload. Done. https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:445: return false; On 2014/03/07 15:46:17, mmenke wrote: > optional: May want to merge this with the above check. Done. https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:92: task_.Run(); On 2014/03/07 15:46:17, mmenke wrote: > Suggest doing the re-entrancy protection here, too. Done. https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1387: TEST_F(NetErrorHelperCoreTest, AutoReloadDisabled) { On 2014/03/07 15:56:44, mmenke wrote: > One more thing: None of these check ShouldSuppressErrorPage Done.
https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:991: if (NetErrorHelper::Get(render_frame)->ShouldSuppressErrorPage(url)) On 2014/03/10 19:46:51, Elly Jones wrote: > On 2014/03/07 15:56:44, mmenke wrote: > > Need to pass in the render_frame, and have the ErrorHelper check if it's the > > main frame, an pass that down to the ErrorHelperCode (A bit weird, but that's > > needed for isolation of dependencies and error logic) > > The NetErrorHelper already knows what RenderFrame it's observing though? Changed > the plumbing further inside NetErrorHelper, at any rate. Right...but this message may not be coming from the main render frame. https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer.cc (right): https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.cc:16: task_.Run(); On 2014/03/10 19:46:51, Elly Jones wrote: > On 2014/03/07 15:46:17, mmenke wrote: > > temp_task.Run(); > > > > There were no unit tests to check this...Should we have a unit test or two for > > the real timer? > > Fixed. > > I don't see how one could write a unit test for this that wouldn't be extremely > flaky - maybe a variable with a side-effect-ful destructor? See base/timer/timer_unittest.cc
https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:991: if (NetErrorHelper::Get(render_frame)->ShouldSuppressErrorPage(url)) On 2014/03/10 19:49:57, mmenke wrote: > On 2014/03/10 19:46:51, Elly Jones wrote: > > On 2014/03/07 15:56:44, mmenke wrote: > > > Need to pass in the render_frame, and have the ErrorHelper check if it's the > > > main frame, an pass that down to the ErrorHelperCode (A bit weird, but > that's > > > needed for isolation of dependencies and error logic) > > > > The NetErrorHelper already knows what RenderFrame it's observing though? > Changed > > the plumbing further inside NetErrorHelper, at any rate. > > Right...but this message may not be coming from the main render frame. Done. https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer.cc (right): https://codereview.chromium.org/136203009/diff/420001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.cc:16: task_.Run(); On 2014/03/10 19:49:57, mmenke wrote: > On 2014/03/10 19:46:51, Elly Jones wrote: > > On 2014/03/07 15:46:17, mmenke wrote: > > > temp_task.Run(); > > > > > > There were no unit tests to check this...Should we have a unit test or two > for > > > the real timer? > > > > Fixed. > > > > I don't see how one could write a unit test for this that wouldn't be > extremely > > flaky - maybe a variable with a side-effect-ful destructor? > > See base/timer/timer_unittest.cc Done.
https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:993: blink::WebFrame* web_frame = render_frame->GetWebFrame(); optional: May want to get rid of this local variable. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer.h (right): https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.h:29: base::Closure old_task_; old_task_ is not used. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer_unittest.cc (right): https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:16: } // namespace Can't this entire file be put in the anonymous namespace? https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:29: class Incrementer { I think this is very hard to follow. How about something like: class HasWeakPtr : public SupportsWeakPtr<HasWeakPtr> { // Do standard weak ptr setup here. } void RestartTimer(HasWeakPtr* has_weak_ptr, ..., MockableOneShotTimer* timer) { base::WeakPtr<HasWeakPtr> weak_ptr(has_weak_ptr->AsWeakPtr()); timer->Start(...); // If starting the timer did not destroy the callback calling this function, // |has_weak_ptr| will not have been deleted. ASSERT_TRUE(weak_ptr.get()); } And use base::Owned(has_weak_ptr) to call the function. Can grab another weak ptr for has_weak_ptr to make sure it's destroyed, though not sure that's needed (Tests are run with ASAN) https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:31: Incrementer(int* counter) : counter_(counter) {} explicit https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:34: private: Blank line before private. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:35: int* counter_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:40: void AllDone(int* counter) { Do we really need both this and BumpCounter? https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper.cc (right): https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper.cc:74: scoped_ptr<MockableOneShotTimer>(new MockableOneShotTimer())) { Optional: I think it's a little cleaner to to remove MockableOneShotTimer from this file, and have core create it itself. Then add a set_timer_for_testing (Or SetTimerForTesting) to core. I think it's cleaner not to have this class know that detail of Core. If you think this is cleaner, or are paranoid about the timer being started on construction, that's fine. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper.h (right): https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper.h:68: bool ShouldSuppressErrorPage(blink::WebFrame*frame, const GURL& url); nit: Space after * https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:23: LOG(ERROR) << "GetAutoReloadTime: " << reload_count; nit: Remove logging https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:401: StartAutoReloadTimer(); If there's an uncommitted load, we should not do this. Doesn't look like we have any check for that. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:409: GetAutoReloadTime(auto_reload_count_); nit: May it on one line. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.h (right): https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.h:124: int GetAutoReloadCount() const; Think you can just rename this to auto_reload_count and inline it. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1552: // set up the environment to test ShouldSuppressErrorPage nit: Comments should be sentences (set->Set, +period)
Sorry for all the rounds of review, but I still feel this isn't quite ready. Certainly not my intention to hold things up.
https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer_unittest.cc (right): https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:29: class Incrementer { On 2014/03/11 20:07:54, mmenke wrote: > I think this is very hard to follow. How about something like: > > class HasWeakPtr : public SupportsWeakPtr<HasWeakPtr> { > // Do standard weak ptr setup here. > } > > void RestartTimer(HasWeakPtr* has_weak_ptr, ..., MockableOneShotTimer* timer) { > base::WeakPtr<HasWeakPtr> weak_ptr(has_weak_ptr->AsWeakPtr()); > timer->Start(...); > // If starting the timer did not destroy the callback calling this function, > // |has_weak_ptr| will not have been deleted. > ASSERT_TRUE(weak_ptr.get()); > } > > And use base::Owned(has_weak_ptr) to call the function. Can grab another weak > ptr for has_weak_ptr to make sure it's destroyed, though not sure that's needed > (Tests are run with ASAN) Could actually get away with just using a WeakFactory<int>, instead of a new class, though a new class may be clearer.
https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:993: blink::WebFrame* web_frame = render_frame->GetWebFrame(); On 2014/03/11 20:07:54, mmenke wrote: > optional: May want to get rid of this local variable. I inserted it to keep the ShouldSuppressErrorPage line from exceeding 80 chars, which caused ugly wrapping. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer.h (right): https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.h:29: base::Closure old_task_; On 2014/03/11 20:07:54, mmenke wrote: > old_task_ is not used. Done. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer_unittest.cc (right): https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:16: } // namespace On 2014/03/11 20:07:54, mmenke wrote: > Can't this entire file be put in the anonymous namespace? Done. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:29: class Incrementer { On 2014/03/11 20:07:54, mmenke wrote: > I think this is very hard to follow. How about something like: > > class HasWeakPtr : public SupportsWeakPtr<HasWeakPtr> { > // Do standard weak ptr setup here. > } > > void RestartTimer(HasWeakPtr* has_weak_ptr, ..., MockableOneShotTimer* timer) { > base::WeakPtr<HasWeakPtr> weak_ptr(has_weak_ptr->AsWeakPtr()); > timer->Start(...); > // If starting the timer did not destroy the callback calling this function, > // |has_weak_ptr| will not have been deleted. > ASSERT_TRUE(weak_ptr.get()); > } > > And use base::Owned(has_weak_ptr) to call the function. Can grab another weak > ptr for has_weak_ptr to make sure it's destroyed, though not sure that's needed > (Tests are run with ASAN) Done. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:29: class Incrementer { On 2014/03/12 13:19:42, mmenke wrote: > On 2014/03/11 20:07:54, mmenke wrote: > > I think this is very hard to follow. How about something like: > > > > class HasWeakPtr : public SupportsWeakPtr<HasWeakPtr> { > > // Do standard weak ptr setup here. > > } > > > > void RestartTimer(HasWeakPtr* has_weak_ptr, ..., MockableOneShotTimer* timer) > { > > base::WeakPtr<HasWeakPtr> weak_ptr(has_weak_ptr->AsWeakPtr()); > > timer->Start(...); > > // If starting the timer did not destroy the callback calling this function, > > // |has_weak_ptr| will not have been deleted. > > ASSERT_TRUE(weak_ptr.get()); > > } > > > > And use base::Owned(has_weak_ptr) to call the function. Can grab another weak > > ptr for has_weak_ptr to make sure it's destroyed, though not sure that's > needed > > (Tests are run with ASAN) > > Could actually get away with just using a WeakFactory<int>, instead of a new > class, though a new class may be clearer. Done. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:31: Incrementer(int* counter) : counter_(counter) {} On 2014/03/11 20:07:54, mmenke wrote: > explicit Deleted. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:34: private: On 2014/03/11 20:07:54, mmenke wrote: > Blank line before private. Deleted. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:35: int* counter_; On 2014/03/11 20:07:54, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN Deleted. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:40: void AllDone(int* counter) { On 2014/03/11 20:07:54, mmenke wrote: > Do we really need both this and BumpCounter? Deleted. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper.cc (right): https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper.cc:74: scoped_ptr<MockableOneShotTimer>(new MockableOneShotTimer())) { On 2014/03/11 20:07:54, mmenke wrote: > Optional: I think it's a little cleaner to to remove MockableOneShotTimer from > this file, and have core create it itself. Then add a set_timer_for_testing (Or > SetTimerForTesting) to core. > > I think it's cleaner not to have this class know that detail of Core. If you > think this is cleaner, or are paranoid about the timer being started on > construction, that's fine. Done. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper.h (right): https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper.h:68: bool ShouldSuppressErrorPage(blink::WebFrame*frame, const GURL& url); On 2014/03/11 20:07:54, mmenke wrote: > nit: Space after * Done. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:23: LOG(ERROR) << "GetAutoReloadTime: " << reload_count; On 2014/03/11 20:07:54, mmenke wrote: > nit: Remove logging Done. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:401: StartAutoReloadTimer(); On 2014/03/11 20:07:54, mmenke wrote: > If there's an uncommitted load, we should not do this. Doesn't look like we > have any check for that. Hm. I've made OnStartLoad update can_auto_reload_page_ appropriate for the type of the page it's observing a start of. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:409: GetAutoReloadTime(auto_reload_count_); On 2014/03/11 20:07:54, mmenke wrote: > nit: May it on one line. Done. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.h (right): https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.h:124: int GetAutoReloadCount() const; On 2014/03/11 20:07:54, mmenke wrote: > Think you can just rename this to auto_reload_count and inline it. Done. https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/136203009/diff/460001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1552: // set up the environment to test ShouldSuppressErrorPage On 2014/03/11 20:07:54, mmenke wrote: > nit: Comments should be sentences (set->Set, +period) Done.
Quick comments. I'm happy with the MockableOneShotTimer tests. I'll review the rest once you've done the merge. https://codereview.chromium.org/136203009/diff/480001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer_unittest.cc (right): https://codereview.chromium.org/136203009/diff/480001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2014? https://codereview.chromium.org/136203009/diff/480001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:9: #include "testing/gtest/include/gtest/gtest.h" include bind.h? https://codereview.chromium.org/136203009/diff/480001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:18: DISALLOW_COPY_AND_ASSIGN(HasWeakPtr); Include macros.h for this. https://codereview.chromium.org/136203009/diff/480001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:38: TEST(MockableOneShotTimerTest, TimerRuns) { May want a timer stops, just like this, but have a Stop call before RunUntilIdle. https://codereview.chromium.org/136203009/diff/480001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:62: // both RestartTimer() and IncrementCounter() must have run. nit: both -> Both
https://codereview.chromium.org/136203009/diff/480001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer_unittest.cc (right): https://codereview.chromium.org/136203009/diff/480001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/03/12 16:03:46, mmenke wrote: > 2014? Done. https://codereview.chromium.org/136203009/diff/480001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:9: #include "testing/gtest/include/gtest/gtest.h" On 2014/03/12 16:03:46, mmenke wrote: > include bind.h? Done. https://codereview.chromium.org/136203009/diff/480001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:18: DISALLOW_COPY_AND_ASSIGN(HasWeakPtr); On 2014/03/12 16:03:46, mmenke wrote: > Include macros.h for this. Done. https://codereview.chromium.org/136203009/diff/480001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:38: TEST(MockableOneShotTimerTest, TimerRuns) { On 2014/03/12 16:03:46, mmenke wrote: > May want a timer stops, just like this, but have a Stop call before > RunUntilIdle. Done. https://codereview.chromium.org/136203009/diff/480001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer_unittest.cc:62: // both RestartTimer() and IncrementCounter() must have run. On 2014/03/12 16:03:46, mmenke wrote: > nit: both -> Both Done.
Still need to go over the tests, but I think we're getting there. https://codereview.chromium.org/136203009/diff/540001/chrome/browser/errorpag... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/136203009/diff/540001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:147: void AddUrlHandler() { I think the ownership semantics are pretty unclear both here, and where it's called. Suggest adding comments in both places about who takes ownership. Could optionally try updating the interface to make things clearer (static AddUrlHandler(scoped_ptr...) or automatically calling it on construction or something), but I can't think of anything that's a significant improvement. https://codereview.chromium.org/136203009/diff/540001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:148: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); Looks like we aren't including content/public/browser/browser_thread.h - we should. https://codereview.chromium.org/136203009/diff/540001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:155: virtual net::URLRequestJob* MaybeCreateJob( nit: "URLRequestJobFactory::ProtocolHandler implementation" https://codereview.chromium.org/136203009/diff/540001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:739: } Think this is an obsolete function that came back as a result of the merge. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer.cc (right): https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.cc:25: base::Bind(&MockableOneShotTimer::Fired, base::Unretained(this))); Should we include bind.h? callback.h doesn't directly include it. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper.h (right): https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper.h:19: class MockableOneShotTimer; nit: No longer needed. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:312: committed_error_page_info_->error.reason, I think this should be -committed_error_page_info_->error.reason, since net errors are negative? Same for the others as well. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:314: UMA_HISTOGRAM_COUNTS("Net.AutoReload.CountAtStop", auto_reload_count_); Are you going to update histograms.xml in this CL, too? Think it's kinda nice to have them in the same CL as they were added., though this is a pretty large CL already. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:314: UMA_HISTOGRAM_COUNTS("Net.AutoReload.CountAtStop", auto_reload_count_); If the process or RV is killed by quitting chrome or closing the tab, we currently don't log anything... Not sure if logging on destruction would make it to the browser process, anyways, but thought I'd mention it. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:344: can_auto_reload_page_ = false; May want to do this in CancelPendingFetches(), since we do it in both places that call it. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:347: can_auto_reload_page_ = true; Suggest: can_auto_reload_page_ = IsReloadableError(*pending_error_page_info_); https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:393: can_auto_reload_page_ = true; Get rid of can_auto_reload_page_, and replace IsReloadableError(*committed_error_page_info_) with can_auto_reload_page_? If there's no unit test to catch that, we should have one. "That" being: * Error page is committed, but not finished loading. * New load starts * Error page stops loading (I'm not really sure if we actually stop loading the first page or not, but we shouldn't depend on it). The timer should not be started. * New load commits. * New load finishes loading. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:575: if (!committed_error_page_info_ || !can_auto_reload_page_) Can't we DCHECK on these now? https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:577: DCHECK(!pending_error_page_info_)? (Logic elsewhere should make sure this never happens). https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:628: GURL error_url = committed_error_page_info_->error.unreachableURL; Maybe add a TODO about checking the new error code against the old one, too? As-is, this catches manual reloads that result in different errors, I believe. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:632: MaybeStartAutoReloadTimer(); Think this is worth a comment (Something along the lines of the timer is started in OnFinishLoad the first time we get the error, but since subsequent errors don't reload the error page, have to start the timer here for them).
https://codereview.chromium.org/136203009/diff/540001/chrome/browser/errorpag... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/136203009/diff/540001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:147: void AddUrlHandler() { On 2014/03/12 19:02:42, mmenke wrote: > I think the ownership semantics are pretty unclear both here, and where it's > called. Suggest adding comments in both places about who takes ownership. > > Could optionally try updating the interface to make things clearer (static > AddUrlHandler(scoped_ptr...) or automatically calling it on construction or > something), but I can't think of anything that's a significant improvement. Done. https://codereview.chromium.org/136203009/diff/540001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:148: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); On 2014/03/12 19:02:42, mmenke wrote: > Looks like we aren't including content/public/browser/browser_thread.h - we > should. Done. https://codereview.chromium.org/136203009/diff/540001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:155: virtual net::URLRequestJob* MaybeCreateJob( On 2014/03/12 19:02:42, mmenke wrote: > nit: "URLRequestJobFactory::ProtocolHandler implementation" Done. https://codereview.chromium.org/136203009/diff/540001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:739: } On 2014/03/12 19:02:42, mmenke wrote: > Think this is an obsolete function that came back as a result of the merge. Done. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer.cc (right): https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.cc:25: base::Bind(&MockableOneShotTimer::Fired, base::Unretained(this))); On 2014/03/12 19:02:42, mmenke wrote: > Should we include bind.h? callback.h doesn't directly include it. Done. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper.h (right): https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper.h:19: class MockableOneShotTimer; On 2014/03/12 19:02:42, mmenke wrote: > nit: No longer needed. Done. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:312: committed_error_page_info_->error.reason, On 2014/03/12 19:02:42, mmenke wrote: > I think this should be -committed_error_page_info_->error.reason, since net > errors are negative? Same for the others as well. Done. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:314: UMA_HISTOGRAM_COUNTS("Net.AutoReload.CountAtStop", auto_reload_count_); On 2014/03/12 19:02:42, mmenke wrote: > Are you going to update histograms.xml in this CL, too? Think it's kinda nice > to have them in the same CL as they were added., though this is a pretty large > CL already. I was going to avoid that - it adds another layer of needed reviews. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:314: UMA_HISTOGRAM_COUNTS("Net.AutoReload.CountAtStop", auto_reload_count_); On 2014/03/12 19:02:42, mmenke wrote: > If the process or RV is killed by quitting chrome or closing the tab, we > currently don't log anything... Not sure if logging on destruction would make > it to the browser process, anyways, but thought I'd mention it. Done. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:344: can_auto_reload_page_ = false; On 2014/03/12 19:02:42, mmenke wrote: > May want to do this in CancelPendingFetches(), since we do it in both places > that call it. Done. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:347: can_auto_reload_page_ = true; On 2014/03/12 19:02:42, mmenke wrote: > Suggest: > > can_auto_reload_page_ = IsReloadableError(*pending_error_page_info_); Done. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:393: can_auto_reload_page_ = true; On 2014/03/12 19:02:42, mmenke wrote: > Get rid of can_auto_reload_page_, and replace > IsReloadableError(*committed_error_page_info_) with can_auto_reload_page_? > > If there's no unit test to catch that, we should have one. > > "That" being: > * Error page is committed, but not finished loading. > * New load starts > * Error page stops loading (I'm not really sure if we actually stop loading the > first page or not, but we shouldn't depend on it). The timer should not be > started. > * New load commits. > * New load finishes loading. Done. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:575: if (!committed_error_page_info_ || !can_auto_reload_page_) On 2014/03/12 19:02:42, mmenke wrote: > Can't we DCHECK on these now? No, because this can be called from NetworkStateChanged, in which they are not guaranteed to be true. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:577: On 2014/03/12 19:02:42, mmenke wrote: > DCHECK(!pending_error_page_info_)? > > (Logic elsewhere should make sure this never happens). Done. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:628: GURL error_url = committed_error_page_info_->error.unreachableURL; On 2014/03/12 19:02:42, mmenke wrote: > Maybe add a TODO about checking the new error code against the old one, too? > > As-is, this catches manual reloads that result in different errors, I believe. Done. https://codereview.chromium.org/136203009/diff/540001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:632: MaybeStartAutoReloadTimer(); On 2014/03/12 19:02:42, mmenke wrote: > Think this is worth a comment (Something along the lines of the timer is started > in OnFinishLoad the first time we get the error, but since subsequent errors > don't reload the error page, have to start the timer here for them). Done.
Yay! More corner cases! https://codereview.chromium.org/136203009/diff/580001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/580001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:612: MaybeStartAutoReloadTimer(); Hrm...We could have either a pending_error_page, or the current error page may not be committed. I think we also need to check the following as well: !pending_error_page_info_ && committed_error_page_info_ && committed_error_page_info_.is_finished_loading (Then you can also make those if's in MaybeStart... into DCHECKs). Sigh...another pair of unit tests for these two cases?
https://codereview.chromium.org/136203009/diff/580001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/580001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:612: MaybeStartAutoReloadTimer(); On 2014/03/12 20:27:17, mmenke wrote: > Hrm...We could have either a pending_error_page, or the current error page may > not be committed. > > I think we also need to check the following as well: !pending_error_page_info_ > && committed_error_page_info_ && committed_error_page_info_.is_finished_loading > > (Then you can also make those if's in MaybeStart... into DCHECKs). > > Sigh...another pair of unit tests for these two cases? Done.
Won't get to the tests today, but still a corner case left, I think. https://codereview.chromium.org/136203009/diff/620001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/620001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:584: DCHECK(!pending_error_page_info_); I think this needs to be up there with the other checks as well, since we set can_auto_reload_page_ on load start, which is before commit. The problem case: * Normal load starts and fails. * Error load starts, commits, finishes loading. * New normal load starts and fails. * New Error page load starts. Note that we still have a committed_error_page_info_ that has finished loading. * We go online. * This DCHECK fails.
On 2014/03/12 21:38:58, mmenke wrote: > Won't get to the tests today, but still a corner case left, I think. > > https://codereview.chromium.org/136203009/diff/620001/chrome/renderer/net/net... > File chrome/renderer/net/net_error_helper_core.cc (right): > > https://codereview.chromium.org/136203009/diff/620001/chrome/renderer/net/net... > chrome/renderer/net/net_error_helper_core.cc:584: > DCHECK(!pending_error_page_info_); > I think this needs to be up there with the other checks as well, since we set > can_auto_reload_page_ on load start, which is before commit. > > The problem case: > > * Normal load starts and fails. > * Error load starts, commits, finishes loading. > * New normal load starts and fails. > * New Error page load starts. Note that we still have a > committed_error_page_info_ that has finished loading. > * We go online. > * This DCHECK fails. Handled.
content/ lgtm, but please consider moving the timer changes to the base clss https://codereview.chromium.org/136203009/diff/660001/chrome/renderer/net/moc... File chrome/renderer/net/mockable_one_shot_timer.h (right): https://codereview.chromium.org/136203009/diff/660001/chrome/renderer/net/moc... chrome/renderer/net/mockable_one_shot_timer.h:18: virtual void Start(const tracked_objects::Location& posted_from, if OneShotTimer is not suitable for testing, I think we should update the base/ implementation https://codereview.chromium.org/136203009/diff/660001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.h (right): https://codereview.chromium.org/136203009/diff/660001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.h:195: }; disallow copy/assign?
LGTM. I'm of course fine with looking into making the real OneShotTimer mockable in base, though if we go that route, should be a separate CL. https://codereview.chromium.org/136203009/diff/680001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/680001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:298: online_(true), Don't suppose we can get the correct initial value for this? Think it's fine to save that for a followup CL, but should probably at least look into it before enabling by default. If we can't manage it, should probably at least set auto_reload_count_ on the offline -> online transition, instead of / in addition to the online -> offline one (That may make sense, anyways). https://codereview.chromium.org/136203009/diff/680001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:580: pending_error_page_info_) { Can any of these not be true, except in the offline to online case? If not, may want to move the checks down there, and turn them into DCHECKs here. https://codereview.chromium.org/136203009/diff/680001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/136203009/diff/680001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1857: EXPECT_FALSE(timer()->IsRunning()); Check that reload count is 0? https://codereview.chromium.org/136203009/diff/680001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1867: EXPECT_TRUE(timer()->IsRunning()); Check that reload count is 1? https://codereview.chromium.org/136203009/diff/680001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1915: // Start the non-error page load. nit: Maybe "Start the" -> "Start a new" https://codereview.chromium.org/136203009/diff/680001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1923: core().OnFinishLoad(NetErrorHelperCore::MAIN_FRAME); Maybe another "EXPECT_FALSE(timer()->IsRunning());" at the end?
https://codereview.chromium.org/136203009/diff/680001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/680001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:298: online_(true), On 2014/03/13 14:39:13, mmenke wrote: > Don't suppose we can get the correct initial value for this? Think it's fine to > save that for a followup CL, but should probably at least look into it before > enabling by default. NetworkChangeNotifier has this state, but it's only browser-side unfortunately. A quick look did not reveal any IPCs that could convey this state to the renderer or any places where it's kept renderer-side except for inside blink :(. I'll add a TODO. > If we can't manage it, should probably at least set auto_reload_count_ on the > offline -> online transition, instead of / in addition to the online -> offline > one (That may make sense, anyways). Actually, we currently set it at both transitions, I think. https://codereview.chromium.org/136203009/diff/680001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:580: pending_error_page_info_) { On 2014/03/13 14:39:13, mmenke wrote: > Can any of these not be true, except in the offline to online case? If not, may > want to move the checks down there, and turn them into DCHECKs here. Yes: as called in ShouldSuppressErrorPage(), is_finished_loading can be false, and pending_error_page_info_ can be true. As called in OnFinish, pending_error_page_info_ can be true. In general, I want this function to contain its own checks for whether it should call StartAutoReloadTimer and not push those into the callees. https://codereview.chromium.org/136203009/diff/680001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/136203009/diff/680001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1857: EXPECT_FALSE(timer()->IsRunning()); On 2014/03/13 14:39:13, mmenke wrote: > Check that reload count is 0? Done. https://codereview.chromium.org/136203009/diff/680001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1867: EXPECT_TRUE(timer()->IsRunning()); On 2014/03/13 14:39:13, mmenke wrote: > Check that reload count is 1? Done. https://codereview.chromium.org/136203009/diff/680001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1915: // Start the non-error page load. On 2014/03/13 14:39:13, mmenke wrote: > nit: Maybe "Start the" -> "Start a new" Done. https://codereview.chromium.org/136203009/diff/680001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1923: core().OnFinishLoad(NetErrorHelperCore::MAIN_FRAME); On 2014/03/13 14:39:13, mmenke wrote: > Maybe another "EXPECT_FALSE(timer()->IsRunning());" at the end? Done.
https://codereview.chromium.org/136203009/diff/680001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/680001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:298: online_(true), On 2014/03/13 17:56:19, Elly Jones wrote: > On 2014/03/13 14:39:13, mmenke wrote: > > Don't suppose we can get the correct initial value for this? Think it's fine > to > > save that for a followup CL, but should probably at least look into it before > > enabling by default. > > NetworkChangeNotifier has this state, but it's only browser-side unfortunately. > A quick look did not reveal any IPCs that could convey this state to the > renderer or any places where it's kept renderer-side except for inside blink :(. > I'll add a TODO. > > > If we can't manage it, should probably at least set auto_reload_count_ on the > > offline -> online transition, instead of / in addition to the online -> > offline > > one (That may make sense, anyways). > > Actually, we currently set it at both transitions, I think. Ah, you're right...unless we're currently automatically reloading the page (As opposed to just running the timer to do so)... https://codereview.chromium.org/136203009/diff/720001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/136203009/diff/720001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:571: if (!committed_error_page_info_) { Can this happen?
The CQ bit was checked by ellyjones@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ellyjones@chromium.org/136203009/740001
The CQ bit was unchecked by ellyjones@chromium.org
The CQ bit was checked by ellyjones@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ellyjones@chromium.org/136203009/740001
Message was sent while issue was closed.
Change committed as 257254 |
