Add error description to the DidFailProvisionalLoad callback.

Add error description to the DidFailProvisionalLoad callback.

This will add an error description field to the TabContentsObserver::
DidFailProvisionalLoad callback.
The change should not have any impact on current behavior.

This is needed for the Chromium port on Android.

BUG=none
TEST=base_unittests,content_unittests,browser_tests


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

[mmenke, 2011-10-05 16:05:18]

[+jam]

You're going to need a content owner to sign off on this, too.

http://codereview.chromium.org/8142032/diff/1/chrome/renderer/chrome_content_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8142032/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.cc:579: const GURL failed_url =
error.unreachableURL;
nit:  const GURL failed_url(error.unreachableURL);

http://codereview.chromium.org/8142032/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.cc:584:
EqualsASCII(failed_request.httpMethod(), "POST");
We now have this logic in 3 places:  Here, just above, and in RenderView, where
we call this function.  I think we should either add a function here to merge
both copies in this file, or maybe update the interface to take it as an
argument.

http://codereview.chromium.org/8142032/diff/1/chrome/renderer/chrome_content_...
File chrome/renderer/chrome_content_renderer_client.h (right):

http://codereview.chromium.org/8142032/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.h:12: #include "base/string16.h"
nit:  alphabetize.

[jam, 2011-10-05 17:12:06]

http://codereview.chromium.org/8142032/diff/1/chrome/renderer/chrome_content_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8142032/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.cc:536: const GURL
failed_url(error.unreachableURL);
nit: the compiler is smart enough to call the copy constructor here, no need to
change this

http://codereview.chromium.org/8142032/diff/1/content/browser/tab_contents/na...
File content/browser/tab_contents/navigation_controller_unittest.cc (right):

http://codereview.chromium.org/8142032/diff/1/content/browser/tab_contents/na...
content/browser/tab_contents/navigation_controller_unittest.cc:426: {
nit: here and below, no need for scoping

http://codereview.chromium.org/8142032/diff/1/content/browser/tab_contents/ta...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/8142032/diff/1/content/browser/tab_contents/ta...
content/browser/tab_contents/tab_contents.cc:915: const
ViewHostMsg_DidFailProvisionalLoadWithError_Params& ipc_params) {
nit: here and elsewhere, we usually just call this "params"

http://codereview.chromium.org/8142032/diff/1/content/public/renderer/content...
File content/public/renderer/content_renderer_client.h (right):

http://codereview.chromium.org/8142032/diff/1/content/public/renderer/content...
content/public/renderer/content_renderer_client.h:76: virtual string16
GetNavigationErrorDescription(
why add another function instead of an extra parameter? we try to keep embedder
functions to a minimum

http://codereview.chromium.org/8142032/diff/1/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/8142032/diff/1/content/renderer/render_view.cc...
content/renderer/render_view.cc:2344: {
nit: adding an extra scope here is unnecessary. we avoid tabbing if we can

[mmenke, 2011-10-05 17:40:11]

http://codereview.chromium.org/8142032/diff/1/chrome/renderer/chrome_content_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8142032/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.cc:579: const GURL failed_url =
error.unreachableURL;
On 2011/10/05 16:05:18, Matt Menke wrote:
> nit:  const GURL failed_url(error.unreachableURL);

This comment was made along the line of "if you're going to change one, change
both".  I defer to John's comment above.

[mkosiba (inactive), 2011-10-05 17:53:18]

It's a bit late in my time zone so I'm just responding to feedback. I'm going to
update the change in ~12h.

http://codereview.chromium.org/8142032/diff/1/chrome/renderer/chrome_content_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8142032/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.cc:536: const GURL
failed_url(error.unreachableURL);
On 2011/10/05 17:12:06, John Abd-El-Malek wrote:
> nit: the compiler is smart enough to call the copy constructor here, no need
to
> change this

Ok, I'm going to keep the const in there though.

http://codereview.chromium.org/8142032/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.cc:579: const GURL failed_url =
error.unreachableURL;
On 2011/10/05 16:05:18, Matt Menke wrote:
> nit:  const GURL failed_url(error.unreachableURL);

Done.

http://codereview.chromium.org/8142032/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.cc:584:
EqualsASCII(failed_request.httpMethod(), "POST");
On 2011/10/05 16:05:18, Matt Menke wrote:
> We now have this logic in 3 places:  Here, just above, and in RenderView,
where
> we call this function.  I think we should either add a function here to merge
> both copies in this file, or maybe update the interface to take it as an
> argument.

I don't think making that information a parameter is a good solution, since the
information can already be derived from the parameter itself.
Adding a field to WebURLError seems like overkill so I was thinking of adding a
static bool IsRepostInterstitial(const WebURLError&, const WebURLRequest&);  to
LocalizedError, though LocalizedError probably isn't the best place to put it.

Any ideas?

http://codereview.chromium.org/8142032/diff/1/chrome/renderer/chrome_content_...
File chrome/renderer/chrome_content_renderer_client.h (right):

http://codereview.chromium.org/8142032/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.h:12: #include "base/string16.h"
On 2011/10/05 16:05:18, Matt Menke wrote:
> nit:  alphabetize.

Done.

http://codereview.chromium.org/8142032/diff/1/content/browser/tab_contents/na...
File content/browser/tab_contents/navigation_controller_unittest.cc (right):

http://codereview.chromium.org/8142032/diff/1/content/browser/tab_contents/na...
content/browser/tab_contents/navigation_controller_unittest.cc:426: {
On 2011/10/05 17:12:06, John Abd-El-Malek wrote:
> nit: here and below, no need for scoping

I can remove the scoping. I was trying to make the params variable as local as
possible to the call. Ideally I'd want to be able to create the struct inline.

http://codereview.chromium.org/8142032/diff/1/content/browser/tab_contents/ta...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/8142032/diff/1/content/browser/tab_contents/ta...
content/browser/tab_contents/tab_contents.cc:915: const
ViewHostMsg_DidFailProvisionalLoadWithError_Params& ipc_params) {
On 2011/10/05 17:12:06, John Abd-El-Malek wrote:
> nit: here and elsewhere, we usually just call this "params"

Done.

http://codereview.chromium.org/8142032/diff/1/content/public/renderer/content...
File content/public/renderer/content_renderer_client.h (right):

http://codereview.chromium.org/8142032/diff/1/content/public/renderer/content...
content/public/renderer/content_renderer_client.h:76: virtual string16
GetNavigationErrorDescription(
On 2011/10/05 17:12:06, John Abd-El-Malek wrote:
> why add another function instead of an extra parameter? we try to keep
embedder
> functions to a minimum

You mean have one function return both/either the error description and the
error html? Is it acceptable if the function takes out* pointers that are filled
with data only if the pointer is not null?

http://codereview.chromium.org/8142032/diff/1/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/8142032/diff/1/content/renderer/render_view.cc...
content/renderer/render_view.cc:2344: {
On 2011/10/05 17:12:06, John Abd-El-Malek wrote:
> nit: adding an extra scope here is unnecessary. we avoid tabbing if we can

Done.

[mmenke, 2011-10-05 18:07:28]

http://codereview.chromium.org/8142032/diff/1/chrome/renderer/chrome_content_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8142032/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.cc:584:
EqualsASCII(failed_request.httpMethod(), "POST");
On 2011/10/05 17:53:18, Martin Kosiba wrote:
> On 2011/10/05 16:05:18, Matt Menke wrote:
> > We now have this logic in 3 places:  Here, just above, and in RenderView,
> where
> > we call this function.  I think we should either add a function here to
merge
> > both copies in this file, or maybe update the interface to take it as an
> > argument.
> 
> I don't think making that information a parameter is a good solution, since
the
> information can already be derived from the parameter itself.
> Adding a field to WebURLError seems like overkill so I was thinking of adding
a
> static bool IsRepostInterstitial(const WebURLError&, const WebURLRequest&); 
to
> LocalizedError, though LocalizedError probably isn't the best place to put it.
> 
> Any ideas?

If you end up merging the two functions, as per John's suggestion, I'm happy
with that.  You're right about it not really belonging in LocalizedError. 
content can't depend on LocalizedError, either, so we'd still need it in two
places in that case.  If you keep the functions separate, could just add a
function in an anonymous namespace in this file.  Not a great solution, but
think it'd be better than the duplicate logic.

[jam, 2011-10-05 19:33:57]

http://codereview.chromium.org/8142032/diff/1/content/public/renderer/content...
File content/public/renderer/content_renderer_client.h (right):

http://codereview.chromium.org/8142032/diff/1/content/public/renderer/content...
content/public/renderer/content_renderer_client.h:76: virtual string16
GetNavigationErrorDescription(
On 2011/10/05 17:53:18, Martin Kosiba wrote:
> On 2011/10/05 17:12:06, John Abd-El-Malek wrote:
> > why add another function instead of an extra parameter? we try to keep
> embedder
> > functions to a minimum
> 
> You mean have one function return both/either the error description and the
> error html? Is it acceptable if the function takes out* pointers that are
filled
> with data only if the pointer is not null? 

sure

[mmenke, 2011-10-07 14:19:43]

http://codereview.chromium.org/8142032/diff/14003/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8142032/diff/14003/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:548: if (NULL != error_html) {
nit:  Chrome style is to put the arguments in the other order.  Below, too.

http://codereview.chromium.org/8142032/diff/14003/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/8142032/diff/14003/content/browser/tab_content...
content/browser/tab_contents/tab_contents.h:61: struct
ViewHostMsg_DidFailProvisionalLoadWithError_Params;
nit:  Alphabetize.

http://codereview.chromium.org/8142032/diff/14003/content/browser/tab_content...
content/browser/tab_contents/tab_contents.h:527: const
ViewHostMsg_DidFailProvisionalLoadWithError_Params& ipc_params);
nit:  ipc_params -> params

http://codereview.chromium.org/8142032/diff/14003/content/renderer/mock_conte...
File content/renderer/mock_content_renderer_client.cc (right):

http://codereview.chromium.org/8142032/diff/14003/content/renderer/mock_conte...
content/renderer/mock_content_renderer_client.cc:50: if (NULL != error_html)
nit:  Reverse order here, too, and reduce indent of next line.

http://codereview.chromium.org/8142032/diff/14003/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/8142032/diff/14003/content/renderer/render_vie...
content/renderer/render_view.cc:1257: if (!html.empty())
nit:  Use brackets here, too, since you use them with the else block.

http://codereview.chromium.org/8142032/diff/14003/content/shell/shell_content...
File content/shell/shell_content_renderer_client.cc (right):

http://codereview.chromium.org/8142032/diff/14003/content/shell/shell_content...
content/shell/shell_content_renderer_client.cc:50: *error_html = std::string();
nit:  Should be indenting only 2 here, not 4.

[mkosiba (inactive), 2011-10-07 15:46:11]

http://codereview.chromium.org/8142032/diff/14003/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8142032/diff/14003/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:548: if (NULL != error_html) {
On 2011/10/07 14:19:44, Matt Menke wrote:
> nit:  Chrome style is to put the arguments in the other order.  Below, too.

Done.

http://codereview.chromium.org/8142032/diff/14003/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/8142032/diff/14003/content/browser/tab_content...
content/browser/tab_contents/tab_contents.h:61: struct
ViewHostMsg_DidFailProvisionalLoadWithError_Params;
On 2011/10/07 14:19:44, Matt Menke wrote:
> nit:  Alphabetize.

Done.

http://codereview.chromium.org/8142032/diff/14003/content/browser/tab_content...
content/browser/tab_contents/tab_contents.h:527: const
ViewHostMsg_DidFailProvisionalLoadWithError_Params& ipc_params);
On 2011/10/07 14:19:44, Matt Menke wrote:
> nit:  ipc_params -> params

Done.

http://codereview.chromium.org/8142032/diff/14003/content/renderer/mock_conte...
File content/renderer/mock_content_renderer_client.cc (right):

http://codereview.chromium.org/8142032/diff/14003/content/renderer/mock_conte...
content/renderer/mock_content_renderer_client.cc:50: if (NULL != error_html)
On 2011/10/07 14:19:44, Matt Menke wrote:
> nit:  Reverse order here, too, and reduce indent of next line.

Done.

http://codereview.chromium.org/8142032/diff/14003/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/8142032/diff/14003/content/renderer/render_vie...
content/renderer/render_view.cc:1257: if (!html.empty())
On 2011/10/07 14:19:44, Matt Menke wrote:
> nit:  Use brackets here, too, since you use them with the else block.

Done.

http://codereview.chromium.org/8142032/diff/14003/content/shell/shell_content...
File content/shell/shell_content_renderer_client.cc (right):

http://codereview.chromium.org/8142032/diff/14003/content/shell/shell_content...
content/shell/shell_content_renderer_client.cc:50: *error_html = std::string();
On 2011/10/07 14:19:44, Matt Menke wrote:
> nit:  Should be indenting only 2 here, not 4.

Done.

[jam, 2011-10-07 16:03:28]

just nits

http://codereview.chromium.org/8142032/diff/15002/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8142032/diff/15002/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:548: if (error_html != NULL) {
nit: here and below, convention is to drop the "!= NULL", i.e. just if
(error_html)

http://codereview.chromium.org/8142032/diff/15002/content/renderer/mock_conte...
File content/renderer/mock_content_renderer_client.cc (right):

http://codereview.chromium.org/8142032/diff/15002/content/renderer/mock_conte...
content/renderer/mock_content_renderer_client.cc:50: if (error_html != NULL)
nit: we prefer to have empty functions in the mock where possible. the caller
should make sure that they pass empty strings, so that not every embedder has to
do this work.

http://codereview.chromium.org/8142032/diff/15002/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/8142032/diff/15002/content/renderer/render_vie...
content/renderer/render_view.cc:2352:
ViewHostMsg_DidFailProvisionalLoadWithError_Params ipc_params;
nit: we usually call this params

[mkosiba (inactive), 2011-10-07 17:54:32]

http://codereview.chromium.org/8142032/diff/15002/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8142032/diff/15002/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:548: if (error_html != NULL) {
On 2011/10/07 16:03:28, John Abd-El-Malek wrote:
> nit: here and below, convention is to drop the "!= NULL", i.e. just if
> (error_html)

Done.

http://codereview.chromium.org/8142032/diff/15002/content/renderer/mock_conte...
File content/renderer/mock_content_renderer_client.cc (right):

http://codereview.chromium.org/8142032/diff/15002/content/renderer/mock_conte...
content/renderer/mock_content_renderer_client.cc:50: if (error_html != NULL)
On 2011/10/07 16:03:28, John Abd-El-Malek wrote:
> nit: we prefer to have empty functions in the mock where possible. the caller
> should make sure that they pass empty strings, so that not every embedder has
to
> do this work.

Done.

http://codereview.chromium.org/8142032/diff/15002/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/8142032/diff/15002/content/renderer/render_vie...
content/renderer/render_view.cc:2352:
ViewHostMsg_DidFailProvisionalLoadWithError_Params ipc_params;
On 2011/10/07 16:03:28, John Abd-El-Malek wrote:
> nit: we usually call this params

Done.

[jam, 2011-10-07 21:13:35]

lgtm

[mmenke, 2011-10-07 21:15:02]

LGTM

[mkosiba (inactive), 2011-10-10 17:35:27]

The refactoring I did based on the feedback caused a unit test to start failing.
The test would hit the NOTREACHED in localized_error because the error domain it
passed in ("", not even "test_domain") was not one of the 2 recognized domains.

The change is to use a well-known error domain in the test.

[mmenke, 2011-10-10 17:41:18]

LGTM, modulo a nit.

http://codereview.chromium.org/8142032/diff/24001/chrome/renderer/localized_e...
File chrome/renderer/localized_error.cc (right):

http://codereview.chromium.org/8142032/diff/24001/chrome/renderer/localized_e...
chrome/renderer/localized_error.cc:8: #include "base/logging.h"
This is where DCHECK is defined.  Think it's best to include it explicitly
rather than relying on some random header to include it.

[mkosiba (inactive), 2011-10-10 18:16:10]

http://codereview.chromium.org/8142032/diff/24001/chrome/renderer/localized_e...
File chrome/renderer/localized_error.cc (right):

http://codereview.chromium.org/8142032/diff/24001/chrome/renderer/localized_e...
chrome/renderer/localized_error.cc:8: #include "base/logging.h"
On 2011/10/10 17:41:18, Matt Menke wrote:
> This is where DCHECK is defined.  Think it's best to include it explicitly
> rather than relying on some random header to include it.

Done.

[commit-bot: I haz the power, 2011-10-11 09:18:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/8142032/27001

[commit-bot: I haz the power, 2011-10-11 09:18:18]

Can't apply patch for file content/renderer/render_view.cc.
While running patch -p1 --forward --force;
patching file content/renderer/render_view.cc
Hunk #1 FAILED at 1259.
Hunk #2 FAILED at 2352.
2 out of 2 hunks FAILED -- saving rejects to file
content/renderer/render_view.cc.rej

[commit-bot: I haz the power, 2011-10-11 09:36:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/8142032/30001

[commit-bot: I haz the power, 2011-10-11 09:38:23]

Try job failure for 8142032-30001 on linux_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2011-10-11 12:28:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/8142032/30001

[commit-bot: I haz the power, 2011-10-11 15:32:00]

Change committed as 104895