Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
259613003:
Fix auto-reload histograms. (Closed)
|
Created:
6 years, 8 months ago by Elly Fong-Jones Modified:
6 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Visibility:
Public. |
DescriptionFix auto-reload histograms.
Two bugs, and general state-machine grotesqueness, prevented auto-reload
histograms from working properly in production:
1. Success could only be reported in OnCommitLoad() if can_auto_reload_page_ was
true, but can_auto_reload_page_ would be set to false in OnStartLoad(), since
OnStartLoad() had no way of knowing that the non-error page load it saw starting
was actually part of an attempt to auto-reload the page. This has been patched
around by tracking is_auto_reloading in committed_error_page_info_; when the
auto reload timer fires, it sets is_auto_reloading to indicate to OnCommitLoad
that auto-reload was running. OnCommitLoad can then use is_auto_reloading and
the loading URL to decide whether the committing load represents a success or
failure for auto-reload.
2. Whenever a non-error load started, OnStartLoad() would call
CancelPendingFetches(), intending to prevent auto-reload from replacing the
starting load with its own; as a side-effect, CancelPendingFetches() would
assume that if there was an existing error page which looked auto-reloadable
that the cancel represented a failure of auto-reload and log a histogram entry.
Unfortunately, this logic would trigger at each reload attempt by auto-reload,
inflating the failure count with nonexistent failures. To prevent this
misbehavior, CancelPendingFetches() is now no longer responsible for logging
auto-reload failures; instead, failures are detected and logged in
OnCommitLoad() (for "user navigated elsewhere" failures), OnStop (for "user
pressed Stop to abort auto-reload" failures), and the NetErrorHelperCore
destructor (for "user closed the tab" failures).
To help clean up the code, statistics are now reported through
ReportAutoReloadSuccess() and ReportAutoReloadFailure().
There are new unit tests covering these cases:
1. Auto-reload succeeding on the first attempt;
2. Auto-reload succeeding on the second attempt;
3. The user manually stopping an auto-reload;
4. The user manually stopping a non-auto-reload load;
5. The user navigating to a different URL;
As part of this cleanup, simplify the state machine; the ambiguous "can_auto_reload_page_" flag is gone, replaced by an implicit state bit in whether the timer is running (or paused) or not.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271007
Patch Set 1 #
Total comments: 29
Patch Set 2 : First round of fixes #Patch Set 3 : Fix stat collection #
Total comments: 31
Patch Set 4 : Rework state machine a bit #
Total comments: 76
Patch Set 5 : More unit-test fixes #
Total comments: 42
Patch Set 6 : More error_url() #
Total comments: 2
Patch Set 7 : Rebase #Patch Set 8 : don't DCHECK uncommitted_load_started_ #Patch Set 9 : fix build breaks on win32 and clang #
Total comments: 2
Patch Set 10 : Don't suppress error pages if AR is disabled #Patch Set 11 : support autoreloading only visible tabs #Patch Set 12 : Replace CL #Patch Set 13 : Correctly terminate uncommitted loads #
Messages
Total messages: 37 (0 generated)
https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:294: bool is_auto_reloading; Think triggered_auto_reload would be clearer (Makes it clearer its set on the old ErrorPageInfo, and not the one for the reloaded page, if the reload results in another error page). https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:345: auto_reload_timer_->IsRunning())) { So if a user presses stop while the autoreload timer is running, we count it as an error, but if they trigger a new navigation while it's running, we don't, unless it's a cross-process navigation? https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:402: void NetErrorHelperCore::ReportAutoReloadSuccess(int error, size_t count) { These should either be in a private anonymous namespace or static. Also, they should ignore the case when committed_error_page_info_->error.domain != net::kErrorDomain. I'd suggest passing in the full WebURLError and having them check it themselves, since it's easy to forget about that when mucking with callsites. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:203: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); GURL(kUnreachableWebDataURL) https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:373: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); Error page commits should use GURL(kUnreachableWebDataURL) https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:2031: } // namespace Can we just stick the entire file in an anonymous namespace? https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:2035: virtual void SetUp() { OVERRIDE https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:2046: static const char *kCountAtStop; "static const char kCountAtStop[];" is preferred, I believe. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:2084: TEST_F(NetErrorHelperCoreHistogramTest, AutoReloadHistogramsAtFirstSuccess) { Think the name could be clearer (It is the name of the histogram, admittedly, but the name of the histogram isn't terribly clear). Maybe AutoReloadHistogramsFirstReloadSucceeds? Or just FirstAutoReloadSucceeds, since Histogram is already in the test fixture name. Also suggest renaming the other tests similarly. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:2101: TEST_F(NetErrorHelperCoreHistogramTest, AutoReloadHistogramsAtSuccess) { Suggest renaming this like the first test, but replacing first with second. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:2112: ExpectDelta(kErrorAtFirstSuccess, 0); Out of paranoia, may want to just include all histograms in all these tests. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:2116: // registers as an auto-reload failure if an auto-reload attempt is in flight. Suggest mentioning these are also caused by cross-process navs, in this test and the next. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:2152: kTestUrl); Suggest having this one and the next call DidFinishLoad (OnFinishLoad? OnWillDidHaveSomeDayFinishedLoading? Whatever) https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:2166: default_url()); Maybe a test using the funky error page URL (Mentioned in the first comments), too?
https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:294: bool is_auto_reloading; On 2014/04/25 14:57:42, mmenke wrote: > Think triggered_auto_reload would be clearer (Makes it clearer its set on the > old ErrorPageInfo, and not the one for the reloaded page, if the reload results > in another error page). Done. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:345: auto_reload_timer_->IsRunning())) { On 2014/04/25 14:57:42, mmenke wrote: > So if a user presses stop while the autoreload timer is running, we count it as > an error, but if they trigger a new navigation while it's running, we don't, > unless it's a cross-process navigation? If they trigger a new navigation while it's running, I think we just get another OnStart, right? and then when the different URL commits, we'll record that as an error. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:402: void NetErrorHelperCore::ReportAutoReloadSuccess(int error, size_t count) { On 2014/04/25 14:57:42, mmenke wrote: > These should either be in a private anonymous namespace or static. Also, they > should ignore the case when committed_error_page_info_->error.domain != > net::kErrorDomain. I'd suggest passing in the full WebURLError and having them > check it themselves, since it's easy to forget about that when mucking with > callsites. Done. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:203: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/25 14:57:42, mmenke wrote: > GURL(kUnreachableWebDataURL) Done. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:373: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/25 14:57:42, mmenke wrote: > Error page commits should use GURL(kUnreachableWebDataURL) Done. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:2031: } // namespace On 2014/04/25 14:57:42, mmenke wrote: > Can we just stick the entire file in an anonymous namespace? Done. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:2035: virtual void SetUp() { On 2014/04/25 14:57:42, mmenke wrote: > OVERRIDE Done. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:2046: static const char *kCountAtStop; On 2014/04/25 14:57:42, mmenke wrote: > "static const char kCountAtStop[];" is preferred, I believe. Done. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:2084: TEST_F(NetErrorHelperCoreHistogramTest, AutoReloadHistogramsAtFirstSuccess) { On 2014/04/25 14:57:42, mmenke wrote: > Think the name could be clearer (It is the name of the histogram, admittedly, > but the name of the histogram isn't terribly clear). Maybe > AutoReloadHistogramsFirstReloadSucceeds? Or just FirstAutoReloadSucceeds, since > Histogram is already in the test fixture name. Also suggest renaming the other > tests similarly. Done. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:2101: TEST_F(NetErrorHelperCoreHistogramTest, AutoReloadHistogramsAtSuccess) { On 2014/04/25 14:57:42, mmenke wrote: > Suggest renaming this like the first test, but replacing first with second. Done. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:2112: ExpectDelta(kErrorAtFirstSuccess, 0); On 2014/04/25 14:57:42, mmenke wrote: > Out of paranoia, may want to just include all histograms in all these tests. Done. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:2116: // registers as an auto-reload failure if an auto-reload attempt is in flight. On 2014/04/25 14:57:42, mmenke wrote: > Suggest mentioning these are also caused by cross-process navs, in this test and > the next. Done. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:2152: kTestUrl); On 2014/04/25 14:57:42, mmenke wrote: > Suggest having this one and the next call DidFinishLoad (OnFinishLoad? > OnWillDidHaveSomeDayFinishedLoading? Whatever) Done. https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core_unittest.cc:2166: default_url()); On 2014/04/25 14:57:42, mmenke wrote: > Maybe a test using the funky error page URL (Mentioned in the first comments), > too? Done.
https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... chrome/renderer/net/net_error_helper_core.cc:345: auto_reload_timer_->IsRunning())) { On 2014/04/25 20:01:26, Elly Jones wrote: > On 2014/04/25 14:57:42, mmenke wrote: > > So if a user presses stop while the autoreload timer is running, we count it > as > > an error, but if they trigger a new navigation while it's running, we don't, > > unless it's a cross-process navigation? > > If they trigger a new navigation while it's running, I think we just get another > OnStart, right? and then when the different URL commits, we'll record that as an > error. But autoreload won't have been triggered. We record a failure if: * Page errors out, error page commits, we start the autoreload timer, stop is pressed (Record failure here). But we don't if: * Page errors out, error page commits, we start the autoreload timer, new navigation started by used (stopping the timer), new page commits. auto_reload_triggered is false in the second case (first, too), so we don't record anything.
On 2014/04/25 20:08:03, mmenke wrote: > https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... > File chrome/renderer/net/net_error_helper_core.cc (right): > > https://codereview.chromium.org/259613003/diff/1/chrome/renderer/net/net_erro... > chrome/renderer/net/net_error_helper_core.cc:345: > auto_reload_timer_->IsRunning())) { > On 2014/04/25 20:01:26, Elly Jones wrote: > > On 2014/04/25 14:57:42, mmenke wrote: > > > So if a user presses stop while the autoreload timer is running, we count it > > as > > > an error, but if they trigger a new navigation while it's running, we don't, > > > unless it's a cross-process navigation? > > > > If they trigger a new navigation while it's running, I think we just get > another > > OnStart, right? and then when the different URL commits, we'll record that as > an > > error. > > But autoreload won't have been triggered. We record a failure if: > > * Page errors out, error page commits, we start the autoreload timer, stop is > pressed (Record failure here). > > But we don't if: > > * Page errors out, error page commits, we start the autoreload timer, new > navigation started by used (stopping the timer), new page commits. > > auto_reload_triggered is false in the second case (first, too), so we don't > record anything. I believe this is handled now. Since auto_reload_triggered controls whether we consider autoreload to have succeeded or failed, if we set it earlier (when we first MaybeStartAutoReloadTimer on an eligible page), we get the correct statistics.
https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:415: committed_error_page_info_->auto_reload_triggered) { Right...So whenever we destroy committed_error_page_info_, we update the stats if auto_reload_triggered is true, unless we're committing another error page. Sounds good. https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:417: const GURL& error_url = error.unreachableURL; Does this work? error.unreachableURL is a WebURL, not a GURL.... https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:663: committed_error_page_info_->auto_reload_triggered = true; So if we never reload because we're offline, but the user triggers a reload and it works (while offline? Hrm...), then we count it as a failure? I don't think that's a problem. https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:684: committed_error_page_info_->auto_reload_triggered = true; This isn't needed any more, is it? https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:403: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); GURL(content::kUnreachableWebDataURL) https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:434: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); GURL(content::kUnreachableWebDataURL) https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:460: core().OnCommitLoad(NetErrorHelperCore::SUB_FRAME, default_url()); GURL(content::kUnreachableWebDataURL) https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:484: core().OnCommitLoad(NetErrorHelperCore::SUB_FRAME, default_url()); GURL(content::kUnreachableWebDataURL) Basically all these that are below an "Error page loads" should use that URL. https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:2061: std::map<std::string, int> old_counts_; Should include string and map...Looks like we aren't including gurl.h, either. https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:2080: NULL Can't just use arraysize or ARRAY_SIZE_UNSAFE? I'm fine with this if not. https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:2191: } Could have one where the user reloads manually before autoreload timer fire, and one where they reload manually after we've started the autoreload... https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:2200: core().OnFinishLoad(NetErrorHelperCore::MAIN_FRAME); You're missing the onstart of the error page. Maybe just use DoErrorLoad here again? https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:2200: core().OnFinishLoad(NetErrorHelperCore::MAIN_FRAME); Erm...wait...When the reload fails, we're now suppressing the new error page, aren't we? Erm...In that case, when do we ever restart the timer? I'm confused...
https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:343: UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtStop", Why isn't this using the helper functions? That violates the principle of least astonishment (i.e. a lazy code reviewer, like, say, me :-}, will find the helper functions, scan for them in this file, and miss that there's a histogram entry being created here). Also, we aren't we checking auto_reload_triggered here? https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:415: committed_error_page_info_->auto_reload_triggered) { On 2014/04/28 20:21:14, mmenke wrote: > Right...So whenever we destroy committed_error_page_info_, we update the stats > if auto_reload_triggered is true, unless we're committing another error page. > Sounds good. Why aren't we recording a failure if we're committing another error page? https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:420: else if (url != GURL(content::kUnreachableWebDataURL)) What is the purpose of this test?
PTAL? https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:415: committed_error_page_info_->auto_reload_triggered) { On 2014/04/28 20:21:14, mmenke wrote: > Right...So whenever we destroy committed_error_page_info_, we update the stats > if auto_reload_triggered is true, unless we're committing another error page. > Sounds good. Done. https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:415: committed_error_page_info_->auto_reload_triggered) { On 2014/04/28 21:05:52, rdsmith wrote: > On 2014/04/28 20:21:14, mmenke wrote: > > Right...So whenever we destroy committed_error_page_info_, we update the stats > > if auto_reload_triggered is true, unless we're committing another error page. > > Sounds good. > > Why aren't we recording a failure if we're committing another error page? Because another error page is just a reload attempt that failed. An actual auto-reload failure is when the user navigates somewhere else or closes the tab because auto-reload didn't do its job. https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:417: const GURL& error_url = error.unreachableURL; On 2014/04/28 20:21:14, mmenke wrote: > Does this work? error.unreachableURL is a WebURL, not a GURL.... Yes, because WebURL has a GURL operator() on it. https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:420: else if (url != GURL(content::kUnreachableWebDataURL)) On 2014/04/28 21:05:52, rdsmith wrote: > What is the purpose of this test? To avoid logging a failure if we're just loading a new error page. https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:663: committed_error_page_info_->auto_reload_triggered = true; On 2014/04/28 20:21:14, mmenke wrote: > So if we never reload because we're offline, but the user triggers a reload and > it works (while offline? Hrm...), then we count it as a failure? I don't think > that's a problem. Right. https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:684: committed_error_page_info_->auto_reload_triggered = true; On 2014/04/28 20:21:14, mmenke wrote: > This isn't needed any more, is it? Done. https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:403: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/28 20:21:14, mmenke wrote: > GURL(content::kUnreachableWebDataURL) Done. https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:434: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/28 20:21:14, mmenke wrote: > GURL(content::kUnreachableWebDataURL) Done. https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:460: core().OnCommitLoad(NetErrorHelperCore::SUB_FRAME, default_url()); On 2014/04/28 20:21:14, mmenke wrote: > GURL(content::kUnreachableWebDataURL) Done. https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:484: core().OnCommitLoad(NetErrorHelperCore::SUB_FRAME, default_url()); On 2014/04/28 20:21:14, mmenke wrote: > GURL(content::kUnreachableWebDataURL) > > Basically all these that are below an "Error page loads" should use that URL. Done. https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:2061: std::map<std::string, int> old_counts_; On 2014/04/28 20:21:14, mmenke wrote: > Should include string and map...Looks like we aren't including gurl.h, either. Done. https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:2080: NULL On 2014/04/28 20:21:14, mmenke wrote: > Can't just use arraysize or ARRAY_SIZE_UNSAFE? I'm fine with this if not. I believe not since the array's values (and hence size) aren't yet in scope in StoreOldCounts(). https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:2191: } On 2014/04/28 20:21:14, mmenke wrote: > Could have one where the user reloads manually before autoreload timer fire, and > one where they reload manually after we've started the autoreload... Done. https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:2200: core().OnFinishLoad(NetErrorHelperCore::MAIN_FRAME); On 2014/04/28 20:21:14, mmenke wrote: > You're missing the onstart of the error page. Maybe just use DoErrorLoad here > again? Done. https://codereview.chromium.org/259613003/diff/40001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:2200: core().OnFinishLoad(NetErrorHelperCore::MAIN_FRAME); On 2014/04/28 20:21:14, mmenke wrote: > Erm...wait...When the reload fails, we're now suppressing the new error page, > aren't we? Erm...In that case, when do we ever restart the timer? I'm > confused... ShouldSuppressErrorPage starts the timer if it suppresses the error page load.
This looks pretty reasonable...At least I can't notice any bugs. Would like more tests of the suppressed error page case, though - all the timer tests assume we aren't suppressing error pages. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:365: if (auto_reload_timer_paused_) I don't think either of these if's are needed. Just running their bodies has no negative side effects if false. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:664: DCHECK(IsReloadableError(*committed_error_page_info_)); Suggest moving this into StartAutoReloadTimer. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:666: committed_error_page_info_->auto_reload_triggered = true; Suggest either DCHECKING this is true in STartAutoReloadTimer, or moving this line there. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:676: auto_reload_timer_paused_ = true; Should expand the CL description to include behavior changes. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:696: // Transitioning offline -> online I believe comments for else/ifs generally go inside the block. Think it's confusing when the else clause has a comment before it (Your indentation below is also wrong). https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:698: if (auto_reload_timer_paused_) I don't think we need to check was_online here. Is there any case where auto_reload_timer_paused_ is true, online is true, but was_online is also true? https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:701: } else if (was_online && !online) { Again, do we get anything out of checking was_online here? Seems like we could just remove it, or even DCHECK it in the inner if body. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:703: DCHECK(committed_error_page_info_); Maybe: DCHECK(!auto_reload_timer_paused_)? DCHECK(committed_error_page_info_->auto_reload_triggered)? https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:725: return false; optional: I think it may make more sense to put this first. The other checks are kinda weird when this is NULL. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:726: Optional: Could throw in a DCHECK(committed_error_page_info_->is_auto_reloading) https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:217: GURL(content::kUnreachableWebDataURL)); error_url() https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:341: GURL error_url_; const? (x2) https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:837: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url() https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:961: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url() https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1007: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url() https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1050: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url() https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1087: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url() https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1107: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url() https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1136: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url() https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1160: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url() https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1184: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url() https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1228: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url() https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1243: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url()...etc (Probe pages, blank pages, corrections pages, all should use the error_url) https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1884: TEST_F(NetErrorHelperCoreTest, AutoReloadStopsOnOffline) { This is redundant with AutoReloadRestartsOnOnline now. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1893: TEST_F(NetErrorHelperCoreTest, AutoReloadStopsOnOfflineThenRestartsOnOnline) { this is identical to OnOnline. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1902: } Suggest a test where the offline event is first, and then make sure we start reloading only after the online event. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1956: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url() https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1971: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url() https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1996: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url() https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:2013: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url() https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:2117: DoErrorLoad(net::ERR_CONNECTION_RESET); This is more typically a suppressed error page, rather than a full load, right? https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:2196: core().OnFinishLoad(NetErrorHelperCore::MAIN_FRAME); This is just DoSuccessLoad, right? https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:2228: DoErrorLoad(net::ERR_CONNECTION_RESET); So in this case...The user presses the reload button after the autoreload failed and the error page was suppressed? I think that's not obvious from the test name, so should either fix that or add comments.
LGTM conditional on my "Just confirming" below; everything else is suggestions you can do what you want with, or stuff for another CL. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper.cc (right): https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper.cc:118: core_.OnCommitLoad(GetFrameType(frame), url); I don't personally care, but is there a reason to use the variable rather than just putting frame->document().url() into the OnCommitLoad argument list? https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:336: // TODO(ellyjones): Make online_ accurate at object creation. Right, I had forgotten about this. Doesn't this mean that if you do a navigation while you're offline, you'll cycle through all the backoff intervals (because this class will think it's online), and when you finally do come back online, you'll wait for the largest interval we have before reloading? That seems like a common enough use case that I'd like to handle it somewhat better than this. It also makes me confused about why I've found this feature so useful so quickly; I'm often in the position of starting a navigation while offline and coming online almost immediately after that (as the network initializes). I would think I would have reloaded at 0, and then started backing off, and I'd have to wait a few seconds (which I'd really like to avoid doing; I think a *lot* of the value of this feature is doing the initial attempt at auto-reload when the network state changes). Do you understand what's going on? This isn't a question for this CL (obviously; you didn't change this code) but I think it's fairly high priority for the user experience of navigation-while-offline, come online to be a reload fairly quickly after you come online. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:423: else if (url != GURL(content::kUnreachableWebDataURL)) Just confirming: This will *not* trip if the user initiated a navigation that errored out? I think we want to record an auto-reload failure in that case, and I don't see that happening on OnStartLoad(). https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core.h (right): https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.h:216: bool auto_reload_timer_paused_; Strictly speaking (IIUC), this isn't pausing the timer as pausing the retry; the timer will always run for a full interval from the backoffs intervals, or not be running. It won't go partway through the interval, stop for a while, and then finish the interval. If you agree with this pedantry, maybe change the variable name? https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.h:218: // True if there is an uncommitted-but-started load. This is used to inhibit nit, suggestion: Maybe add "of either an error or a non-error page" to the end of the first sentence? When I first read this, I thought this only marked non-error loads (no reason for that, it was just my assumption) and that clarification would prevent that misunderstanding.
https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper.cc (right): https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper.cc:118: core_.OnCommitLoad(GetFrameType(frame), url); On 2014/04/30 18:22:16, rdsmith wrote: > I don't personally care, but is there a reason to use the variable rather than > just putting frame->document().url() into the OnCommitLoad argument list? Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:336: // TODO(ellyjones): Make online_ accurate at object creation. On 2014/04/30 18:22:16, rdsmith wrote: > Right, I had forgotten about this. Doesn't this mean that if you do a > navigation while you're offline, you'll cycle through all the backoff intervals > (because this class will think it's online), and when you finally do come back > online, you'll wait for the largest interval we have before reloading? That > seems like a common enough use case that I'd like to handle it somewhat better > than this. > > It also makes me confused about why I've found this feature so useful so > quickly; I'm often in the position of starting a navigation while offline and > coming online almost immediately after that (as the network initializes). I > would think I would have reloaded at 0, and then started backing off, and I'd > have to wait a few seconds (which I'd really like to avoid doing; I think a > *lot* of the value of this feature is doing the initial attempt at auto-reload > when the network state changes). Do you understand what's going on? > > This isn't a question for this CL (obviously; you didn't change this code) but I > think it's fairly high priority for the user experience of > navigation-while-offline, come online to be a reload fairly quickly after you > come online. http://crbug.com/368796 tracks this. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:365: if (auto_reload_timer_paused_) On 2014/04/30 18:06:58, mmenke wrote: > I don't think either of these if's are needed. Just running their bodies has no > negative side effects if false. Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:423: else if (url != GURL(content::kUnreachableWebDataURL)) On 2014/04/30 18:22:16, rdsmith wrote: > Just confirming: This will *not* trip if the user initiated a navigation that > errored out? I think we want to record an auto-reload failure in that case, and > I don't see that happening on OnStartLoad(). In that case, committed_error_page_info_->auto_reload_triggered is false, so this whole block is not run. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:664: DCHECK(IsReloadableError(*committed_error_page_info_)); On 2014/04/30 18:06:58, mmenke wrote: > Suggest moving this into StartAutoReloadTimer. Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:666: committed_error_page_info_->auto_reload_triggered = true; On 2014/04/30 18:06:58, mmenke wrote: > Suggest either DCHECKING this is true in STartAutoReloadTimer, or moving this > line there. Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:676: auto_reload_timer_paused_ = true; On 2014/04/30 18:06:58, mmenke wrote: > Should expand the CL description to include behavior changes. argh gerrit eats changed commit messages. Will fix. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:696: // Transitioning offline -> online On 2014/04/30 18:06:58, mmenke wrote: > I believe comments for else/ifs generally go inside the block. Think it's > confusing when the else clause has a comment before it (Your indentation below > is also wrong). Moved the comment. I do not understand your remark about indentation. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:698: if (auto_reload_timer_paused_) On 2014/04/30 18:06:58, mmenke wrote: > I don't think we need to check was_online here. Is there any case where > auto_reload_timer_paused_ is true, online is true, but was_online is also true? I wrote the code this way to make it very clear what state transitions are being looked for, even though the checks aren't strictly needed. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:701: } else if (was_online && !online) { On 2014/04/30 18:06:58, mmenke wrote: > Again, do we get anything out of checking was_online here? Seems like we could > just remove it, or even DCHECK it in the inner if body. No. It is just there for clarity. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:703: DCHECK(committed_error_page_info_); On 2014/04/30 18:06:58, mmenke wrote: > Maybe: > > DCHECK(!auto_reload_timer_paused_)? > DCHECK(committed_error_page_info_->auto_reload_triggered)? Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:725: return false; On 2014/04/30 18:06:58, mmenke wrote: > optional: I think it may make more sense to put this first. The other checks > are kinda weird when this is NULL. Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.cc:726: On 2014/04/30 18:06:58, mmenke wrote: > Optional: Could throw in a > DCHECK(committed_error_page_info_->is_auto_reloading) Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core.h (right): https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.h:216: bool auto_reload_timer_paused_; On 2014/04/30 18:22:16, rdsmith wrote: > Strictly speaking (IIUC), this isn't pausing the timer as pausing the retry; the > timer will always run for a full interval from the backoffs intervals, or not be > running. It won't go partway through the interval, stop for a while, and then > finish the interval. If you agree with this pedantry, maybe change the variable > name? Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core.h:218: // True if there is an uncommitted-but-started load. This is used to inhibit On 2014/04/30 18:22:16, rdsmith wrote: > nit, suggestion: Maybe add "of either an error or a non-error page" to the end > of the first sentence? When I first read this, I thought this only marked > non-error loads (no reason for that, it was just my assumption) and that > clarification would prevent that misunderstanding. Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:217: GURL(content::kUnreachableWebDataURL)); On 2014/04/30 18:06:58, mmenke wrote: > error_url() Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:341: GURL error_url_; On 2014/04/30 18:06:58, mmenke wrote: > const? (x2) Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:837: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/30 18:06:58, mmenke wrote: > error_url() Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:961: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/30 18:06:58, mmenke wrote: > error_url() Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1007: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/30 18:06:58, mmenke wrote: > error_url() Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1050: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/30 18:06:58, mmenke wrote: > error_url() Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1087: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/30 18:06:58, mmenke wrote: > error_url() Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1107: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/30 18:06:58, mmenke wrote: > error_url() Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1136: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/30 18:06:58, mmenke wrote: > error_url() Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1160: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/30 18:06:58, mmenke wrote: > error_url() Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1184: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/30 18:06:58, mmenke wrote: > error_url() Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1228: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/30 18:06:58, mmenke wrote: > error_url() Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1243: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/30 18:06:58, mmenke wrote: > error_url()...etc (Probe pages, blank pages, corrections pages, all should use > the error_url) Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1956: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/30 18:06:58, mmenke wrote: > error_url() Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1971: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/30 18:06:58, mmenke wrote: > error_url() Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1996: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/30 18:06:58, mmenke wrote: > error_url() Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:2013: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/04/30 18:06:58, mmenke wrote: > error_url() Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:2117: DoErrorLoad(net::ERR_CONNECTION_RESET); On 2014/04/30 18:06:58, mmenke wrote: > This is more typically a suppressed error page, rather than a full load, right? Yes, actually. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:2196: core().OnFinishLoad(NetErrorHelperCore::MAIN_FRAME); On 2014/04/30 18:06:58, mmenke wrote: > This is just DoSuccessLoad, right? Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:2228: DoErrorLoad(net::ERR_CONNECTION_RESET); On 2014/04/30 18:06:58, mmenke wrote: > So in this case...The user presses the reload button after the autoreload failed > and the error page was suppressed? I think that's not obvious from the test > name, so should either fix that or add comments. Done.
https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1280: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1300: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1367: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1401: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1422: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1460: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1485: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1499: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1535: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1551: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1575: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1594: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1621: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1642: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1669: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1690: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1713: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1728: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, error_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1750: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1765: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, error_url()); error_url. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1902: } Looks like you didn't response to the three comments here. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1902: } Also wonder if we should have a browser test for suppressing the error page.
PTAL? https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1884: TEST_F(NetErrorHelperCoreTest, AutoReloadStopsOnOffline) { On 2014/04/30 18:06:58, mmenke wrote: > This is redundant with AutoReloadRestartsOnOnline now. Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1893: TEST_F(NetErrorHelperCoreTest, AutoReloadStopsOnOfflineThenRestartsOnOnline) { On 2014/04/30 18:06:58, mmenke wrote: > this is identical to OnOnline. Done. https://codereview.chromium.org/259613003/diff/60001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1902: } On 2014/04/30 18:06:58, mmenke wrote: > Suggest a test where the offline event is first, and then make sure we start > reloading only after the online event. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1280: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1300: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1367: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1401: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1422: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1460: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1485: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1499: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1535: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1551: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1575: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1594: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1621: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1642: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1669: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1690: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1713: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1728: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, error_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1750: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, default_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done. https://codereview.chromium.org/259613003/diff/80001/chrome/renderer/net/net_... chrome/renderer/net/net_error_helper_core_unittest.cc:1765: core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME, error_url()); On 2014/05/02 14:35:12, mmenke wrote: > error_url. Done.
LGTM https://codereview.chromium.org/259613003/diff/100001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/259613003/diff/100001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:1804: DoErrorLoad(net::ERR_CONNECTION_RESET); A bit far afield of this CL, I suppose, but some of these tests assume we aren't suppressing error page loads. Suggest a followup CL to beef up unittests of this case. https://codereview.chromium.org/259613003/diff/100001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:2108: // DoErrorLoad(net::ERR_CONNECTION_RESET); Remove commented out line.
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/259613003/100001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
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/259613003/100001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
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/259613003/120001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
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/259613003/120001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
LGTM https://codereview.chromium.org/259613003/diff/160001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/259613003/diff/160001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:517: if (committed_error_page_info_ && !pending_error_page_info_ && Hrm...In a followup CL, should probably reset pending_error_page_info_ if the commit URL does not match the pending URL. I'm happy to do the followup CL myself. https://codereview.chromium.org/259613003/diff/160001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/259613003/diff/160001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:2395: TEST_F(NetErrorHelperCoreTest, ShouldSuppressNonReloadableErrorPage) { I suggest renaming this...Yes, it tests ShouldSuppress for a NonReloadableErrorPage, but it should *not* suppress it. Maybe just ShouldNotSuppressNonReloadableErrorPage...?
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/259613003/240001
Message was sent while issue was closed.
Change committed as 271007 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
