Move alternate error page loading out of WebFrame....

Move alternate error page loading out of WebFrame.

Make the RenderView be in charge of loading alternate error pages.

While working on this change, I noticed several related bugs:

1-  Loading an URL with an invalid host name from the new tab page results in
an error page.  If you hit back and then forward, you will be left with an
empty location bar.  In a debug build this trips an assertion in
ClassifyNavigation because the given page_id is -1.  This problem is caused by
not duplicating the NavigationState of the failed load when creating a load for
the error page.  Hence, the pending_page_id of the forward navigation is lost.

2-  Loading an URL with an invalid host name as a subframe results in an extra
navigation in session history.  One navigation for the main frame and one
navigation for the error page load.  This is another symptom of the problem
described in #1.  However, the solution is different.  Here, we need to know
that the subframe load is an AUTO_SUBFRAME load so that we load the error page
using 'replace' semantics (so that WebCore does not generate a new session
history entry).

Finally, I decided to restrict alternative DNS error pages to only work for the
main frame to match what we do for alternative 404 error pages.  It doesn't seem
worth it to show link doctor results for subframes since their primary purpose
is to assist people who mis-type an URL.

R=tony,brettw
BUG=15648
TEST=covered by errorpage_uitest.cc


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=22253

[tony, 2009-07-31 20:56:30]

I see, since AltErrorPageResourceFetcher now takes a WebFrame, you can call it
from RenderView.  Nice!

The changes to move the resource fetching into the RenderView LGTM.  I'll let
brettw comment about the navigation state changes.

http://codereview.chromium.org/159575/diff/62/1017
File webkit/glue/webframe_impl.h (right):

http://codereview.chromium.org/159575/diff/62/1017#newcode280
Line 280: //XXX scoped_ptr<webkit_glue::AltErrorPageResourceFetcher>
alt_error_page_fetcher_;
Nit: Remove these?

http://codereview.chromium.org/159575/diff/68/1039
File webkit/glue/webframeloaderclient_impl.cc (right):

http://codereview.chromium.org/159575/diff/68/1039#newcode393
Line 393: WebViewDelegate* d = webframe_->GetWebViewImpl()->delegate();
Should these now use |frame|?

[brettw, 2009-07-31 21:45:56]

http://codereview.chromium.org/159575/diff/68/1048
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/159575/diff/68/1048#newcode969
Line 969: navigation_state->set_transition_type(params.transition);
This just gets clobbered 9 lines below with LINK.

http://codereview.chromium.org/159575/diff/68/1048#newcode1145
Line 1145: } else if (frame->GetParent()->IsLoading()) {
It seems bad to use this signal here, and another way of computing whether this
is an auto-subframe above. Is it possible to somehow be consistent in what
signals we use for this?

I'm also concerned about: what if the main frame is still loading becuase it's
very long or something, can we start loading subframes and allow the user to
interact with the page such that there could be manual subframe navigations
while the outer one is still pending?

http://codereview.chromium.org/159575/diff/68/1048#newcode2858
Line 2858: const GURL& error_page_url =
GetAlternateErrorPageURL(error.unreachableURL,
GetAlternateErrorPageURL returns a copy, so I wouldn't bother with the const and
reference for this variable.

http://codereview.chromium.org/159575/diff/68/1048#newcode2866
Line 2866: frame->LoadHTMLString("",
The first argument is a WebData using an implicit constructor. I suppose WebKit
doesn't want it to be marked explicit? But it seems nice to make it explicit
here, I originally thought it was a string.

[darin (slow to review), 2009-07-31 22:56:24]

http://codereview.chromium.org/159575/diff/62/1017
File webkit/glue/webframe_impl.h (right):

http://codereview.chromium.org/159575/diff/62/1017#newcode280
Line 280: //XXX scoped_ptr<webkit_glue::AltErrorPageResourceFetcher>
alt_error_page_fetcher_;
On 2009/07/31 20:56:30, tony wrote:
> Nit: Remove these?

Done.

http://codereview.chromium.org/159575/diff/68/1039
File webkit/glue/webframeloaderclient_impl.cc (right):

http://codereview.chromium.org/159575/diff/68/1039#newcode393
Line 393: WebViewDelegate* d = webframe_->GetWebViewImpl()->delegate();
On 2009/07/31 20:56:30, tony wrote:
> Should these now use |frame|?

No need.  |frame| is the same as |webframe_|.  Note: there is one
WebFrameLoaderClient per WebFrameImpl.  This code is going away soon anyways.  I
plan to move it to RenderView as well.

[darin (slow to review), 2009-07-31 23:28:38]

http://codereview.chromium.org/159575/diff/68/1048
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/159575/diff/68/1048#newcode969
Line 969: navigation_state->set_transition_type(params.transition);
On 2009/07/31 21:45:56, brettw wrote:
> This just gets clobbered 9 lines below with LINK.

Indeed.  I don't think that is necessary.

http://codereview.chromium.org/159575/diff/68/1048#newcode1145
Line 1145: } else if (frame->GetParent()->IsLoading()) {
On 2009/07/31 21:45:56, brettw wrote:
> It seems bad to use this signal here, and another way of computing whether
this
> is an auto-subframe above. Is it possible to somehow be consistent in what
> signals we use for this?

Yeah, I was not happy about this either.  But, unfortunately, I cannot use the
same signal as above.  That  one is based on whether or not there is a page
transition (did the page_id_ increment or not?).  Unfortunately, at the time of
DidFailProvisionalLoadWithError, there will never have been a page transition
for the frame.

So, instead, I went with looking to see if the parent frame is loading.  I could
store this information as a bool on NavigationState instead of reusing the
transition_type_ field, but this seemed better.

I open to suggestions for how to make this cleaner :-)


> I'm also concerned about: what if the main frame is still loading becuase it's
> very long or something, can we start loading subframes and allow the user to
> interact with the page such that there could be manual subframe navigations
> while the outer one is still pending?

Hmm... I would assume that existing page loading would stop before new page
loading starts.  That's how things normally work, and session history would get
royally screwed up if this were not the case.  However, I'm not certain that
WebCore stops the main frame and all children or if it only stops the target
frame and all of its children.  I think if this were possible, then it would be
a legit WebCore bug and not something for us to worry about at this layer.

http://codereview.chromium.org/159575/diff/68/1048#newcode2858
Line 2858: const GURL& error_page_url =
GetAlternateErrorPageURL(error.unreachableURL,
On 2009/07/31 21:45:56, brettw wrote:
> GetAlternateErrorPageURL returns a copy, so I wouldn't bother with the const
and
> reference for this variable.

I just moved this code.  Are you concerned about readability?  It is safe to use
a const reference here.

http://codereview.chromium.org/159575/diff/68/1048#newcode2866
Line 2866: frame->LoadHTMLString("",
On 2009/07/31 21:45:56, brettw wrote:
> The first argument is a WebData using an implicit constructor. I suppose
WebKit
> doesn't want it to be marked explicit? But it seems nice to make it explicit
> here, I originally thought it was a string.

Yeah, it is convenient for WebData to allow implicit conversion from a
null-terminated char array.  I can make this code explicit.

[brettw, 2009-08-01 00:23:19]

On Fri, Jul 31, 2009 at 11:28 PM, <darin@chromium.org> wrote:
>
> http://codereview.chromium.org/159575/diff/68/1048
> File chrome/renderer/render_view.cc (right):
>
> http://codereview.chromium.org/159575/diff/68/1048#newcode969
> Line 969: navigation_state->set_transition_type(params.transition);
> On 2009/07/31 21:45:56, brettw wrote:
>>
>> This just gets clobbered 9 lines below with LINK.
>
> Indeed. =A0I don't think that is necessary.
>
> http://codereview.chromium.org/159575/diff/68/1048#newcode1145
> Line 1145: } else if (frame->GetParent()->IsLoading()) {
> On 2009/07/31 21:45:56, brettw wrote:
>>
>> It seems bad to use this signal here, and another way of computing
>
> whether this
>>
>> is an auto-subframe above. Is it possible to somehow be consistent in
>
> what
>>
>> signals we use for this?
>
> Yeah, I was not happy about this either. =A0But, unfortunately, I cannot
> use the same signal as above. =A0That =A0one is based on whether or not
> there is a page transition (did the page_id_ increment or not?).
> Unfortunately, at the time of DidFailProvisionalLoadWithError, there
> will never have been a page transition for the frame.
>
> So, instead, I went with looking to see if the parent frame is loading.
> I could store this information as a bool on NavigationState instead of
> reusing the transition_type_ field, but this seemed better.
>
> I open to suggestions for how to make this cleaner :-)

No, I'll trust you on this that bad things won't happen.

LGTM