Only display Link Doctor page on 404 errors with no body

Only display Link Doctor page on 404 errors with no body.

Had to remove some of the old tests, since TestServer.py
will not sent blank pages when there's an error code (On
the trybots, at least).

BUG=36558
TEST=ErrorPageTest.Page404


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

[mmenke, 2011-02-23 14:51:42]

The python http server used on the trybots doesn't allow completely blank pages
with error codes (Curiously, on my local installs of Windows, OSX, and Goobuntu,
the python http server does).

As a result, I decided to add the IsAsciiWhitespace() check and keep the 512
character limit (out of paranoia).

While it is technically possible we could have a UTF16 plain-text file without a
BOM mark that contains solely the characters 0909, 090A, 090D, 0920, 0A09, ....,
2020.  There are at most 4 characters from each of 4 different languages in that
group, so getting a single meaningful error page only containing them in UTF16,
with no whitespace, strikes me as unlikely in the extreme.

[tony, 2011-02-23 18:21:10]

I was thinking it might be nice if we could remove all the code for handling
postponed data and delaying the commit.  I think I would prefer deleting code to
being able to handle whitespace 404 pages.

[mmenke, 2011-02-23 18:29:06]

That will also require messing with TestServer.py, unfortunately, which is one
of the reasons I chose to keep it.  I'll go ahead and try to figure out why the
tryservers are behaving differently from all of my local machines.

On 2011/02/23 18:21:10, tony wrote:
> I was thinking it might be nice if we could remove all the code for handling
> postponed data and delaying the commit.  I think I would prefer deleting code
to
> being able to handle whitespace 404 pages.

[tony, 2011-02-23 18:40:11]

I think it's OK to not test completely blank error pages.  Or are there other
reasons that we have to mess with TestServer.py?

On 2011/02/23 18:29:06, Matt Menke wrote:
> That will also require messing with TestServer.py, unfortunately, which is one
> of the reasons I chose to keep it.  I'll go ahead and try to figure out why
the
> tryservers are behaving differently from all of my local machines.
> 
> On 2011/02/23 18:21:10, tony wrote:
> > I was thinking it might be nice if we could remove all the code for handling
> > postponed data and delaying the commit.  I think I would prefer deleting
code
> to
> > being able to handle whitespace 404 pages.

[mmenke, 2011-02-23 18:42:19]

That's the only reason.  So I should just go ahead and remove all the 404/Link
Doctor tests?

On 2011/02/23 18:40:11, tony wrote:
> I think it's OK to not test completely blank error pages.  Or are there other
> reasons that we have to mess with TestServer.py?

[mmenke, 2011-02-23 20:35:14]

Postponed data stuff removed.

[tony, 2011-02-23 21:43:20]

http://codereview.chromium.org/6546069/diff/11001/chrome/renderer/navigation_...
File chrome/renderer/navigation_state.h (right):

http://codereview.chromium.org/6546069/diff/11001/chrome/renderer/navigation_...
chrome/renderer/navigation_state.h:213: const bool maybe_use_error_page() const
{ return maybe_use_error_page_; }
Nit: Drop the const in front of the bool.  Also, I would just name this
use_error_page().

http://codereview.chromium.org/6546069/diff/11001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6546069/diff/11001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:3698: if (error_page_url.is_valid()) {
When will this be invalid?  Should we at least keep the dcheck?

http://codereview.chromium.org/6546069/diff/11001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:3712: http_status_code)) {
Do we have a test for some of the other http status codes?  They could be added
in a follow up change.

[mmenke, 2011-02-23 21:53:36]

Thanks.

http://codereview.chromium.org/6546069/diff/11001/chrome/renderer/navigation_...
File chrome/renderer/navigation_state.h (right):

http://codereview.chromium.org/6546069/diff/11001/chrome/renderer/navigation_...
chrome/renderer/navigation_state.h:213: const bool maybe_use_error_page() const
{ return maybe_use_error_page_; }
On 2011/02/23 21:43:20, tony wrote:
> Nit: Drop the const in front of the bool.  Also, I would just name this
> use_error_page().

Both done.

http://codereview.chromium.org/6546069/diff/11001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6546069/diff/11001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:3698: if (error_page_url.is_valid()) {
On 2011/02/23 21:43:20, tony wrote:
> When will this be invalid?  Should we at least keep the dcheck?

It's an empty string (And thus invalid) when chrome is configured not to "Use a
web service to help resolve navigation errors".  There used to be a check for
this before setting |postpone_loading_data| (Now |use_error_page|) to true in
the first place.

http://codereview.chromium.org/6546069/diff/11001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:3712: http_status_code)) {
On 2011/02/23 21:43:20, tony wrote:
> Do we have a test for some of the other http status codes?  They could be
added
> in a follow up change.

No, we don't.  They only trigger on responses with no bodies, too, so would need
to mess with the python http server to do any useful testing of them, though I
agree that making a test for them is a good idea.

[tony, 2011-02-23 22:01:41]

Ah, ok, all makes sense.  LGTM!

[Paweł Hajdan Jr., 2011-02-25 16:02:12]

Drive-by with a tiny nit.

http://codereview.chromium.org/6546069/diff/8002/chrome/browser/errorpage_uit...
File chrome/browser/errorpage_uitest.cc (right):

http://codereview.chromium.org/6546069/diff/8002/chrome/browser/errorpage_uit...
chrome/browser/errorpage_uitest.cc:170:
NavigateToURLBlockUntilNavigationsComplete(
nit: If you're waiting for just one navigation, please use NavigateToURL for
simplicity.