Mark error pages as such on Ctrl-Shift-T reload.

Mark error pages as such on Ctrl-Shift-T reload.

When reloading a page whose url leads to an error page, a different
pathway is taken in NavigationControllerImpl. We must be careful to
check for unreachable urls in
NavigationControllerImpl::RendererDidNavigateToExistingPage() in this
case, and mark them as error pages.

BUG=458036
Committed: https://crrev.com/7431bb2983db683dcc4556e1918556254c3d5589
Cr-Commit-Position: refs/heads/master@{#317046}

[wjmaclean, 2015-02-12 17:15:36]

wjmaclean@chromium.org changed reviewers:
+ creis@chromium.org

[wjmaclean, 2015-02-12 17:16:33]

Tiny CL, currently running this on the bots to make sure it doesn't break stuff.

Can you please check the approach, and if you're OK with it, I'll see if I can
devise a suitable test?

[Charlie Reis, 2015-02-12 21:57:47]

https://codereview.chromium.org/917043004/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/917043004/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1128:
entry->set_page_type(PAGE_TYPE_ERROR);
Seems reasonable to me.  What about the other RendererDidNavigate* functions? 
SamePage and InPage might matter (e.g., #foo on the network error page).  Not
sure about the subframe cases.

[wjmaclean, 2015-02-13 17:23:40]

On 2015/02/12 21:57:47, Charlie Reis wrote:
>
https://codereview.chromium.org/917043004/diff/1/content/browser/frame_host/n...
> File content/browser/frame_host/navigation_controller_impl.cc (right):
> 
>
https://codereview.chromium.org/917043004/diff/1/content/browser/frame_host/n...
> content/browser/frame_host/navigation_controller_impl.cc:1128:
> entry->set_page_type(PAGE_TYPE_ERROR);
> Seems reasonable to me.  What about the other RendererDidNavigate* functions? 
> SamePage and InPage might matter (e.g., #foo on the network error page).  Not
> sure about the subframe cases.

I wondered about that. Can SamePage/InPage ever be invoked on a network error
page?

[wjmaclean, 2015-02-13 19:55:04]

creis@ - PTAL, I've added a test.

I'm open to adding the same machinery for the other load types, if we can
identify the cases that might trigger them (otherwise writing tests might be
difficult or impossible).

We could always add them in a follow on CL if we identify how to trigger these
paths with error pages at some later date.

[Charlie Reis, 2015-02-13 21:38:37]

On 2015/02/13 19:55:04, wjmaclean wrote:
> creis@ - PTAL, I've added a test.
> 
> I'm open to adding the same machinery for the other load types, if we can
> identify the cases that might trigger them (otherwise writing tests might be
> difficult or impossible).

I mentioned that navigating to #foo on a network error page would likely hit one
of those cases.  Can you check if that works?

Also, I like your zoom test as a way to catch the regression of the bug.  It
probably makes sense to also add unit tests to
navigation_controller_impl_unittest.cc (similar to
NavigationControllerTest.Interstitial).

> We could always add them in a follow on CL if we identify how to trigger these
> paths with error pages at some later date.

https://codereview.chromium.org/917043004/diff/20001/chrome/browser/ui/zoom/z...
File chrome/browser/ui/zoom/zoom_controller_browsertest.cc (right):

https://codereview.chromium.org/917043004/diff/20001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_controller_browsertest.cc:177: GURL
url("http://kjfhkjsdf.com");
Let's add a comment saying this is meant to cause a network error page.  (Tests
can't reach the network, so that should work even if someone registers this
domain.)

https://codereview.chromium.org/917043004/diff/20001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_controller_browsertest.cc:199:
content::WindowedNotificationObserver tab_added_observer(
Can we use content::WebContentsAddedObserver plus WaitForLoadStop instead? 
NotificationService is deprecated.

[wjmaclean, 2015-02-17 20:39:14]

creis@ - PTAL?

I've added the code to set the page type in the additional two cases, but have a
question: will we ever need to re-set the page type from ERROR to NORMAL in the
case of existing entries?

https://codereview.chromium.org/917043004/diff/20001/chrome/browser/ui/zoom/z...
File chrome/browser/ui/zoom/zoom_controller_browsertest.cc (right):

https://codereview.chromium.org/917043004/diff/20001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_controller_browsertest.cc:177: GURL
url("http://kjfhkjsdf.com");
On 2015/02/13 21:38:37, Charlie Reis wrote:
> Let's add a comment saying this is meant to cause a network error page. 
(Tests
> can't reach the network, so that should work even if someone registers this
> domain.)

Done.

https://codereview.chromium.org/917043004/diff/20001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_controller_browsertest.cc:199:
content::WindowedNotificationObserver tab_added_observer(
On 2015/02/13 21:38:36, Charlie Reis wrote:
> Can we use content::WebContentsAddedObserver plus WaitForLoadStop instead? 
> NotificationService is deprecated.

Done.

[Charlie Reis, 2015-02-18 01:01:53]

https://codereview.chromium.org/917043004/diff/40001/chrome/browser/ui/zoom/z...
File chrome/browser/ui/zoom/zoom_controller_browsertest.cc (right):

https://codereview.chromium.org/917043004/diff/40001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_controller_browsertest.cc:175: // It assumes that
tests can't reach the network, so this test should continue
nit: Drop both "It assumes" and "that."  Just start with "Tests can't reach"

https://codereview.chromium.org/917043004/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/917043004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl.cc:1212: // it is
persisted between navigations?
This is a good question.  Can you test this manually?  I'd suggest navigating to
two pages, disabling your network, clearing cache, then going back.  That should
give you an existing entry with an error page.  Then repeat with the network
re-enabled.  I bet you're right that we would need to fix the page type to be
normal again (in all cases, not just this one).

[wjmaclean, 2015-02-18 19:10:11]

PTAL - I've addressed the comment issue, and modified so that we reset to
PAGE_TYPE_NORMAL when param.url_is_unreachable is false.

https://codereview.chromium.org/917043004/diff/40001/chrome/browser/ui/zoom/z...
File chrome/browser/ui/zoom/zoom_controller_browsertest.cc (right):

https://codereview.chromium.org/917043004/diff/40001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_controller_browsertest.cc:175: // It assumes that
tests can't reach the network, so this test should continue
On 2015/02/18 01:01:53, Charlie Reis wrote:
> nit: Drop both "It assumes" and "that."  Just start with "Tests can't reach"

Done.

https://codereview.chromium.org/917043004/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/917043004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl.cc:1212: // it is
persisted between navigations?
On 2015/02/18 01:01:53, Charlie Reis wrote:
> This is a good question.  Can you test this manually?  I'd suggest navigating
to
> two pages, disabling your network, clearing cache, then going back.  That
should
> give you an existing entry with an error page.  Then repeat with the network
> re-enabled.  I bet you're right that we would need to fix the page type to be
> normal again (in all cases, not just this one).

After a manual test, there doesn't seem to be a difference, though in a way I'm
not surprised given how few locations in the code test for PAGE_TYPE_ERROR. But,
I suspect it's the right thing to do, and so have added it in.

[Charlie Reis, 2015-02-18 23:53:04]

Thanks!  LGTM with one change and a few nits.

https://codereview.chromium.org/917043004/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/917043004/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl.cc:1212: // it is
persisted between navigations?
On 2015/02/18 19:10:11, wjmaclean wrote:
> On 2015/02/18 01:01:53, Charlie Reis wrote:
> > This is a good question.  Can you test this manually?  I'd suggest
navigating
> to
> > two pages, disabling your network, clearing cache, then going back.  That
> should
> > give you an existing entry with an error page.  Then repeat with the network
> > re-enabled.  I bet you're right that we would need to fix the page type to
be
> > normal again (in all cases, not just this one).
> 
> After a manual test, there doesn't seem to be a difference, though in a way
I'm
> not surprised given how few locations in the code test for PAGE_TYPE_ERROR.
But,
> I suspect it's the right thing to do, and so have added it in.

I think that's the right thing to do.  Thanks for being thorough!

https://codereview.chromium.org/917043004/diff/100001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller_browsertest.cc (right):

https://codereview.chromium.org/917043004/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller_browsertest.cc:174: // This url is meant
to cause a network error page to be loaded. It assumes
nit: Drop "It assumes"

(There were two instances of it before.)  :)

https://codereview.chromium.org/917043004/diff/100001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/917043004/diff/100001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1078:
new_entry->set_page_type(PAGE_TYPE_ERROR);
It looks like this will behave the same as the cases below, since we're
guaranteed for page_type to be NORMAL given the two branches above.

However, I think it's worth making this code consistent with the cases below. 
Let's:

1) Do the same thing here as below (set to ERROR or NORMAL based on
url_is_unreachable).
2) Delete the set_page_type call on line 1056.
3) Move the "Don't use the page type..." comment from 1053 to here and update it
to say "must always be normal or error."

https://codereview.chromium.org/917043004/diff/100001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1213: 
nit: No blank line.

[wjmaclean, 2015-02-19 13:22:31]

New patchsets have been uploaded after l-g-t-m from creis@chromium.org

[wjmaclean, 2015-02-19 13:22:42]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2015-02-19 13:23:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/917043004/120001

[wjmaclean, 2015-02-19 13:24:38]

https://codereview.chromium.org/917043004/diff/100001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller_browsertest.cc (right):

https://codereview.chromium.org/917043004/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller_browsertest.cc:174: // This url is meant
to cause a network error page to be loaded. It assumes
On 2015/02/18 23:53:04, Charlie Reis wrote:
> nit: Drop "It assumes"
> 
> (There were two instances of it before.)  :)

Ooops, missed that!

Done.

https://codereview.chromium.org/917043004/diff/100001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/917043004/diff/100001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1078:
new_entry->set_page_type(PAGE_TYPE_ERROR);
On 2015/02/18 23:53:04, Charlie Reis wrote:
> It looks like this will behave the same as the cases below, since we're
> guaranteed for page_type to be NORMAL given the two branches above.
> 
> However, I think it's worth making this code consistent with the cases below. 
> Let's:
> 
> 1) Do the same thing here as below (set to ERROR or NORMAL based on
> url_is_unreachable).
> 2) Delete the set_page_type call on line 1056.
> 3) Move the "Don't use the page type..." comment from 1053 to here and update
it
> to say "must always be normal or error."

Done.

https://codereview.chromium.org/917043004/diff/100001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1213: 
On 2015/02/18 23:53:04, Charlie Reis wrote:
> nit: No blank line.

Done.

[commit-bot: I haz the power, 2015-02-19 14:53:50]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-19 14:54:28]

Patchset 7 (id:??) landed as
https://crrev.com/7431bb2983db683dcc4556e1918556254c3d5589
Cr-Commit-Position: refs/heads/master@{#317046}