Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2345913002:
Fix NetErrorTabHelper with PlzNavigate. (Closed)
DescriptionFix NetErrorTabHelper with PlzNavigate.
This includes:
-switching NetErrorTabHelper to use the new WebContentsObserver navigation callbacks that work with PlzNavigate
-fixing NavigationHandle::IsErrorPage to return true for reloads of error pages
This fixes
DnsProbeBrowserTest.CorrectionsDisabled
DnsProbeBrowserTest.CorrectionsLoadStopped
DnsProbeBrowserTest.CorrectionsLoadStoppedSlowProbe
DnsProbeBrowserTest.Incognito
DnsProbeBrowserTest.NoInternetProbeResultWithBrokenCorrections
DnsProbeBrowserTest.NoInternetProbeResultWithSlowBrokenCorrections
DnsProbeBrowserTest.NxdomainProbeResultWithWorkingCorrections
DnsProbeBrowserTest.NxdomainProbeResultWithWorkingSlowCorrections
DnsProbeBrowserTest.ProbesDisabled
DnsProbeBrowserTest.SyncFailureWithBrokenCorrections
ErrorPageNavigationCorrectionsFailTest.FetchCorrectionsFails
BUG=504347, 647859
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
R=clamy@chromium.org, juliatuttle@chromium.org
Committed: https://crrev.com/a9a4ec98f42e665ecdef10370825e76c5299d72f
Cr-Commit-Position: refs/heads/master@{#419483}
Patch Set 1 #Patch Set 2 : fix NavigationHandle::IsErrorPage for reloads of error pages #
Total comments: 10
Messages
Total messages: 39 (29 generated)
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jam@chromium.org
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Description was changed from ========== Fix NetErroTabHelper with PlzNavigate. This fixes DnsProbeBrowserTest.CorrectionsDisabled DnsProbeBrowserTest.CorrectionsLoadStopped DnsProbeBrowserTest.CorrectionsLoadStoppedSlowProbe DnsProbeBrowserTest.Incognito DnsProbeBrowserTest.NoInternetProbeResultWithBrokenCorrections DnsProbeBrowserTest.NoInternetProbeResultWithSlowBrokenCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingSlowCorrections DnsProbeBrowserTest.ProbesDisabled DnsProbeBrowserTest.SyncFailureWithBrokenCorrections BUG=504347 ========== to ========== Fix NetErroTabHelper with PlzNavigate. This fixes DnsProbeBrowserTest.CorrectionsDisabled DnsProbeBrowserTest.CorrectionsLoadStopped DnsProbeBrowserTest.CorrectionsLoadStoppedSlowProbe DnsProbeBrowserTest.Incognito DnsProbeBrowserTest.NoInternetProbeResultWithBrokenCorrections DnsProbeBrowserTest.NoInternetProbeResultWithSlowBrokenCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingSlowCorrections DnsProbeBrowserTest.ProbesDisabled DnsProbeBrowserTest.SyncFailureWithBrokenCorrections BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jam@chromium.org
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix NetErroTabHelper with PlzNavigate. This fixes DnsProbeBrowserTest.CorrectionsDisabled DnsProbeBrowserTest.CorrectionsLoadStopped DnsProbeBrowserTest.CorrectionsLoadStoppedSlowProbe DnsProbeBrowserTest.Incognito DnsProbeBrowserTest.NoInternetProbeResultWithBrokenCorrections DnsProbeBrowserTest.NoInternetProbeResultWithSlowBrokenCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingSlowCorrections DnsProbeBrowserTest.ProbesDisabled DnsProbeBrowserTest.SyncFailureWithBrokenCorrections BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix NetErrorTabHelper with PlzNavigate. This includes: -switching NetErrorTabHelper to use the new WebContentsObserver navigation callbacks that work with PlzNavigate -fixing NavigationHandle::IsErrorPage to return true for reloads of error pages This fixes DnsProbeBrowserTest.CorrectionsDisabled DnsProbeBrowserTest.CorrectionsLoadStopped DnsProbeBrowserTest.CorrectionsLoadStoppedSlowProbe DnsProbeBrowserTest.Incognito DnsProbeBrowserTest.NoInternetProbeResultWithBrokenCorrections DnsProbeBrowserTest.NoInternetProbeResultWithSlowBrokenCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingSlowCorrections DnsProbeBrowserTest.ProbesDisabled DnsProbeBrowserTest.SyncFailureWithBrokenCorrections BUG=504347,647859 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
The CQ bit was unchecked by jam@chromium.org
Description was changed from ========== Fix NetErrorTabHelper with PlzNavigate. This includes: -switching NetErrorTabHelper to use the new WebContentsObserver navigation callbacks that work with PlzNavigate -fixing NavigationHandle::IsErrorPage to return true for reloads of error pages This fixes DnsProbeBrowserTest.CorrectionsDisabled DnsProbeBrowserTest.CorrectionsLoadStopped DnsProbeBrowserTest.CorrectionsLoadStoppedSlowProbe DnsProbeBrowserTest.Incognito DnsProbeBrowserTest.NoInternetProbeResultWithBrokenCorrections DnsProbeBrowserTest.NoInternetProbeResultWithSlowBrokenCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingSlowCorrections DnsProbeBrowserTest.ProbesDisabled DnsProbeBrowserTest.SyncFailureWithBrokenCorrections BUG=504347,647859 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix NetErrorTabHelper with PlzNavigate. This includes: -switching NetErrorTabHelper to use the new WebContentsObserver navigation callbacks that work with PlzNavigate -fixing NavigationHandle::IsErrorPage to return true for reloads of error pages This fixes DnsProbeBrowserTest.CorrectionsDisabled DnsProbeBrowserTest.CorrectionsLoadStopped DnsProbeBrowserTest.CorrectionsLoadStoppedSlowProbe DnsProbeBrowserTest.Incognito DnsProbeBrowserTest.NoInternetProbeResultWithBrokenCorrections DnsProbeBrowserTest.NoInternetProbeResultWithSlowBrokenCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingSlowCorrections DnsProbeBrowserTest.ProbesDisabled DnsProbeBrowserTest.SyncFailureWithBrokenCorrections ErrorPageNavigationCorrectionsFailTest.FetchCorrectionsFails BUG=504347,647859 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
jam@chromium.org changed reviewers: + clamy@chromium.org
Camille: ptal at content/ (I'll get a chrome/browser/net owner later if the content changes sound good). Since PlzNavigate only gives one commit instead of two for error pages, the NetErrorTabHelper had to be updated. Additionally, I had to fix NavigationHandle to still return IsErrorPage when error pages are reloaded.
Thanks! It looks mostly good, a few questions below. https://codereview.chromium.org/2345913002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2345913002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:465: // If an error page reloads, net_error_code might be 200 but we still want to nit: no "we" in comments? https://codereview.chromium.org/2345913002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:467: if (params.base_url.spec() == kUnreachableWebDataURL || When we reload the error page, do we know at the start of the navigation that we're loading kUnreachableWebDataURL, or just at commit time? If at the start, should we modify IsErrorPage so that it returns true also when we're trying to load kUnreachableWebDataURL? https://codereview.chromium.org/2345913002/diff/100001/content/test/web_conte... File content/test/web_contents_observer_sanity_checker.cc (left): https://codereview.chromium.org/2345913002/diff/100001/content/test/web_conte... content/test/web_contents_observer_sanity_checker.cc:206: navigation_handle->GetNetErrorCode() != net::OK); Rather than removing this check, can we expend it to cover navigation_handle->GetNetErrorCode() != net::OK || rul = kUnreachableUrl? I think these are the only two cases where we can have an error page.
https://codereview.chromium.org/2345913002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2345913002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:465: // If an error page reloads, net_error_code might be 200 but we still want to On 2016/09/19 11:46:44, clamy wrote: > nit: no "we" in comments? I realize some people think it's unambiguous in some situations, but I think in this case it's not. https://codereview.chromium.org/2345913002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:467: if (params.base_url.spec() == kUnreachableWebDataURL || On 2016/09/19 11:46:44, clamy wrote: > When we reload the error page, do we know at the start of the navigation that > we're loading kUnreachableWebDataURL, or just at commit time? If at the start, > should we modify IsErrorPage so that it returns true also when we're trying to > load kUnreachableWebDataURL? It's not known at the start (because the url is the what the user typed). Only when the commit IPC comes does it have the base url to identify it as an error page. https://codereview.chromium.org/2345913002/diff/100001/content/test/web_conte... File content/test/web_contents_observer_sanity_checker.cc (left): https://codereview.chromium.org/2345913002/diff/100001/content/test/web_conte... content/test/web_contents_observer_sanity_checker.cc:206: navigation_handle->GetNetErrorCode() != net::OK); On 2016/09/19 11:46:44, clamy wrote: > Rather than removing this check, can we expend it to cover > navigation_handle->GetNetErrorCode() != net::OK || rul = kUnreachableUrl? I > think these are the only two cases where we can have an error page. if you mean navigation_handle->GetURL(), it'll be the URL of the page the user navigated to, and not the error page. base_url isn't exposed at this point.
Thanks! Lgtm. https://codereview.chromium.org/2345913002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2345913002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:465: // If an error page reloads, net_error_code might be 200 but we still want to On 2016/09/19 14:19:40, jam wrote: > On 2016/09/19 11:46:44, clamy wrote: > > nit: no "we" in comments? > > I realize some people think it's unambiguous in some situations, but I think in > this case it's not. Acknowledged. https://codereview.chromium.org/2345913002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:467: if (params.base_url.spec() == kUnreachableWebDataURL || On 2016/09/19 14:19:40, jam wrote: > On 2016/09/19 11:46:44, clamy wrote: > > When we reload the error page, do we know at the start of the navigation that > > we're loading kUnreachableWebDataURL, or just at commit time? If at the start, > > should we modify IsErrorPage so that it returns true also when we're trying to > > load kUnreachableWebDataURL? > > It's not known at the start (because the url is the what the user typed). Only > when the commit IPC comes does it have the base url to identify it as an error > page. Acknowledged. https://codereview.chromium.org/2345913002/diff/100001/content/test/web_conte... File content/test/web_contents_observer_sanity_checker.cc (left): https://codereview.chromium.org/2345913002/diff/100001/content/test/web_conte... content/test/web_contents_observer_sanity_checker.cc:206: navigation_handle->GetNetErrorCode() != net::OK); On 2016/09/19 14:19:40, jam wrote: > On 2016/09/19 11:46:44, clamy wrote: > > Rather than removing this check, can we expend it to cover > > navigation_handle->GetNetErrorCode() != net::OK || rul = kUnreachableUrl? I > > think these are the only two cases where we can have an error page. > > if you mean navigation_handle->GetURL(), it'll be the URL of the page the user > navigated to, and not the error page. base_url isn't exposed at this point. Acknowledged.
jam@chromium.org changed reviewers: + juliatuttle@chromium.org
Julia: ptal at chrome/browser/net/, I saw you added the tests. The existing unit tests depended on the old implementation detail of navigation events which is deprecated. With PlzNavigate, there's only one commit for error pages instead of two. I'm not familiar with these unit tests, so I mostly tried to green them up but I don't know if now they're redundant or if I missed something important in converting them.
Description was changed from ========== Fix NetErrorTabHelper with PlzNavigate. This includes: -switching NetErrorTabHelper to use the new WebContentsObserver navigation callbacks that work with PlzNavigate -fixing NavigationHandle::IsErrorPage to return true for reloads of error pages This fixes DnsProbeBrowserTest.CorrectionsDisabled DnsProbeBrowserTest.CorrectionsLoadStopped DnsProbeBrowserTest.CorrectionsLoadStoppedSlowProbe DnsProbeBrowserTest.Incognito DnsProbeBrowserTest.NoInternetProbeResultWithBrokenCorrections DnsProbeBrowserTest.NoInternetProbeResultWithSlowBrokenCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingSlowCorrections DnsProbeBrowserTest.ProbesDisabled DnsProbeBrowserTest.SyncFailureWithBrokenCorrections ErrorPageNavigationCorrectionsFailTest.FetchCorrectionsFails BUG=504347,647859 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix NetErrorTabHelper with PlzNavigate. This includes: -switching NetErrorTabHelper to use the new WebContentsObserver navigation callbacks that work with PlzNavigate -fixing NavigationHandle::IsErrorPage to return true for reloads of error pages This fixes DnsProbeBrowserTest.CorrectionsDisabled DnsProbeBrowserTest.CorrectionsLoadStopped DnsProbeBrowserTest.CorrectionsLoadStoppedSlowProbe DnsProbeBrowserTest.Incognito DnsProbeBrowserTest.NoInternetProbeResultWithBrokenCorrections DnsProbeBrowserTest.NoInternetProbeResultWithSlowBrokenCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingSlowCorrections DnsProbeBrowserTest.ProbesDisabled DnsProbeBrowserTest.SyncFailureWithBrokenCorrections ErrorPageNavigationCorrectionsFailTest.FetchCorrectionsFails BUG=504347,647859 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
lgtm with nit https://codereview.chromium.org/2345913002/diff/100001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper_unittest.cc (right): https://codereview.chromium.org/2345913002/diff/100001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper_unittest.cc:17: #undef NO_ERROR Comment where you're undefining this from?
The CQ bit was checked by jam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the quick review! On 2016/09/19 16:23:19, Julia Tuttle wrote: > lgtm with nit > > https://codereview.chromium.org/2345913002/diff/100001/chrome/browser/net/net... > File chrome/browser/net/net_error_tab_helper_unittest.cc (right): > > https://codereview.chromium.org/2345913002/diff/100001/chrome/browser/net/net... > chrome/browser/net/net_error_tab_helper_unittest.cc:17: #undef NO_ERROR > Comment where you're undefining this from? Sure, I'll do that in a followup (since trybots are already green :) )
Message was sent while issue was closed.
Description was changed from ========== Fix NetErrorTabHelper with PlzNavigate. This includes: -switching NetErrorTabHelper to use the new WebContentsObserver navigation callbacks that work with PlzNavigate -fixing NavigationHandle::IsErrorPage to return true for reloads of error pages This fixes DnsProbeBrowserTest.CorrectionsDisabled DnsProbeBrowserTest.CorrectionsLoadStopped DnsProbeBrowserTest.CorrectionsLoadStoppedSlowProbe DnsProbeBrowserTest.Incognito DnsProbeBrowserTest.NoInternetProbeResultWithBrokenCorrections DnsProbeBrowserTest.NoInternetProbeResultWithSlowBrokenCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingSlowCorrections DnsProbeBrowserTest.ProbesDisabled DnsProbeBrowserTest.SyncFailureWithBrokenCorrections ErrorPageNavigationCorrectionsFailTest.FetchCorrectionsFails BUG=504347,647859 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix NetErrorTabHelper with PlzNavigate. This includes: -switching NetErrorTabHelper to use the new WebContentsObserver navigation callbacks that work with PlzNavigate -fixing NavigationHandle::IsErrorPage to return true for reloads of error pages This fixes DnsProbeBrowserTest.CorrectionsDisabled DnsProbeBrowserTest.CorrectionsLoadStopped DnsProbeBrowserTest.CorrectionsLoadStoppedSlowProbe DnsProbeBrowserTest.Incognito DnsProbeBrowserTest.NoInternetProbeResultWithBrokenCorrections DnsProbeBrowserTest.NoInternetProbeResultWithSlowBrokenCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingCorrections DnsProbeBrowserTest.NxdomainProbeResultWithWorkingSlowCorrections DnsProbeBrowserTest.ProbesDisabled DnsProbeBrowserTest.SyncFailureWithBrokenCorrections ErrorPageNavigationCorrectionsFailTest.FetchCorrectionsFails BUG=504347,647859 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation R=clamy@chromium.org, juliatuttle@chromium.org Committed: https://crrev.com/a9a4ec98f42e665ecdef10370825e76c5299d72f Cr-Commit-Position: refs/heads/master@{#419483} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a9a4ec98f42e665ecdef10370825e76c5299d72f Cr-Commit-Position: refs/heads/master@{#419483}
Message was sent while issue was closed.
I'm so sorry but I wonder why You didn't use params.url_is_unreachable == true instead of params.base_url.spec() == kUnreachableWebDataURL. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
