Update entry page type to include error pages when appropriate.

Update entry page type to include error pages when appropriate.

At present, the NavigationEntry page type always reports
PAGE_TYPE_NORMAL for all committed entries, even if an
error page is currently showing. There is a need for a
mechanism to detect when an error page is showing in order
to prevent zooming in that case, so we modify the
navigation entry to correctly report error pages.

BUG=403268
Committed: https://crrev.com/e6a5d22fadcf548ec89226a464a947538f79ae5a
Cr-Commit-Position: refs/heads/master@{#296614}

[wjmaclean, 2014-09-22 17:11:12]

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

[wjmaclean, 2014-09-22 17:11:13]

creis@ - would you be able to give this an initial sanity check? Please note
comments below.

https://codereview.chromium.org/580133002/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/580133002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2584: render_frame_host, url,
url_is_unreachable, transition_type));
We could avoid touching a lot of files if we stored the url_is_unreachable state
in WebContents, and retrieved it with WebContents::IsUrlReachable() ... does
that seem reasonable?

[Charlie Reis, 2014-09-22 21:43:53]

This affects much more code than I was expecting.  I don't think we need to
change WebContentsObserver::DidCommitProvisionalLoad, do we?  We can just ask
for the page type from GetLastCommittedEntry in ZoomController.

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

https://codereview.chromium.org/580133002/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl.cc:1042: if
(new_entry->GetPageType() != PAGE_TYPE_ERROR)
I thought we were going to call set_page_type(PAGE_TYPE_ERROR) here if
params.url_is_unreachable.

https://codereview.chromium.org/580133002/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/580133002/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:168:
pending_entry->set_page_type(PAGE_TYPE_ERROR);
I question whether we should do this on the pending entry.  As you found, not
all error navigations have pending entries.  Might be better to only set the
type on committed entries.

https://codereview.chromium.org/580133002/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:543: params.url_is_unreachable,
Why is this needed here?  I thought we only needed it in
NavigationControllerImpl::RendererDidNavigate.

https://codereview.chromium.org/580133002/diff/80001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/580133002/diff/80001/content/common/frame_mes...
content/common/frame_messages.h:144: // The URL reachability status.
This comment isn't very helpful for understanding what it's about.  Can we say
this is about error pages, or is this used for other things as well?

[wjmaclean, 2014-09-23 18:42:38]

I think I've addressed your concerns (and now have the tests working). PTAL?

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

https://codereview.chromium.org/580133002/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl.cc:1042: if
(new_entry->GetPageType() != PAGE_TYPE_ERROR)
On 2014/09/22 21:43:53, Charlie Reis wrote:
> I thought we were going to call set_page_type(PAGE_TYPE_ERROR) here if
> params.url_is_unreachable.

Hmmm, not here, as this only gets called if there's a pending_entry. I'll try
below this if-else block.

https://codereview.chromium.org/580133002/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/580133002/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:168:
pending_entry->set_page_type(PAGE_TYPE_ERROR);
On 2014/09/22 21:43:53, Charlie Reis wrote:
> I question whether we should do this on the pending entry.  As you found, not
> all error navigations have pending entries.  Might be better to only set the
> type on committed entries.

Ooops, this was meant to be deleted.

https://codereview.chromium.org/580133002/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:543: params.url_is_unreachable,
On 2014/09/22 21:43:53, Charlie Reis wrote:
> Why is this needed here?  I thought we only needed it in
> NavigationControllerImpl::RendererDidNavigate.

I was concerned about the page-type getting reset elsewhere, but perhaps that
was short-sighted. I've removed this.

https://codereview.chromium.org/580133002/diff/80001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/580133002/diff/80001/content/common/frame_mes...
content/common/frame_messages.h:144: // The URL reachability status.
On 2014/09/22 21:43:53, Charlie Reis wrote:
> This comment isn't very helpful for understanding what it's about.  Can we say
> this is about error pages, or is this used for other things as well?

You got into more detail on this round than I expected ... this was just a
placeholder comment until I found out if the approach was ok.

Fixed.

[wjmaclean, 2014-09-23 18:43:42]

wjmaclean@chromium.org changed reviewers:
+ kenrb@chromium.org, rockot@chromium.org

[wjmaclean, 2014-09-23 18:43:42]

rockot@chromium.org: Please review changes in 

tabs_test.cc

kenrb@chromium.org: Please review changes in 

frame_messages.h

[kenrb, 2014-09-23 18:56:36]

ipc lgtm

[wjmaclean, 2014-09-23 19:08:42]

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

https://codereview.chromium.org/580133002/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller_browsertest.cc:124: 
I should put the page-type check back in place here.

[wjmaclean, 2014-09-24 14:30:59]

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

[wjmaclean, 2014-09-24 14:31:00]

agl@chromium.org: Please review changes in 

ssl_browser_tests.cc

[Ken Rockot(use gerrit already), 2014-09-24 16:16:53]

c/b/ext lgtm

[Charlie Reis, 2014-09-24 16:43:44]

Great-- this looks better.  One quick sanity check about the actual behavior of
zoom on error pages (and interstitials) below.

https://codereview.chromium.org/580133002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

https://codereview.chromium.org/580133002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:800: // In this test we need two
URLs that (1) represent real pages (i.e. they
FYI: You can also achieve this with a host resolver that rewrites all domains to
localhost:

  host_resolver()->AddRule("*", "127.0.0.1");
  ASSERT_TRUE(test_server()->Start());

You can then create URLs on arbitrary origins with:

  GURL::Replacements replace_host;
  std::string foo_com("foo.com");
  GURL cross_site_url(test_server()->GetURL("files/title2.html"));
  replace_host.SetHostStr(foo_com);
  cross_site_url = cross_site_url.ReplaceComponents(replace_host);

There's examples in SitePerProcessBrowserTest.

It's fine to stick with the about:blank transition if it meets your needs, but
the other way is more general if you need to handle more domains.

https://codereview.chromium.org/580133002/diff/120001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/580133002/diff/120001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:86: // a crashed tab or an error page.
Sanity check: I realize that changing the zoom level does nothing on error pages
today, and this change makes that explicit (e.g., disabling the zoom bubble,
etc).  An alternate fix would make the error message actually zoom, which might
be better for accessibility.  Is that worth considering, or are there reasons
not to do that?

(Either way, we'll need the PAGE_TYPE_ERROR fixes, which are great regardless.)

Oh, and it looks like interstitial pages have this same issue-- zooming them
doesn't affect the interstitial but does show the bubble.  I wonder if that's
affecting the destination site if you end up proceeding through the
interstitial?

https://codereview.chromium.org/580133002/diff/120001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/580133002/diff/120001/content/common/frame_me...
content/common/frame_messages.h:145: // so that we can set the appropriate page
type. Some services, such as
nit: We don't need the second sentence, just in case that behavior changes in
the future.

[wjmaclean, 2014-09-24 17:59:11]

PTAL?

https://codereview.chromium.org/580133002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

https://codereview.chromium.org/580133002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:800: // In this test we need two
URLs that (1) represent real pages (i.e. they
On 2014/09/24 16:43:43, Charlie Reis wrote:
> FYI: You can also achieve this with a host resolver that rewrites all domains
to
> localhost:
> 
>   host_resolver()->AddRule("*", "127.0.0.1");
>   ASSERT_TRUE(test_server()->Start());
> 
> You can then create URLs on arbitrary origins with:
> 
>   GURL::Replacements replace_host;
>   std::string foo_com("foo.com");
>   GURL cross_site_url(test_server()->GetURL("files/title2.html"));
>   replace_host.SetHostStr(foo_com);
>   cross_site_url = cross_site_url.ReplaceComponents(replace_host);
> 
> There's examples in SitePerProcessBrowserTest.
> 
> It's fine to stick with the about:blank transition if it meets your needs, but
> the other way is more general if you need to handle more domains.

Unfortunately this doesn't work in this case, since HostZoomMap operates on
re-written URLs, so it sees the localhost domain for everything and the test
becomes invalid (and fails). Oh well :-)

https://codereview.chromium.org/580133002/diff/120001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/580133002/diff/120001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:86: // a crashed tab or an error page.
On 2014/09/24 16:43:43, Charlie Reis wrote:
> Sanity check: I realize that changing the zoom level does nothing on error
pages
> today, and this change makes that explicit (e.g., disabling the zoom bubble,
> etc).  An alternate fix would make the error message actually zoom, which
might
> be better for accessibility.  Is that worth considering, or are there reasons
> not to do that?
> 
> (Either way, we'll need the PAGE_TYPE_ERROR fixes, which are great
regardless.)
> 
> Oh, and it looks like interstitial pages have this same issue-- zooming them
> doesn't affect the interstitial but does show the bubble.  I wonder if that's
> affecting the destination site if you end up proceeding through the
> interstitial?

Good observation ... let's do this: make error & interstitial pages not zoom
(for now), and get the UX folks involved. But we need to proceed carefully,
since when an error page is showing, zooming will affect the intended url, not
just the error page, so we would need to think about how to handle that
properly.

https://codereview.chromium.org/580133002/diff/120001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/580133002/diff/120001/content/common/frame_me...
content/common/frame_messages.h:145: // so that we can set the appropriate page
type. Some services, such as
On 2014/09/24 16:43:43, Charlie Reis wrote:
> nit: We don't need the second sentence, just in case that behavior changes in
> the future.

Done.

[agl, 2014-09-24 17:59:59]

LGTM for ssl_browser_tests.cc.

[Charlie Reis, 2014-09-24 18:08:52]

Great.  LGTM with nits.

https://codereview.chromium.org/580133002/diff/120001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/580133002/diff/120001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:86: // a crashed tab or an error page.
On 2014/09/24 17:59:11, wjmaclean wrote:
> On 2014/09/24 16:43:43, Charlie Reis wrote:
> > Sanity check: I realize that changing the zoom level does nothing on error
> pages
> > today, and this change makes that explicit (e.g., disabling the zoom bubble,
> > etc).  An alternate fix would make the error message actually zoom, which
> might
> > be better for accessibility.  Is that worth considering, or are there
reasons
> > not to do that?
> > 
> > (Either way, we'll need the PAGE_TYPE_ERROR fixes, which are great
> regardless.)
> > 
> > Oh, and it looks like interstitial pages have this same issue-- zooming them
> > doesn't affect the interstitial but does show the bubble.  I wonder if
that's
> > affecting the destination site if you end up proceeding through the
> > interstitial?
> 
> Good observation ... let's do this: make error & interstitial pages not zoom
> (for now), and get the UX folks involved. But we need to proceed carefully,
> since when an error page is showing, zooming will affect the intended url, not
> just the error page, so we would need to think about how to handle that
> properly.

Sounds perfect-- thanks.

https://codereview.chromium.org/580133002/diff/140001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/580133002/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:82: bool is_error_or_interstitial_page
=
Let's invert and call this is_normal.

https://codereview.chromium.org/580133002/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:86: // a crashed tab or an error page.
nit: mention interstitials here as well (as the other example of non-normal
pages)

[wjmaclean, 2014-09-24 18:18:51]

https://codereview.chromium.org/580133002/diff/140001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/580133002/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:82: bool is_error_or_interstitial_page
=
On 2014/09/24 18:08:52, Charlie Reis wrote:
> Let's invert and call this is_normal.

Done.

https://codereview.chromium.org/580133002/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:86: // a crashed tab or an error page.
On 2014/09/24 18:08:52, Charlie Reis wrote:
> nit: mention interstitials here as well (as the other example of non-normal
> pages)

Done.

[wjmaclean, 2014-09-24 18:18:58]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-09-24 18:19:58]

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

[commit-bot: I haz the power, 2014-09-25 00:21:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-25 00:21:44]

Exceeded time limit waiting for builds to trigger.

[wjmaclean, 2014-09-25 01:39:31]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-09-25 01:40:24]

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

[commit-bot: I haz the power, 2014-09-25 01:41:50]

Committed patchset #9 (id:160001) as 6671da5cf399ecb5ad45faf35dc36185b6439eb1

[commit-bot: I haz the power, 2014-09-25 01:42:23]

Patchset 9 (id:??) landed as
https://crrev.com/e6a5d22fadcf548ec89226a464a947538f79ae5a
Cr-Commit-Position: refs/heads/master@{#296614}

[xiyuan, 2014-09-26 19:52:40]

xiyuan@chromium.org changed reviewers:
+ xiyuan@chromium.org

[xiyuan, 2014-09-26 19:52:40]

This is causing crash on launching a kiosk app.
Filed http://crbug.com/418214.