Add an error page for HTTP 404 error pages without bodies.

Add an error page for HTTP 404 error pages without bodies.

We used to show Link Doctor pages in this case, but that broke long ago.
https://crbug.com/331745 was recently fixed, which allows us to show error
pages in that case once more.

Also add a browser test for HTTP 500 errors pages without bodies, which
also fixed by that CL.

Also fixes a bunch of browser tests that implicitly depend on *not* showing
error pages when we get a 404 response without a body.

BUG=632781, 331745
Committed: https://crrev.com/d485e9156b72ea4bb4682ffbf73ea0eb248ecb6c
Cr-Commit-Position: refs/heads/master@{#409567}

[mmenke, 2016-07-29 17:08:47]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 17:09:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 17:24:20]

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

[commit-bot: I haz the power, 2016-07-29 17:24:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mmenke, 2016-07-29 19:12:15]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 19:12:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-07-29 19:33:39]

Description was changed from

==========
Add test for empty HTTP error pages.

BUG=
==========

to

==========
Add an error page for HTTP 404 error pages without bodies.

We used to show Link Doctor pages in this case, but that broke long ago.

Also add a test for HTTP 500 errors pages without bodies.

BUG=632781,331745
==========

[mmenke, 2016-07-29 19:52:01]

The CQ bit was unchecked by mmenke@chromium.org

[mmenke, 2016-07-29 22:11:56]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 22:12:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 22:58:19]

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

[commit-bot: I haz the power, 2016-07-29 22:58:21]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mmenke, 2016-08-01 17:35:48]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 17:36:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-08-01 17:36:08]

Patchset #4 (id:60001) has been deleted

[mmenke, 2016-08-01 17:38:02]

Description was changed from

==========
Add an error page for HTTP 404 error pages without bodies.

We used to show Link Doctor pages in this case, but that broke long ago.

Also add a test for HTTP 500 errors pages without bodies.

BUG=632781,331745
==========

to

==========
Add an error page for HTTP 404 error pages without bodies.

We used to show Link Doctor pages in this case, but that broke long ago. 
https://crbug.com/331745 was recently fixed, which allows us to show error
pages in that case once more.

Also add a browser test for HTTP 500 errors pages without bodies, which
also fixed by that CL.

BUG=632781,331745
==========

[mmenke, 2016-08-01 17:52:10]

mmenke@chromium.org changed reviewers:
+ edwardjung@chromium.org, rdsmith@chromium.org

[mmenke, 2016-08-01 17:52:11]

[rdsmith]:  Please review chrome/test/data/errorpage/,
chrome/browser/errorpage_browsertest.cc, and
components/error_page/common/localized_error.cc.  The rest of the files are just
fixing tests that depending on *not* showing an error page on HTTP 404 responses
with empty bodies.  I'll add owners once I get a signoff on the meat of the CL.

[edwardjung]:  Please review components/error_page/common/localized_error.cc. 
Note that I'm just re-using generic "not found" text.  If you feel this isn't
good enough, we can try to improve it in this CL, or if you prefer, you can take
it on in a followup.

https://codereview.chromium.org/2190463008/diff/100001/components/error_page/...
File components/error_page/common/localized_error.cc (right):

https://codereview.chromium.org/2190463008/diff/100001/components/error_page/...
components/error_page/common/localized_error.cc:372: const LocalizedErrorMap
http_error_options[] = {
I got sick of deliberately not allowing git CL format to reformat these arrays. 
Suppose there's some comment to disable formatting we could use, if this is a
concern.

[edwardjung, 2016-08-01 18:03:43]

LGTM

I'd updated the strings as part of the big strings update so these look fine.

[commit-bot: I haz the power, 2016-08-01 18:07:06]

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

[commit-bot: I haz the power, 2016-08-01 18:07:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mmenke, 2016-08-01 18:11:54]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 18:12:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-08-01 18:27:12]

On 2016/08/01 18:03:43, edwardjung wrote:
> LGTM
> 
> I'd updated the strings as part of the big strings update so these look fine.

The strings themselves are find, was just wondering it something that made
clearer we could connect to the server, but the server couldn't find the
resource would be better.  OF course, not sure that distinction is actionable.

[Randy Smith (Not in Mondays), 2016-08-01 18:50:50]

On 2016/08/01 17:52:11, mmenke wrote:
> [rdsmith]:  Please review chrome/test/data/errorpage/,
> chrome/browser/errorpage_browsertest.cc, and
> components/error_page/common/localized_error.cc.  The rest of the files are
just
> fixing tests that depending on *not* showing an error page on HTTP 404
responses
> with empty bodies.  I'll add owners once I get a signoff on the meat of the
CL.
> 
> [edwardjung]:  Please review components/error_page/common/localized_error.cc. 
> Note that I'm just re-using generic "not found" text.  If you feel this isn't
> good enough, we can try to improve it in this CL, or if you prefer, you can
take
> it on in a followup.
> 
>
https://codereview.chromium.org/2190463008/diff/100001/components/error_page/...
> File components/error_page/common/localized_error.cc (right):
> 
>
https://codereview.chromium.org/2190463008/diff/100001/components/error_page/...
> components/error_page/common/localized_error.cc:372: const LocalizedErrorMap
> http_error_options[] = {
> I got sick of deliberately not allowing git CL format to reformat these
arrays. 
> Suppose there's some comment to disable formatting we could use, if this is a
> concern.

QQ (will review in parallel): Where is the empty body check?  I went wandering
up and down the callstack and didn't see it.

[mmenke, 2016-08-01 18:54:22]

On 2016/08/01 18:50:50, Randy Smith - OOO until 8-1 wrote:
> On 2016/08/01 17:52:11, mmenke wrote:
> > [rdsmith]:  Please review chrome/test/data/errorpage/,
> > chrome/browser/errorpage_browsertest.cc, and
> > components/error_page/common/localized_error.cc.  The rest of the files are
> just
> > fixing tests that depending on *not* showing an error page on HTTP 404
> responses
> > with empty bodies.  I'll add owners once I get a signoff on the meat of the
> CL.
> > 
> > [edwardjung]:  Please review
components/error_page/common/localized_error.cc. 
> > Note that I'm just re-using generic "not found" text.  If you feel this
isn't
> > good enough, we can try to improve it in this CL, or if you prefer, you can
> take
> > it on in a followup.
> > 
> >
>
https://codereview.chromium.org/2190463008/diff/100001/components/error_page/...
> > File components/error_page/common/localized_error.cc (right):
> > 
> >
>
https://codereview.chromium.org/2190463008/diff/100001/components/error_page/...
> > components/error_page/common/localized_error.cc:372: const LocalizedErrorMap
> > http_error_options[] = {
> > I got sick of deliberately not allowing git CL format to reformat these
> arrays. 
> > Suppose there's some comment to disable formatting we could use, if this is
a
> > concern.
> 
> QQ (will review in parallel): Where is the empty body check?  I went wandering
> up and down the callstack and didn't see it.

It's in RenderViewHostImpl -
https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rc...

[mmenke, 2016-08-01 18:59:05]

On 2016/08/01 18:54:22, mmenke wrote:
> On 2016/08/01 18:50:50, Randy Smith - OOO until 8-1 wrote:
> > On 2016/08/01 17:52:11, mmenke wrote:
> > > [rdsmith]:  Please review chrome/test/data/errorpage/,
> > > chrome/browser/errorpage_browsertest.cc, and
> > > components/error_page/common/localized_error.cc.  The rest of the files
are
> > just
> > > fixing tests that depending on *not* showing an error page on HTTP 404
> > responses
> > > with empty bodies.  I'll add owners once I get a signoff on the meat of
the
> > CL.
> > > 
> > > [edwardjung]:  Please review
> components/error_page/common/localized_error.cc. 
> > > Note that I'm just re-using generic "not found" text.  If you feel this
> isn't
> > > good enough, we can try to improve it in this CL, or if you prefer, you
can
> > take
> > > it on in a followup.
> > > 
> > >
> >
>
https://codereview.chromium.org/2190463008/diff/100001/components/error_page/...
> > > File components/error_page/common/localized_error.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2190463008/diff/100001/components/error_page/...
> > > components/error_page/common/localized_error.cc:372: const
LocalizedErrorMap
> > > http_error_options[] = {
> > > I got sick of deliberately not allowing git CL format to reformat these
> > arrays. 
> > > Suppose there's some comment to disable formatting we could use, if this
is
> a
> > > concern.
> > 
> > QQ (will review in parallel): Where is the empty body check?  I went
wandering
> > up and down the callstack and didn't see it.
> 
> It's in RenderViewHostImpl -
>
https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rc...

I'll go ahead and add a test for a 404 with a body.

[commit-bot: I haz the power, 2016-08-01 18:59:31]

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

[commit-bot: I haz the power, 2016-08-01 18:59:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Randy Smith (Not in Mondays), 2016-08-01 18:59:50]

LGTM with presumption that you'll do the right thing (which may be nothing :-})
with my minor comments below.  Thanks for the pointer to the empty body check.

https://codereview.chromium.org/2190463008/diff/100001/components/error_page/...
File components/error_page/common/localized_error.cc (right):

https://codereview.chromium.org/2190463008/diff/100001/components/error_page/...
components/error_page/common/localized_error.cc:372: const LocalizedErrorMap
http_error_options[] = {
On 2016/08/01 17:52:11, mmenke wrote:
> I got sick of deliberately not allowing git CL format to reformat these
arrays. 
> Suppose there's some comment to disable formatting we could use, if this is a
> concern.

Reformatting it at some point makes sense to me, and I don't mind piggybacking
that on this CL.

https://codereview.chromium.org/2190463008/diff/120001/chrome/browser/errorpa...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/2190463008/diff/120001/chrome/browser/errorpa...
chrome/browser/errorpage_browsertest.cc:954: // body.
Is this comment correct?  It seems to be copied from the above and not match the
test title/HTML.

https://codereview.chromium.org/2190463008/diff/120001/chrome/browser/errorpa...
chrome/browser/errorpage_browsertest.cc:956:
NavigateToFileURL("errorpage/empty500.html");
I'm not familiar with the underlying logic of the helper functions, but does
using NavigateToFileURL() risk not noticing if there's a two phase navigation as
in the last test (i.e. if we load something then fail over to link doctor and
load the link doctor response as a second navigation)?  Or is the timing such
that that will always be caught (without races) by the EXPECT_EQ below?

[Randy Smith (Not in Mondays), 2016-08-01 19:01:44]

On 2016/08/01 18:59:05, mmenke wrote:
> On 2016/08/01 18:54:22, mmenke wrote:
> > On 2016/08/01 18:50:50, Randy Smith - OOO until 8-1 wrote:
> > > On 2016/08/01 17:52:11, mmenke wrote:
> > > > [rdsmith]:  Please review chrome/test/data/errorpage/,
> > > > chrome/browser/errorpage_browsertest.cc, and
> > > > components/error_page/common/localized_error.cc.  The rest of the files
> are
> > > just
> > > > fixing tests that depending on *not* showing an error page on HTTP 404
> > > responses
> > > > with empty bodies.  I'll add owners once I get a signoff on the meat of
> the
> > > CL.
> > > > 
> > > > [edwardjung]:  Please review
> > components/error_page/common/localized_error.cc. 
> > > > Note that I'm just re-using generic "not found" text.  If you feel this
> > isn't
> > > > good enough, we can try to improve it in this CL, or if you prefer, you
> can
> > > take
> > > > it on in a followup.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2190463008/diff/100001/components/error_page/...
> > > > File components/error_page/common/localized_error.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2190463008/diff/100001/components/error_page/...
> > > > components/error_page/common/localized_error.cc:372: const
> LocalizedErrorMap
> > > > http_error_options[] = {
> > > > I got sick of deliberately not allowing git CL format to reformat these
> > > arrays. 
> > > > Suppose there's some comment to disable formatting we could use, if this
> is
> > a
> > > > concern.
> > > 
> > > QQ (will review in parallel): Where is the empty body check?  I went
> wandering
> > > up and down the callstack and didn't see it.
> > 
> > It's in RenderViewHostImpl -
> >
>
https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rc...
> 
> I'll go ahead and add a test for a 404 with a body.

It looked to me like there already was one?  (I had that thought and didn't
include it because I didn't think it was necessary.)

[mmenke, 2016-08-01 19:09:26]

On 2016/08/01 19:01:44, Randy Smith - OOO until 8-1 wrote:
> On 2016/08/01 18:59:05, mmenke wrote:
> > On 2016/08/01 18:54:22, mmenke wrote:
> > > On 2016/08/01 18:50:50, Randy Smith - OOO until 8-1 wrote:
> > > > On 2016/08/01 17:52:11, mmenke wrote:
> > > > > [rdsmith]:  Please review chrome/test/data/errorpage/,
> > > > > chrome/browser/errorpage_browsertest.cc, and
> > > > > components/error_page/common/localized_error.cc.  The rest of the
files
> > are
> > > > just
> > > > > fixing tests that depending on *not* showing an error page on HTTP 404
> > > > responses
> > > > > with empty bodies.  I'll add owners once I get a signoff on the meat
of
> > the
> > > > CL.
> > > > > 
> > > > > [edwardjung]:  Please review
> > > components/error_page/common/localized_error.cc. 
> > > > > Note that I'm just re-using generic "not found" text.  If you feel
this
> > > isn't
> > > > > good enough, we can try to improve it in this CL, or if you prefer,
you
> > can
> > > > take
> > > > > it on in a followup.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2190463008/diff/100001/components/error_page/...
> > > > > File components/error_page/common/localized_error.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2190463008/diff/100001/components/error_page/...
> > > > > components/error_page/common/localized_error.cc:372: const
> > LocalizedErrorMap
> > > > > http_error_options[] = {
> > > > > I got sick of deliberately not allowing git CL format to reformat
these
> > > > arrays. 
> > > > > Suppose there's some comment to disable formatting we could use, if
this
> > is
> > > a
> > > > > concern.
> > > > 
> > > > QQ (will review in parallel): Where is the empty body check?  I went
> > wandering
> > > > up and down the callstack and didn't see it.
> > > 
> > > It's in RenderViewHostImpl -
> > >
> >
>
https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rc...
> > 
> > I'll go ahead and add a test for a 404 with a body.
> 
> It looked to me like there already was one?  (I had that thought and didn't
> include it because I didn't think it was necessary.)

Thanks!  You're right, I even deliberately put the new tests right after it. 
Just forgot about it.

[mmenke, 2016-08-01 20:18:29]

https://codereview.chromium.org/2190463008/diff/120001/chrome/browser/errorpa...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/2190463008/diff/120001/chrome/browser/errorpa...
chrome/browser/errorpage_browsertest.cc:954: // body.
On 2016/08/01 18:59:50, Randy Smith - OOO until 8-1 wrote:
> Is this comment correct?  It seems to be copied from the above and not match
the
> test title/HTML.

Done.

https://codereview.chromium.org/2190463008/diff/120001/chrome/browser/errorpa...
chrome/browser/errorpage_browsertest.cc:956:
NavigateToFileURL("errorpage/empty500.html");
On 2016/08/01 18:59:50, Randy Smith - OOO until 8-1 wrote:
> I'm not familiar with the underlying logic of the helper functions, but does
> using NavigateToFileURL() risk not noticing if there's a two phase navigation
as
> in the last test (i.e. if we load something then fail over to link doctor and
> load the link doctor response as a second navigation)?  Or is the timing such
> that that will always be caught (without races) by the EXPECT_EQ below?

If there's a two phase navigation, we load a blank page, and then the link
doctor page.  So unless that changes significantly, we'll either have a blank
page or a link doctor page, both of which will fail.

I'm pretty sure that it will quit the run loop after the first navigation (i.e.,
it doesn't spin the run loop after the navigation), but I'm not positive of
that.

[mmenke, 2016-08-01 21:03:30]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 21:03:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-08-01 21:26:29]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 21:26:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-08-01 21:33:35]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 21:33:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-01 23:01:48]

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

[commit-bot: I haz the power, 2016-08-01 23:01:49]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[edwardjung, 2016-08-02 09:52:57]

On 2016/08/01 18:27:12, mmenke wrote:
> On 2016/08/01 18:03:43, edwardjung wrote:
> > LGTM
> > 
> > I'd updated the strings as part of the big strings update so these look
fine.
> 
> The strings themselves are find, was just wondering it something that made
> clearer we could connect to the server, but the server couldn't find the
> resource would be better.  OF course, not sure that distinction is actionable.

Maybe, I'll discuss this with rachelis and follow up with amends in a new CL if
necessary.

[mmenke, 2016-08-02 14:59:39]

Description was changed from

==========
Add an error page for HTTP 404 error pages without bodies.

We used to show Link Doctor pages in this case, but that broke long ago. 
https://crbug.com/331745 was recently fixed, which allows us to show error
pages in that case once more.

Also add a browser test for HTTP 500 errors pages without bodies, which
also fixed by that CL.

BUG=632781,331745
==========

to

==========
Add an error page for HTTP 404 error pages without bodies.

We used to show Link Doctor pages in this case, but that broke long ago.
https://crbug.com/331745 was recently fixed, which allows us to show error
pages in that case once more.

Also add a browser test for HTTP 500 errors pages without bodies, which
also fixed by that CL.

Also fixes a bunch of browser tests that implicitly depend on *not* showing
error pages when we get a 404 response without a body.

BUG=632781,331745
==========

[mmenke, 2016-08-02 15:13:18]

mmenke@chromium.org changed reviewers:
+ estark@chromium.org, pkasting@chromium.org, rdevlin.cronin@chromium.org,
tedchoc@chromium.org

[mmenke, 2016-08-02 15:13:19]

Hey guys, this CL fixes a bunch of tests that depend on not showing error pages
in response to a 404 error (Which the test server does for unrecognized URLs).

[+estark]:  Please review chrome/browser/ssl/
[+rdevlin.cronin]:  Please review chrome/browser/extensions/,
chrome/test/data/extensions/, and extensions/browser/api/runtime/
[+pkasting]:  Please review chrome/browser/ui/browser_browsertest.cc
[+tedchoc]:  Please review
chrome/android/javatests/src/org/chromium/chrome/browser/

[estark, 2016-08-02 15:56:27]

//chrome/browser/ssl lgtm

https://codereview.chromium.org/2190463008/diff/180001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
(right):

https://codereview.chromium.org/2190463008/diff/180001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:713:
embedded_test_server()->GetURL("/title1.html").host()));
None of the changes above here appear to be necessary, as they all just grab the
host and ignore the path. I have no problem if you want to keep it as is just
for consistency, but just making a note of it.

[Devlin, 2016-08-02 16:17:34]

https://codereview.chromium.org/2190463008/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/activity_log/activity_log_browsertest.cc (right):

https://codereview.chromium.org/2190463008/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/activity_log/activity_log_browsertest.cc:58:
base::StringPrintf("http://www.google.com.bo:%u/title1.html", port),
So previously these tried navigating to a nonexistent page, which now fails?

https://codereview.chromium.org/2190463008/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
(right):

https://codereview.chromium.org/2190463008/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:339: if
(!base::StartsWith(request.relative_url, "/test",
Is there a reason this is preferred to just passing in an url that has a backing
file?

https://codereview.chromium.org/2190463008/diff/180001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/runtime/content_script/manifest.json
(right):

https://codereview.chromium.org/2190463008/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/runtime/content_script/manifest.json:7:
"matches": ["*://*/echo"],
Why this change?

[mmenke, 2016-08-02 16:27:00]

Thanks for the feedback!

https://codereview.chromium.org/2190463008/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/activity_log/activity_log_browsertest.cc (right):

https://codereview.chromium.org/2190463008/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/activity_log/activity_log_browsertest.cc:58:
base::StringPrintf("http://www.google.com.bo:%u/title1.html", port),
On 2016/08/02 16:17:34, Devlin wrote:
> So previously these tried navigating to a nonexistent page, which now fails?

Now when we navigate to an empty 404 page, we show an error page.  Prerenders
are cancelled when they see an error page commit.  Showing a 404 page received
from a server doesn't count as an error page, but if we generate the error page,
it does count as an error page.  May be worth changing behavior there, but
beyond the scope of this CL.

https://codereview.chromium.org/2190463008/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
(right):

https://codereview.chromium.org/2190463008/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:339: if
(!base::StartsWith(request.relative_url, "/test",
On 2016/08/02 16:17:34, Devlin wrote:
> Is there a reason this is preferred to just passing in an url that has a
backing
> file?

The tests are all using different paths (test1, test2, test3, ...test7).  I
assume there's a reason they're all using different paths, and creating 7
different files for them all seems silly, as does using 7 different random
pre-existing files.

Open to other approaches.

https://codereview.chromium.org/2190463008/diff/180001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
(right):

https://codereview.chromium.org/2190463008/diff/180001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:713:
embedded_test_server()->GetURL("/title1.html").host()));
On 2016/08/02 15:56:27, estark wrote:
> None of the changes above here appear to be necessary, as they all just grab
the
> host and ignore the path. I have no problem if you want to keep it as is just
> for consistency, but just making a note of it.

Two tests in this file fail without the change, not sure if all of them do. 
Maybe showing an error page removes the secure connection information?

Sample failure:

../../chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:854:
Failure
Value of: observer.latest_security_style()
  Actual: 1
Expected: content::SECURITY_STYLE_AUTHENTICATED
Which is: 4
../../chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:142:
Failure
Value of: secure_explanations.size()
  Actual: 1
Expected: cert_status == VALID_CERTIFICATE ? 2u : 1u
Which is: 2
[30450:30565:0729/130753:WARNING:embedded_test_server.cc(202)] Request not
handled. Returning 404: /
[30512:30522:0729/130753:ERROR:node_controller.cc(1099)] Could not be introduced
to peer 386E624A087FAB90.1FB5EFE2A9AF3BAA
../../chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:107:
Failure
Value of: observer.latest_security_style()
  Actual: 1
Expected: content::SECURITY_STYLE_AUTHENTICATION_BROKEN
Which is: 2
../../chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:112:
Failure
Value of: expired_explanation.broken_explanations.size()
  Actual: 0
Expected: 1u
Which is: 1
../../chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:159:
Failure
Value of: secure_explanations.back().summary
  Actual: "Valid Certificate"
Expected: l10n_util::GetStringUTF8(18966)
Which is: "Secure TLS connection"
../../chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:162:
Failure
Value of: secure_explanations.back().description
  Actual: "The connection to this site is using a valid, trusted server
certificate."
Expected: l10n_util::GetStringUTF8(18967)

https://codereview.chromium.org/2190463008/diff/180001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/runtime/content_script/manifest.json
(right):

https://codereview.chromium.org/2190463008/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/runtime/content_script/manifest.json:7:
"matches": ["*://*/echo"],
On 2016/08/02 16:17:34, Devlin wrote:
> Why this change?

See the change in /extensions/browser/api/runtime/runtime_apitest.cc.

There is no webpage.html, and I was sure what the base path of the test server
was, so I just used a file the embedded test server provides itself.

[Ted C, 2016-08-02 16:30:48]

On 2016/08/02 16:27:00, mmenke wrote:
> Thanks for the feedback!
> 
>
https://codereview.chromium.org/2190463008/diff/180001/chrome/browser/extensi...
> File chrome/browser/extensions/activity_log/activity_log_browsertest.cc
(right):
> 
>
https://codereview.chromium.org/2190463008/diff/180001/chrome/browser/extensi...
> chrome/browser/extensions/activity_log/activity_log_browsertest.cc:58:
> base::StringPrintf("http://www.google.com.bo:%u/title1.html", port),
> On 2016/08/02 16:17:34, Devlin wrote:
> > So previously these tried navigating to a nonexistent page, which now fails?
> 
> Now when we navigate to an empty 404 page, we show an error page.  Prerenders
> are cancelled when they see an error page commit.  Showing a 404 page received
> from a server doesn't count as an error page, but if we generate the error
page,
> it does count as an error page.  May be worth changing behavior there, but
> beyond the scope of this CL.
> 
>
https://codereview.chromium.org/2190463008/diff/180001/chrome/browser/extensi...
> File chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
> (right):
> 
>
https://codereview.chromium.org/2190463008/diff/180001/chrome/browser/extensi...
> chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:339: if
> (!base::StartsWith(request.relative_url, "/test",
> On 2016/08/02 16:17:34, Devlin wrote:
> > Is there a reason this is preferred to just passing in an url that has a
> backing
> > file?
> 
> The tests are all using different paths (test1, test2, test3, ...test7).  I
> assume there's a reason they're all using different paths, and creating 7
> different files for them all seems silly, as does using 7 different random
> pre-existing files.
> 
> Open to other approaches.
> 
>
https://codereview.chromium.org/2190463008/diff/180001/chrome/browser/ssl/chr...
> File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
> (right):
> 
>
https://codereview.chromium.org/2190463008/diff/180001/chrome/browser/ssl/chr...
> chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:713:
> embedded_test_server()->GetURL("/title1.html").host()));
> On 2016/08/02 15:56:27, estark wrote:
> > None of the changes above here appear to be necessary, as they all just grab
> the
> > host and ignore the path. I have no problem if you want to keep it as is
just
> > for consistency, but just making a note of it.
> 
> Two tests in this file fail without the change, not sure if all of them do. 
> Maybe showing an error page removes the secure connection information?
> 
> Sample failure:
> 
>
../../chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:854:
> Failure
> Value of: observer.latest_security_style()
>   Actual: 1
> Expected: content::SECURITY_STYLE_AUTHENTICATED
> Which is: 4
>
../../chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:142:
> Failure
> Value of: secure_explanations.size()
>   Actual: 1
> Expected: cert_status == VALID_CERTIFICATE ? 2u : 1u
> Which is: 2
> [30450:30565:0729/130753:WARNING:embedded_test_server.cc(202)] Request not
> handled. Returning 404: /
> [30512:30522:0729/130753:ERROR:node_controller.cc(1099)] Could not be
introduced
> to peer 386E624A087FAB90.1FB5EFE2A9AF3BAA
>
../../chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:107:
> Failure
> Value of: observer.latest_security_style()
>   Actual: 1
> Expected: content::SECURITY_STYLE_AUTHENTICATION_BROKEN
> Which is: 2
>
../../chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:112:
> Failure
> Value of: expired_explanation.broken_explanations.size()
>   Actual: 0
> Expected: 1u
> Which is: 1
>
../../chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:159:
> Failure
> Value of: secure_explanations.back().summary
>   Actual: "Valid Certificate"
> Expected: l10n_util::GetStringUTF8(18966)
> Which is: "Secure TLS connection"
>
../../chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:162:
> Failure
> Value of: secure_explanations.back().description
>   Actual: "The connection to this site is using a valid, trusted server
> certificate."
> Expected: l10n_util::GetStringUTF8(18967)
> 
>
https://codereview.chromium.org/2190463008/diff/180001/chrome/test/data/exten...
> File chrome/test/data/extensions/api_test/runtime/content_script/manifest.json
> (right):
> 
>
https://codereview.chromium.org/2190463008/diff/180001/chrome/test/data/exten...
> chrome/test/data/extensions/api_test/runtime/content_script/manifest.json:7:
> "matches": ["*://*/echo"],
> On 2016/08/02 16:17:34, Devlin wrote:
> > Why this change?
> 
> See the change in /extensions/browser/api/runtime/runtime_apitest.cc.
> 
> There is no webpage.html, and I was sure what the base path of the test server
> was, so I just used a file the embedded test server provides itself.

chrome/android/javatests - lgtm

[mmenke, 2016-08-02 18:18:34]

https://codereview.chromium.org/2190463008/diff/180001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/runtime/content_script/manifest.json
(right):

https://codereview.chromium.org/2190463008/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/runtime/content_script/manifest.json:7:
"matches": ["*://*/echo"],
On 2016/08/02 16:27:00, mmenke wrote:
> On 2016/08/02 16:17:34, Devlin wrote:
> > Why this change?
> 
> See the change in /extensions/browser/api/runtime/runtime_apitest.cc.
> 
> There is no webpage.html, and I was sure what the base path of the test server
> was, so I just used a file the embedded test server provides itself.

Looks like this is just using the default path, so I switched to title1.html. 
(And the reason why I had to change it is extensions can't run on / otherwise
access error pages.  I assume this is because error pages have magic internal
URLs that don't match the visible URL)

[mmenke, 2016-08-02 18:23:01]

mmenke@chromium.org changed reviewers:
+ thakis@chromium.org

[mmenke, 2016-08-02 18:23:02]

[+thakis]:  I need a chrome/ owners signoff for
chrome/browser/errorpage_browsertest.cc (Which rdsmith has already reviewed). 
Thanks!

Perhaps this file should be moved into chrome/browser/net/, so we don't have to
bug a chrome/ owner to update it?

It mostly serves to have integration tests for code in components/error_page/, a
couple other chrome/ and content/ renderer files, and one or two files in
chrome/browser/net.

[Nico, 2016-08-02 18:24:31]

rs-lgtm

Moving the test to a subdirectory sounds fine to me.

[Peter Kasting, 2016-08-03 01:41:08]

c/b/ui LGTM

[Devlin, 2016-08-03 16:18:39]

extensions lgtm

[mmenke, 2016-08-03 16:24:30]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2016-08-03 16:24:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
edwardjung@chromium.org, rdsmith@chromium.org, tedchoc@chromium.org,
estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2190463008/#ps200001
(title: "Switch runtime test to title1.html")

[commit-bot: I haz the power, 2016-08-03 16:25:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 17:59:22]

Description was changed from

==========
Add an error page for HTTP 404 error pages without bodies.

We used to show Link Doctor pages in this case, but that broke long ago.
https://crbug.com/331745 was recently fixed, which allows us to show error
pages in that case once more.

Also add a browser test for HTTP 500 errors pages without bodies, which
also fixed by that CL.

Also fixes a bunch of browser tests that implicitly depend on *not* showing
error pages when we get a 404 response without a body.

BUG=632781,331745
==========

to

==========
Add an error page for HTTP 404 error pages without bodies.

We used to show Link Doctor pages in this case, but that broke long ago.
https://crbug.com/331745 was recently fixed, which allows us to show error
pages in that case once more.

Also add a browser test for HTTP 500 errors pages without bodies, which
also fixed by that CL.

Also fixes a bunch of browser tests that implicitly depend on *not* showing
error pages when we get a 404 response without a body.

BUG=632781,331745
==========

[commit-bot: I haz the power, 2016-08-03 17:59:24]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-08-03 18:01:55]

Description was changed from

==========
Add an error page for HTTP 404 error pages without bodies.

We used to show Link Doctor pages in this case, but that broke long ago.
https://crbug.com/331745 was recently fixed, which allows us to show error
pages in that case once more.

Also add a browser test for HTTP 500 errors pages without bodies, which
also fixed by that CL.

Also fixes a bunch of browser tests that implicitly depend on *not* showing
error pages when we get a 404 response without a body.

BUG=632781,331745
==========

to

==========
Add an error page for HTTP 404 error pages without bodies.

We used to show Link Doctor pages in this case, but that broke long ago.
https://crbug.com/331745 was recently fixed, which allows us to show error
pages in that case once more.

Also add a browser test for HTTP 500 errors pages without bodies, which
also fixed by that CL.

Also fixes a bunch of browser tests that implicitly depend on *not* showing
error pages when we get a 404 response without a body.

BUG=632781,331745

Committed: https://crrev.com/d485e9156b72ea4bb4682ffbf73ea0eb248ecb6c
Cr-Commit-Position: refs/heads/master@{#409567}
==========

[commit-bot: I haz the power, 2016-08-03 18:01:57]

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