Componentize NetErrorHelperCore

Componentize NetErrorHelperCore

Create a new component error_page which will host NetErrorHelper to make it usable from binaries other than chrome.

Move NetErrorHelperCore and net_error_info.{cc,h} to components/error_page.
Split LocalizedError::ErrorPageParams to components/error_page/common/error_page_params.
Move string resources to error_page_strings.grdp
Fix GYP, GN, DEPS.
Copy OWNERS from chrome/renderer/net to components/error_page.

BUG=398173
TEST=build
TBR=sky@chromium.org for +ui/base in DEPS

Committed: https://crrev.com/9b160e24e7f967dda04cac13d393124ba173a0e8
Cr-Commit-Position: refs/heads/master@{#299647}

[hashimoto, 2014-09-16 09:03:47]

Patchset #2 (id:20001) has been deleted

[hashimoto, 2014-09-16 09:05:49]

hashimoto@chromium.org changed reviewers:
+ blundell@chromium.org, mmenke@chromium.org

[hashimoto, 2014-09-16 09:05:50]

mmenke@: Could you review this change as an owner of the code being moved?
blundell@: Could you review this change as an owner of components?

[hashimoto, 2014-09-16 09:11:20]

Patchset #1 (id:1) has been deleted

[blundell, 2014-09-16 10:20:03]

//components LGTM.
Please add the new grdp file on the internal side once this lands.

[mmenke, 2014-09-16 14:33:59]

[+cc ttuttle, ellyjones, rdsmith]:  Just FYI

[mmenke, 2014-09-16 14:45:05]

I support componentizing this logic (iOS, for instance, needs to have error page
logic componentized, though I suppose all it currently uses is probably
LocalizedError.h), but I want to think about the choice of where to slice
things.

https://codereview.chromium.org/570253002/diff/60001/components/error_page/OW...
File components/error_page/OWNERS (right):

https://codereview.chromium.org/570253002/diff/60001/components/error_page/OW...
components/error_page/OWNERS:1: jar@chromium.org
Should remove jar from this list, so reviews for this code don't go to him.

https://codereview.chromium.org/570253002/diff/60001/components/error_page/co...
File components/error_page/common/error_page_params.h (right):

https://codereview.chromium.org/570253002/diff/60001/components/error_page/co...
components/error_page/common/error_page_params.h:20: struct ErrorPageParams {
It seems kinda weird to separate this out from LocalizedError.  Do you have any
plans to actually hook this up?

[hashimoto, 2014-09-16 16:12:03]

https://codereview.chromium.org/570253002/diff/60001/components/error_page/OW...
File components/error_page/OWNERS (right):

https://codereview.chromium.org/570253002/diff/60001/components/error_page/OW...
components/error_page/OWNERS:1: jar@chromium.org
On 2014/09/16 14:45:05, mmenke wrote:
> Should remove jar from this list, so reviews for this code don't go to him.

Done.

https://codereview.chromium.org/570253002/diff/60001/components/error_page/co...
File components/error_page/common/error_page_params.h (right):

https://codereview.chromium.org/570253002/diff/60001/components/error_page/co...
components/error_page/common/error_page_params.h:20: struct ErrorPageParams {
On 2014/09/16 14:45:05, mmenke wrote:
> It seems kinda weird to separate this out from LocalizedError.  Do you have
any
> plans to actually hook this up?

It might look weird when you consider ErrorPageParams as a part of
LocalizedError interface.
But, I think it might make sense if you consider this struct as a part of
NetErrorHelperCore::Delegate interface.
Since, ideally, NetErrorHelperCore shouldn't be aware of how the delegate
implements GenerateLocalizedErrorPage() method, I think NetErrorHelperCore
shouldn't depend on LocalizedError.


Regarding LocalizedError itself, I cannot be sure when it gets componentized
because it's using a number of string resources from chromium_strings.h.
Since componentized code can be used from binaries other than chrome (e.g.
athena and app_shell), some strings from chromium_strings.h might be too
chrome-specific to be used on other binaries.

My current plan is to componentize LocalizedError in long term.
For short term, however, I want to introduce an interface which abstracts
LocalizedError so chrome can use LocalizedError to return the real strings while
other binaries like athena can implement a stub which returns almost nothing.
Does this sound good to you?
Should we immediately componentize LocalizedError instead?

[mmenke, 2014-09-16 18:02:41]

On 2014/09/16 16:12:03, hashimoto wrote:
>
https://codereview.chromium.org/570253002/diff/60001/components/error_page/OW...
> File components/error_page/OWNERS (right):
> 
>
https://codereview.chromium.org/570253002/diff/60001/components/error_page/OW...
> components/error_page/OWNERS:1: mailto:jar@chromium.org
> On 2014/09/16 14:45:05, mmenke wrote:
> > Should remove jar from this list, so reviews for this code don't go to him.
> 
> Done.
> 
>
https://codereview.chromium.org/570253002/diff/60001/components/error_page/co...
> File components/error_page/common/error_page_params.h (right):
> 
>
https://codereview.chromium.org/570253002/diff/60001/components/error_page/co...
> components/error_page/common/error_page_params.h:20: struct ErrorPageParams {
> On 2014/09/16 14:45:05, mmenke wrote:
> > It seems kinda weird to separate this out from LocalizedError.  Do you have
> any
> > plans to actually hook this up?
> 
> It might look weird when you consider ErrorPageParams as a part of
> LocalizedError interface.
> But, I think it might make sense if you consider this struct as a part of
> NetErrorHelperCore::Delegate interface.
> Since, ideally, NetErrorHelperCore shouldn't be aware of how the delegate
> implements GenerateLocalizedErrorPage() method, I think NetErrorHelperCore
> shouldn't depend on LocalizedError.
> 
> 
> Regarding LocalizedError itself, I cannot be sure when it gets componentized
> because it's using a number of string resources from chromium_strings.h.
> Since componentized code can be used from binaries other than chrome (e.g.
> athena and app_shell), some strings from chromium_strings.h might be too
> chrome-specific to be used on other binaries.
> 
> My current plan is to componentize LocalizedError in long term.
> For short term, however, I want to introduce an interface which abstracts
> LocalizedError so chrome can use LocalizedError to return the real strings
while
> other binaries like athena can implement a stub which returns almost nothing.
> Does this sound good to you?
> Should we immediately componentize LocalizedError instead?

Let me think about it a bit.  Not going to have time to get to this today,
unfortunately, between perf and meetings.

[blundell, 2014-09-17 12:02:09]

On 2014/09/16 18:02:41, mmenke wrote:
> On 2014/09/16 16:12:03, hashimoto wrote:
> >
>
https://codereview.chromium.org/570253002/diff/60001/components/error_page/OW...
> > File components/error_page/OWNERS (right):
> > 
> >
>
https://codereview.chromium.org/570253002/diff/60001/components/error_page/OW...
> > components/error_page/OWNERS:1: mailto:jar@chromium.org
> > On 2014/09/16 14:45:05, mmenke wrote:
> > > Should remove jar from this list, so reviews for this code don't go to
him.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/570253002/diff/60001/components/error_page/co...
> > File components/error_page/common/error_page_params.h (right):
> > 
> >
>
https://codereview.chromium.org/570253002/diff/60001/components/error_page/co...
> > components/error_page/common/error_page_params.h:20: struct ErrorPageParams
{
> > On 2014/09/16 14:45:05, mmenke wrote:
> > > It seems kinda weird to separate this out from LocalizedError.  Do you
have
> > any
> > > plans to actually hook this up?
> > 
> > It might look weird when you consider ErrorPageParams as a part of
> > LocalizedError interface.
> > But, I think it might make sense if you consider this struct as a part of
> > NetErrorHelperCore::Delegate interface.
> > Since, ideally, NetErrorHelperCore shouldn't be aware of how the delegate
> > implements GenerateLocalizedErrorPage() method, I think NetErrorHelperCore
> > shouldn't depend on LocalizedError.
> > 
> > 
> > Regarding LocalizedError itself, I cannot be sure when it gets componentized
> > because it's using a number of string resources from chromium_strings.h.
> > Since componentized code can be used from binaries other than chrome (e.g.
> > athena and app_shell), some strings from chromium_strings.h might be too
> > chrome-specific to be used on other binaries.
> > 
> > My current plan is to componentize LocalizedError in long term.
> > For short term, however, I want to introduce an interface which abstracts
> > LocalizedError so chrome can use LocalizedError to return the real strings
> while
> > other binaries like athena can implement a stub which returns almost
nothing.
> > Does this sound good to you?
> > Should we immediately componentize LocalizedError instead?
> 
> Let me think about it a bit.  Not going to have time to get to this today,
> unfortunately, between perf and meetings.

The iOS port needs LocalizedError, fwiw.

[mmenke, 2014-09-18 17:50:43]

I am really concerned about layering violations here.  If you can clean them all
up, I think we can live with this for now (Though due to iOS, we'll presumably
want to move LocalizedError into this component at some point).

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
File components/error_page/common/error_page_params.h (right):

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
components/error_page/common/error_page_params.h:29: // optionally contain a
body as well.  Must not be NULL.
If this is going to be in a component, this comment should have more meat.

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
File components/error_page/common/net_error_info.cc (right):

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
components/error_page/common/net_error_info.cc:7: #include
"components/error_page/common/net_error_info.h"
nit:  This should be first (Know it was in the wrong spot before)

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
File components/error_page/common/net_error_info.h (right):

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
components/error_page/common/net_error_info.h:8: // TODO(hashimoto): Change this
to namespace error_page.
Planning to do this in a followup CL?

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
components/error_page/common/net_error_info.h:26:
NETWORK_ERROR_PAGE_BROWSER_INITIATED_RELOAD,   // Reload from browser.
Having this event recorded from outside the component seems a little weird.  Not
sure if we can do better, though.

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
components/error_page/common/net_error_info.h:31: // The status of a DNS probe
that the NetErrorTabHelper may or may not have
Mentioning "NetErrorTabHelper" is a layering violation.

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
components/error_page/common/net_error_info.h:77: // localized error system, and
we don't want to require the caller to cast back
This comment is a bit weird - it talks about specific behavior of
chrome/renderer code.

https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
File components/error_page/renderer/net_error_helper_core.cc (right):

https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
components/error_page/renderer/net_error_helper_core.cc:31: #include
"third_party/WebKit/public/platform/WebURLError.h"
Can we get rid of these dependencies?

https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
components/error_page/renderer/net_error_helper_core.cc:546: else if (url !=
GURL(content::kUnreachableWebDataURL))
Can we get rid of the content dependency?  Have the NetErrorHelper do this
check?  Wasn't worth the effort before, but now, seems silly to depend on
content for this.

https://codereview.chromium.org/570253002/diff/80001/components/error_page_st...
File components/error_page_strings.grdp (right):

https://codereview.chromium.org/570253002/diff/80001/components/error_page_st...
components/error_page_strings.grdp:11: Go to <ph name="LINK">&lt;a
jsvalues="href:urlCorrection;.jstdata:$this"
onmousedown="linkClicked(this.jstdata)"
jscontent="urlCorrectionForDisplay"&gt;&lt;/a&gt;<ex>www.somewhere.com</ex></ph>
These seem really weird - they call functions from outside the component.

[blundell, 2014-09-19 07:16:42]

On 2014/09/18 17:50:43, mmenke wrote:
> I am really concerned about layering violations here.  If you can clean them
all
> up, I think we can live with this for now (Though due to iOS, we'll presumably
> want to move LocalizedError into this component at some point).
> 
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> File components/error_page/common/error_page_params.h (right):
> 
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> components/error_page/common/error_page_params.h:29: // optionally contain a
> body as well.  Must not be NULL.
> If this is going to be in a component, this comment should have more meat.
> 
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> File components/error_page/common/net_error_info.cc (right):
> 
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> components/error_page/common/net_error_info.cc:7: #include
> "components/error_page/common/net_error_info.h"
> nit:  This should be first (Know it was in the wrong spot before)
> 
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> File components/error_page/common/net_error_info.h (right):
> 
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> components/error_page/common/net_error_info.h:8: // TODO(hashimoto): Change
this
> to namespace error_page.
> Planning to do this in a followup CL?
> 
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> components/error_page/common/net_error_info.h:26:
> NETWORK_ERROR_PAGE_BROWSER_INITIATED_RELOAD,   // Reload from browser.
> Having this event recorded from outside the component seems a little weird. 
Not
> sure if we can do better, though.
> 
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> components/error_page/common/net_error_info.h:31: // The status of a DNS probe
> that the NetErrorTabHelper may or may not have
> Mentioning "NetErrorTabHelper" is a layering violation.
> 
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> components/error_page/common/net_error_info.h:77: // localized error system,
and
> we don't want to require the caller to cast back
> This comment is a bit weird - it talks about specific behavior of
> chrome/renderer code.
> 
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
> File components/error_page/renderer/net_error_helper_core.cc (right):
> 
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
> components/error_page/renderer/net_error_helper_core.cc:31: #include
> "third_party/WebKit/public/platform/WebURLError.h"
> Can we get rid of these dependencies?
> 
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
> components/error_page/renderer/net_error_helper_core.cc:546: else if (url !=
> GURL(content::kUnreachableWebDataURL))
> Can we get rid of the content dependency?  Have the NetErrorHelper do this
> check?  Wasn't worth the effort before, but now, seems silly to depend on
> content for this.
> 
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page_st...
> File components/error_page_strings.grdp (right):
> 
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page_st...
> components/error_page_strings.grdp:11: Go to <ph name="LINK">&lt;a
> jsvalues="href:urlCorrection;.jstdata:$this"
> onmousedown="linkClicked(this.jstdata)"
>
jscontent="urlCorrectionForDisplay"&gt;&lt;/a&gt;<ex>www.somewhere.com</ex></ph>
> These seem really weird - they call functions from outside the component.

Given that we will want LocalizedError componentized eventually, does it make
sense to just go the whole way now?

[hashimoto, 2014-09-22 09:28:12]

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
File components/error_page/common/error_page_params.h (right):

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
components/error_page/common/error_page_params.h:29: // optionally contain a
body as well.  Must not be NULL.
On 2014/09/18 17:50:42, mmenke wrote:
> If this is going to be in a component, this comment should have more meat.
Sorry, I cannot be sure how I should rewrite this comment as the contents of
this variable is totally determined by the undocumented behavior of the Link
Doctor server. (e.g. Does "optionally contain a body" mean all fields other than
"header" is optional?)

Can't we leave this comment unchanged as NetErrorHelper and LocalizedError will
be componentized later as well?

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
File components/error_page/common/net_error_info.cc (right):

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
components/error_page/common/net_error_info.cc:7: #include
"components/error_page/common/net_error_info.h"
On 2014/09/18 17:50:42, mmenke wrote:
> nit:  This should be first (Know it was in the wrong spot before)

Done.

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
File components/error_page/common/net_error_info.h (right):

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
components/error_page/common/net_error_info.h:8: // TODO(hashimoto): Change this
to namespace error_page.
On 2014/09/18 17:50:42, mmenke wrote:
> Planning to do this in a followup CL?

Yes I will.
Let me leave this for now as it will result in touching random files which use
enums below.

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
components/error_page/common/net_error_info.h:26:
NETWORK_ERROR_PAGE_BROWSER_INITIATED_RELOAD,   // Reload from browser.
On 2014/09/18 17:50:42, mmenke wrote:
> Having this event recorded from outside the component seems a little weird. 
Not
> sure if we can do better, though.

Eventually we may componentize net_error_tab_helper.cc so all users of
RecordEvent() move under components/error_page.
(I cannot be sure when it will happen, though)

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
components/error_page/common/net_error_info.h:31: // The status of a DNS probe
that the NetErrorTabHelper may or may not have
On 2014/09/18 17:50:42, mmenke wrote:
> Mentioning "NetErrorTabHelper" is a layering violation.

Removed the said part.

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
components/error_page/common/net_error_info.h:77: // localized error system, and
we don't want to require the caller to cast back
On 2014/09/18 17:50:42, mmenke wrote:
> This comment is a bit weird - it talks about specific behavior of
> chrome/renderer code.

Removed the first sentence.
This comment makes sense even without it.

https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
File components/error_page/renderer/net_error_helper_core.cc (right):

https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
components/error_page/renderer/net_error_helper_core.cc:31: #include
"third_party/WebKit/public/platform/WebURLError.h"
On 2014/09/18 17:50:42, mmenke wrote:
> Can we get rid of these dependencies?

It might be possible if blink::WebURLError is removed from
NetErrorHelperCore::ErrorPageInfo after massive refactoring.
Do we have a strong reason to remove blink dependencies from renderer code?

https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
components/error_page/renderer/net_error_helper_core.cc:546: else if (url !=
GURL(content::kUnreachableWebDataURL))
On 2014/09/18 17:50:42, mmenke wrote:
> Can we get rid of the content dependency?  Have the NetErrorHelper do this
> check?  Wasn't worth the effort before, but now, seems silly to depend on
> content for this.

Do we need to remove content dependencies from renderer code?
NetErrorHelper will be also componentized and it will be useless without content
dependencies.

https://codereview.chromium.org/570253002/diff/80001/components/error_page_st...
File components/error_page_strings.grdp (right):

https://codereview.chromium.org/570253002/diff/80001/components/error_page_st...
components/error_page_strings.grdp:11: Go to <ph name="LINK">&lt;a
jsvalues="href:urlCorrection;.jstdata:$this"
onmousedown="linkClicked(this.jstdata)"
jscontent="urlCorrectionForDisplay"&gt;&lt;/a&gt;<ex>www.somewhere.com</ex></ph>
On 2014/09/18 17:50:42, mmenke wrote:
> These seem really weird - they call functions from outside the component.

Oops, I didn't expect string resources to contain JS code.

The file which contains the function linkClicked() will be componentized when
it's exclusive user, NetErrorHelper, gets componentized.
If it's not OK to leave these string resources in this state until then, maybe
we should componentize these files (neterror.js and its relatives) even though
it's not used by anyone in this component yet?

[hashimoto, 2014-09-22 09:39:44]

On 2014/09/19 07:16:42, blundell wrote:
> On 2014/09/18 17:50:43, mmenke wrote:
> > I am really concerned about layering violations here.  If you can clean them
> all
> > up, I think we can live with this for now (Though due to iOS, we'll
presumably
> > want to move LocalizedError into this component at some point).
> > 
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > File components/error_page/common/error_page_params.h (right):
> > 
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > components/error_page/common/error_page_params.h:29: // optionally contain a
> > body as well.  Must not be NULL.
> > If this is going to be in a component, this comment should have more meat.
> > 
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > File components/error_page/common/net_error_info.cc (right):
> > 
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > components/error_page/common/net_error_info.cc:7: #include
> > "components/error_page/common/net_error_info.h"
> > nit:  This should be first (Know it was in the wrong spot before)
> > 
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > File components/error_page/common/net_error_info.h (right):
> > 
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > components/error_page/common/net_error_info.h:8: // TODO(hashimoto): Change
> this
> > to namespace error_page.
> > Planning to do this in a followup CL?
> > 
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > components/error_page/common/net_error_info.h:26:
> > NETWORK_ERROR_PAGE_BROWSER_INITIATED_RELOAD,   // Reload from browser.
> > Having this event recorded from outside the component seems a little weird. 
> Not
> > sure if we can do better, though.
> > 
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > components/error_page/common/net_error_info.h:31: // The status of a DNS
probe
> > that the NetErrorTabHelper may or may not have
> > Mentioning "NetErrorTabHelper" is a layering violation.
> > 
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > components/error_page/common/net_error_info.h:77: // localized error system,
> and
> > we don't want to require the caller to cast back
> > This comment is a bit weird - it talks about specific behavior of
> > chrome/renderer code.
> > 
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
> > File components/error_page/renderer/net_error_helper_core.cc (right):
> > 
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
> > components/error_page/renderer/net_error_helper_core.cc:31: #include
> > "third_party/WebKit/public/platform/WebURLError.h"
> > Can we get rid of these dependencies?
> > 
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
> > components/error_page/renderer/net_error_helper_core.cc:546: else if (url !=
> > GURL(content::kUnreachableWebDataURL))
> > Can we get rid of the content dependency?  Have the NetErrorHelper do this
> > check?  Wasn't worth the effort before, but now, seems silly to depend on
> > content for this.
> > 
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page_st...
> > File components/error_page_strings.grdp (right):
> > 
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page_st...
> > components/error_page_strings.grdp:11: Go to <ph name="LINK">&lt;a
> > jsvalues="href:urlCorrection;.jstdata:$this"
> > onmousedown="linkClicked(this.jstdata)"
> >
>
jscontent="urlCorrectionForDisplay"&gt;&lt;/a&gt;<ex>www.somewhere.com</ex></ph>
> > These seem really weird - they call functions from outside the component.
> 
> Given that we will want LocalizedError componentized eventually, does it make
> sense to just go the whole way now?

Let me componentize LocalizedError separately as it requires some amount of
effort to get componentized.

It requires massive componentization of IDS_ERRORPAGES_* string resources. 
Also, it depends on strings defined in
chromium_strings.grd/google_chrome_strings.grd.
These strings should be changed to take the product name as a parameter, or
should be injected through a client.
Strings like IDS_SETTINGS_TITLE, IDS_SETTINGS_SHOW_ADVANCED_SETTINGS,
IDS_NETWORK_PREDICTION_ENABLED_DESCRIPTION, IDS_OPTIONS_PROXIES_CONFIGURE_BUTTON
are also needed, but it seems they cannot be componentized as they are widely
used by other chrome code.

Adding to that, LocalizedError contains extensions dependency.
I think we should purge LocalizedError::GetAppErrorStrings() which needs
extensions dependency before componentizing LocalizedError.

[mmenke, 2014-09-23 15:04:25]

On 2014/09/22 09:39:44, hashimoto wrote:
> On 2014/09/19 07:16:42, blundell wrote:
> > On 2014/09/18 17:50:43, mmenke wrote:
> > > I am really concerned about layering violations here.  If you can clean
them
> > all
> > > up, I think we can live with this for now (Though due to iOS, we'll
> presumably
> > > want to move LocalizedError into this component at some point).
> > > 
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > > File components/error_page/common/error_page_params.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > > components/error_page/common/error_page_params.h:29: // optionally contain
a
> > > body as well.  Must not be NULL.
> > > If this is going to be in a component, this comment should have more meat.
> > > 
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > > File components/error_page/common/net_error_info.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > > components/error_page/common/net_error_info.cc:7: #include
> > > "components/error_page/common/net_error_info.h"
> > > nit:  This should be first (Know it was in the wrong spot before)
> > > 
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > > File components/error_page/common/net_error_info.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > > components/error_page/common/net_error_info.h:8: // TODO(hashimoto):
Change
> > this
> > > to namespace error_page.
> > > Planning to do this in a followup CL?
> > > 
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > > components/error_page/common/net_error_info.h:26:
> > > NETWORK_ERROR_PAGE_BROWSER_INITIATED_RELOAD,   // Reload from browser.
> > > Having this event recorded from outside the component seems a little
weird. 
> > Not
> > > sure if we can do better, though.
> > > 
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > > components/error_page/common/net_error_info.h:31: // The status of a DNS
> probe
> > > that the NetErrorTabHelper may or may not have
> > > Mentioning "NetErrorTabHelper" is a layering violation.
> > > 
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > > components/error_page/common/net_error_info.h:77: // localized error
system,
> > and
> > > we don't want to require the caller to cast back
> > > This comment is a bit weird - it talks about specific behavior of
> > > chrome/renderer code.
> > > 
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
> > > File components/error_page/renderer/net_error_helper_core.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
> > > components/error_page/renderer/net_error_helper_core.cc:31: #include
> > > "third_party/WebKit/public/platform/WebURLError.h"
> > > Can we get rid of these dependencies?
> > > 
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
> > > components/error_page/renderer/net_error_helper_core.cc:546: else if (url
!=
> > > GURL(content::kUnreachableWebDataURL))
> > > Can we get rid of the content dependency?  Have the NetErrorHelper do this
> > > check?  Wasn't worth the effort before, but now, seems silly to depend on
> > > content for this.
> > > 
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page_st...
> > > File components/error_page_strings.grdp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page_st...
> > > components/error_page_strings.grdp:11: Go to <ph name="LINK">&lt;a
> > > jsvalues="href:urlCorrection;.jstdata:$this"
> > > onmousedown="linkClicked(this.jstdata)"
> > >
> >
>
jscontent="urlCorrectionForDisplay"&gt;&lt;/a&gt;<ex>www.somewhere.com</ex></ph>
> > > These seem really weird - they call functions from outside the component.
> > 
> > Given that we will want LocalizedError componentized eventually, does it
make
> > sense to just go the whole way now?
> 
> Let me componentize LocalizedError separately as it requires some amount of
> effort to get componentized.
> 
> It requires massive componentization of IDS_ERRORPAGES_* string resources. 
> Also, it depends on strings defined in
> chromium_strings.grd/google_chrome_strings.grd.
> These strings should be changed to take the product name as a parameter, or
> should be injected through a client.

Taking a product name as a parameter does not internationalize, unfortunately,
since nouns inflect in some languages, so that's a non-starter.

> Strings like IDS_SETTINGS_TITLE, IDS_SETTINGS_SHOW_ADVANCED_SETTINGS,
> IDS_NETWORK_PREDICTION_ENABLED_DESCRIPTION,
IDS_OPTIONS_PROXIES_CONFIGURE_BUTTON
> are also needed, but it seems they cannot be componentized as they are widely
> used by other chrome code.
> 
> Adding to that, LocalizedError contains extensions dependency.
> I think we should purge LocalizedError::GetAppErrorStrings() which needs
> extensions dependency before componentizing LocalizedError.

I'm fine with moving GetAppErrorStrings elsewhere.  I'd actually rather just get
rid of a different, less useful, error page for URLs associated with apps, but
that's not my call.

[mmenke, 2014-09-23 15:04:34]

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
File components/error_page/common/error_page_params.h (right):

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
components/error_page/common/error_page_params.h:29: // optionally contain a
body as well.  Must not be NULL.
On 2014/09/22 09:28:11, hashimoto wrote:
> On 2014/09/18 17:50:42, mmenke wrote:
> > If this is going to be in a component, this comment should have more meat.
> Sorry, I cannot be sure how I should rewrite this comment as the contents of
> this variable is totally determined by the undocumented behavior of the Link
> Doctor server. (e.g. Does "optionally contain a body" mean all fields other
than
> "header" is optional?)
> 
> Can't we leave this comment unchanged as NetErrorHelper and LocalizedError
will
> be componentized later as well?

The format of the suggestions depends on LocalizedError's logic and resources,
not the Link Doctor.

https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
File components/error_page/renderer/net_error_helper_core.cc (right):

https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
components/error_page/renderer/net_error_helper_core.cc:31: #include
"third_party/WebKit/public/platform/WebURLError.h"
On 2014/09/22 09:28:12, hashimoto wrote:
> On 2014/09/18 17:50:42, mmenke wrote:
> > Can we get rid of these dependencies?
> 
> It might be possible if blink::WebURLError is removed from
> NetErrorHelperCore::ErrorPageInfo after massive refactoring.
> Do we have a strong reason to remove blink dependencies from renderer code?

See comment below (Though I guess we're already using LocalizedError on iOS, so
we presumably have access to the WebURLError struct on iOS, anyways).

https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
components/error_page/renderer/net_error_helper_core.cc:546: else if (url !=
GURL(content::kUnreachableWebDataURL))
On 2014/09/22 09:28:12, hashimoto wrote:
> On 2014/09/18 17:50:42, mmenke wrote:
> > Can we get rid of the content dependency?  Have the NetErrorHelper do this
> > check?  Wasn't worth the effort before, but now, seems silly to depend on
> > content for this.
> 
> Do we need to remove content dependencies from renderer code?
> NetErrorHelper will be also componentized and it will be useless without
content
> dependencies.

I had been thinking that we'd move LocalizedError into this component as
well...And we, of course, don't want content/ dependencies in iOS code.  If the
plan is to componentize LocalizedError into a separate component, and move
NetErrorHelper into this one, I'm fine with the dependency.

https://codereview.chromium.org/570253002/diff/80001/components/error_page_st...
File components/error_page_strings.grdp (right):

https://codereview.chromium.org/570253002/diff/80001/components/error_page_st...
components/error_page_strings.grdp:11: Go to <ph name="LINK">&lt;a
jsvalues="href:urlCorrection;.jstdata:$this"
onmousedown="linkClicked(this.jstdata)"
jscontent="urlCorrectionForDisplay"&gt;&lt;/a&gt;<ex>www.somewhere.com</ex></ph>
On 2014/09/22 09:28:12, hashimoto wrote:
> On 2014/09/18 17:50:42, mmenke wrote:
> > These seem really weird - they call functions from outside the component.
> 
> Oops, I didn't expect string resources to contain JS code.
> 
> The file which contains the function linkClicked() will be componentized when
> it's exclusive user, NetErrorHelper, gets componentized.
> If it's not OK to leave these string resources in this state until then, maybe
> we should componentize these files (neterror.js and its relatives) even though
> it's not used by anyone in this component yet?

neterror.js is used on iOS, so it should not be part of a component that depends
on content/.  Any component that includes NetErrorHelper will have to depend on
content/.  (With just NetErrorHelperCore, that dependency could trivially be
removed)

[mmenke, 2014-09-23 15:06:21]

One final comment:  I'd like to arrive at a consensus on the path forward before
signing off on this (How to handle neterror.html and other resources,
particularly) before signing off on this CL.

[blundell, 2014-09-23 15:08:14]

The component could have core/ and content/ subdirectories, with the renderer
code living in the content/renderer subdirectory. iOS would use targets building
the code in the core/ subdirectory, which couldn't depend on //content. The code
in the content/ subdirectory would be free to depend on //content.

[hashimoto, 2014-09-24 10:31:17]

On 2014/09/23 15:04:25, mmenke wrote:
> On 2014/09/22 09:39:44, hashimoto wrote:
> > On 2014/09/19 07:16:42, blundell wrote:
> > > On 2014/09/18 17:50:43, mmenke wrote:
> > > > I am really concerned about layering violations here.  If you can clean
> them
> > > all
> > > > up, I think we can live with this for now (Though due to iOS, we'll
> > presumably
> > > > want to move LocalizedError into this component at some point).
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > > > File components/error_page/common/error_page_params.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > > > components/error_page/common/error_page_params.h:29: // optionally
contain
> a
> > > > body as well.  Must not be NULL.
> > > > If this is going to be in a component, this comment should have more
meat.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > > > File components/error_page/common/net_error_info.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > > > components/error_page/common/net_error_info.cc:7: #include
> > > > "components/error_page/common/net_error_info.h"
> > > > nit:  This should be first (Know it was in the wrong spot before)
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > > > File components/error_page/common/net_error_info.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > > > components/error_page/common/net_error_info.h:8: // TODO(hashimoto):
> Change
> > > this
> > > > to namespace error_page.
> > > > Planning to do this in a followup CL?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > > > components/error_page/common/net_error_info.h:26:
> > > > NETWORK_ERROR_PAGE_BROWSER_INITIATED_RELOAD,   // Reload from browser.
> > > > Having this event recorded from outside the component seems a little
> weird. 
> > > Not
> > > > sure if we can do better, though.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > > > components/error_page/common/net_error_info.h:31: // The status of a DNS
> > probe
> > > > that the NetErrorTabHelper may or may not have
> > > > Mentioning "NetErrorTabHelper" is a layering violation.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> > > > components/error_page/common/net_error_info.h:77: // localized error
> system,
> > > and
> > > > we don't want to require the caller to cast back
> > > > This comment is a bit weird - it talks about specific behavior of
> > > > chrome/renderer code.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
> > > > File components/error_page/renderer/net_error_helper_core.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
> > > > components/error_page/renderer/net_error_helper_core.cc:31: #include
> > > > "third_party/WebKit/public/platform/WebURLError.h"
> > > > Can we get rid of these dependencies?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
> > > > components/error_page/renderer/net_error_helper_core.cc:546: else if
(url
> !=
> > > > GURL(content::kUnreachableWebDataURL))
> > > > Can we get rid of the content dependency?  Have the NetErrorHelper do
this
> > > > check?  Wasn't worth the effort before, but now, seems silly to depend
on
> > > > content for this.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page_st...
> > > > File components/error_page_strings.grdp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page_st...
> > > > components/error_page_strings.grdp:11: Go to <ph name="LINK">&lt;a
> > > > jsvalues="href:urlCorrection;.jstdata:$this"
> > > > onmousedown="linkClicked(this.jstdata)"
> > > >
> > >
> >
>
jscontent="urlCorrectionForDisplay"&gt;&lt;/a&gt;<ex>www.somewhere.com</ex></ph>
> > > > These seem really weird - they call functions from outside the
component.
> > > 
> > > Given that we will want LocalizedError componentized eventually, does it
> make
> > > sense to just go the whole way now?
> > 
> > Let me componentize LocalizedError separately as it requires some amount of
> > effort to get componentized.
> > 
> > It requires massive componentization of IDS_ERRORPAGES_* string resources. 
> > Also, it depends on strings defined in
> > chromium_strings.grd/google_chrome_strings.grd.
> > These strings should be changed to take the product name as a parameter, or
> > should be injected through a client.
> 
> Taking a product name as a parameter does not internationalize, unfortunately,
> since nouns inflect in some languages, so that's a non-starter.
I thought it was OK because LocalizedError was already doing it. (e.g.
IDS_ERRORPAGES_SUMMARY_CONNECTION_REFUSED)
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/generat...)
Since there are already existing examples, can we go ahead with this approach?

blundell@: If PRODUCT_NAME parameter in a string resource is inappropriate, how
should this be resolved?
Should we inject these product-specific strings through client, or should we
introduce a chromium_strings.grd/google_chrome_strings.grd equivalent to
components?
> 
> > Strings like IDS_SETTINGS_TITLE, IDS_SETTINGS_SHOW_ADVANCED_SETTINGS,
> > IDS_NETWORK_PREDICTION_ENABLED_DESCRIPTION,
> IDS_OPTIONS_PROXIES_CONFIGURE_BUTTON
> > are also needed, but it seems they cannot be componentized as they are
widely
> > used by other chrome code.
> > 
> > Adding to that, LocalizedError contains extensions dependency.
> > I think we should purge LocalizedError::GetAppErrorStrings() which needs
> > extensions dependency before componentizing LocalizedError.
> 
> I'm fine with moving GetAppErrorStrings elsewhere.  I'd actually rather just
get
> rid of a different, less useful, error page for URLs associated with apps, but
> that's not my call.

[blundell, 2014-09-24 10:35:08]

>
jscontent="urlCorrectionForDisplay"&gt;&lt;/a&gt;<ex>www.somewhere.com</ex></ph>
> > > > > These seem really weird - they call functions from outside the
> component.
> > > > 
> > > > Given that we will want LocalizedError componentized eventually, does it
> > make
> > > > sense to just go the whole way now?
> > > 
> > > Let me componentize LocalizedError separately as it requires some amount
of
> > > effort to get componentized.
> > > 
> > > It requires massive componentization of IDS_ERRORPAGES_* string resources.

> > > Also, it depends on strings defined in
> > > chromium_strings.grd/google_chrome_strings.grd.
> > > These strings should be changed to take the product name as a parameter,
or
> > > should be injected through a client.
> > 
> > Taking a product name as a parameter does not internationalize,
unfortunately,
> > since nouns inflect in some languages, so that's a non-starter.
> I thought it was OK because LocalizedError was already doing it. (e.g.
> IDS_ERRORPAGES_SUMMARY_CONNECTION_REFUSED)
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/generat...)
> Since there are already existing examples, can we go ahead with this approach?
> 
> blundell@: If PRODUCT_NAME parameter in a string resource is inappropriate,
how
> should this be resolved?
> Should we inject these product-specific strings through client, or should we
> introduce a chromium_strings.grd/google_chrome_strings.grd equivalent to
> components?

Inject the product-specific strings through the client.

[hashimoto, 2014-09-24 10:59:41]

https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
File components/error_page/renderer/net_error_helper_core.cc (right):

https://codereview.chromium.org/570253002/diff/80001/components/error_page/re...
components/error_page/renderer/net_error_helper_core.cc:546: else if (url !=
GURL(content::kUnreachableWebDataURL))
On 2014/09/23 15:04:34, mmenke wrote:
> On 2014/09/22 09:28:12, hashimoto wrote:
> > On 2014/09/18 17:50:42, mmenke wrote:
> > > Can we get rid of the content dependency?  Have the NetErrorHelper do this
> > > check?  Wasn't worth the effort before, but now, seems silly to depend on
> > > content for this.
> > 
> > Do we need to remove content dependencies from renderer code?
> > NetErrorHelper will be also componentized and it will be useless without
> content
> > dependencies.
> 
> I had been thinking that we'd move LocalizedError into this component as
> well...And we, of course, don't want content/ dependencies in iOS code.  If
the
> plan is to componentize LocalizedError into a separate component, and move
> NetErrorHelper into this one, I'm fine with the dependency.

LocalizedError will move to error_page/core/common where there is no content
dependency.

https://codereview.chromium.org/570253002/diff/80001/components/error_page_st...
File components/error_page_strings.grdp (right):

https://codereview.chromium.org/570253002/diff/80001/components/error_page_st...
components/error_page_strings.grdp:11: Go to <ph name="LINK">&lt;a
jsvalues="href:urlCorrection;.jstdata:$this"
onmousedown="linkClicked(this.jstdata)"
jscontent="urlCorrectionForDisplay"&gt;&lt;/a&gt;<ex>www.somewhere.com</ex></ph>
On 2014/09/23 15:04:34, mmenke wrote:
> On 2014/09/22 09:28:12, hashimoto wrote:
> > On 2014/09/18 17:50:42, mmenke wrote:
> > > These seem really weird - they call functions from outside the component.
> > 
> > Oops, I didn't expect string resources to contain JS code.
> > 
> > The file which contains the function linkClicked() will be componentized
when
> > it's exclusive user, NetErrorHelper, gets componentized.
> > If it's not OK to leave these string resources in this state until then,
maybe
> > we should componentize these files (neterror.js and its relatives) even
though
> > it's not used by anyone in this component yet?
> 
> neterror.js is used on iOS, so it should not be part of a component that
depends
> on content/.  Any component that includes NetErrorHelper will have to depend
on
> content/.  (With just NetErrorHelperCore, that dependency could trivially be
> removed)

iOS can depend on content-free part of this component. (i.e. error_page_common)
Auto-reload for iOS will not use NetErrorHelper as iOS doesn't have the idea of
renderer processes.
(You can track the status of iOS's autoreload feature at
http://crbug.com/329623)

[hashimoto, 2014-09-29 07:13:16]

Patchset #6 (id:140001) has been deleted

[hashimoto, 2014-09-29 11:54:12]

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
File components/error_page/common/error_page_params.h (right):

https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
components/error_page/common/error_page_params.h:29: // optionally contain a
body as well.  Must not be NULL.
On 2014/09/23 15:04:34, mmenke wrote:
> On 2014/09/22 09:28:11, hashimoto wrote:
> > On 2014/09/18 17:50:42, mmenke wrote:
> > > If this is going to be in a component, this comment should have more meat.
> > Sorry, I cannot be sure how I should rewrite this comment as the contents of
> > this variable is totally determined by the undocumented behavior of the Link
> > Doctor server. (e.g. Does "optionally contain a body" mean all fields other
> than
> > "header" is optional?)
> > 
> > Can't we leave this comment unchanged as NetErrorHelper and LocalizedError
> will
> > be componentized later as well?
> 
> The format of the suggestions depends on LocalizedError's logic and resources,
> not the Link Doctor.

Made it a bit more descriptive.

Though I'm not sure if the updated comment is correct or not as these data comes
from LinkDoctor, an undocumented data source, and consumed by LocalizedError who
has no documentation about the expected format of |override_suggestions|.
It seems the "header" is the string acquired from the resource bundle which is
shown on the error page, but not sure what "body" in the existing comment means.
All members of |override_suggestions| other than "header" are to be used as
parameters of the header.
Also, I cannot be sure each of "urlCorrection", "urlCorrectionForDisplay" and
"originalUrlForDisplay" is optional or not.
Even if it seems currently "urlCorrection" is always provided (all 3 string
resources are using it), I'm not sure if LinkDoctor may return a response which
does not contain "urlCorrection" in future or not.
In other words, I cannot find any clue which describes the contract made between
LinkDoctor and LocalizdError.

[mmenke, 2014-09-29 19:31:21]

On 2014/09/29 11:54:12, hashimoto wrote:
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> File components/error_page/common/error_page_params.h (right):
> 
>
https://codereview.chromium.org/570253002/diff/80001/components/error_page/co...
> components/error_page/common/error_page_params.h:29: // optionally contain a
> body as well.  Must not be NULL.
> On 2014/09/23 15:04:34, mmenke wrote:
> > On 2014/09/22 09:28:11, hashimoto wrote:
> > > On 2014/09/18 17:50:42, mmenke wrote:
> > > > If this is going to be in a component, this comment should have more
meat.
> > > Sorry, I cannot be sure how I should rewrite this comment as the contents
of
> > > this variable is totally determined by the undocumented behavior of the
Link
> > > Doctor server. (e.g. Does "optionally contain a body" mean all fields
other
> > than
> > > "header" is optional?)
> > > 
> > > Can't we leave this comment unchanged as NetErrorHelper and LocalizedError
> > will
> > > be componentized later as well?
> > 
> > The format of the suggestions depends on LocalizedError's logic and
resources,
> > not the Link Doctor.
> 
> Made it a bit more descriptive.
> 
> Though I'm not sure if the updated comment is correct or not as these data
comes
> from LinkDoctor, an undocumented data source, and consumed by LocalizedError
who
> has no documentation about the expected format of |override_suggestions|.
> It seems the "header" is the string acquired from the resource bundle which is
> shown on the error page, but not sure what "body" in the existing comment
means.
> All members of |override_suggestions| other than "header" are to be used as
> parameters of the header.
> Also, I cannot be sure each of "urlCorrection", "urlCorrectionForDisplay" and
> "originalUrlForDisplay" is optional or not.
> Even if it seems currently "urlCorrection" is always provided (all 3 string
> resources are using it), I'm not sure if LinkDoctor may return a response
which
> does not contain "urlCorrection" in future or not.
> In other words, I cannot find any clue which describes the contract made
between
> LinkDoctor and LocalizdError.

I'll take a look today or tomorrow.  Sorry I've been so unresponsive on this bug
- been overwhelmed by a combination of perf and reviews trying to make it in
before branch point, and de-prioritized other reviews as a result.

[mmenke, 2014-10-01 14:39:10]

LGTM, so sorry for slowness.

https://codereview.chromium.org/570253002/diff/180001/components/error_page/c...
File components/error_page/common/error_page_params.h (right):

https://codereview.chromium.org/570253002/diff/180001/components/error_page/c...
components/error_page/common/error_page_params.h:30: // Must not be NULL.
Sorry, should have provided suggested text for these myself.

Maybe:

"Overrides default suggestions.  Each entry must be a DictionaryValuethat
contains a "header" entry.  A "body" entry may optionally be specified. 
JSTemplate evaluation will be applied when added to the DOM.  If NULL, the
default suggestions will be used.

[hashimoto, 2014-10-02 12:07:40]

https://codereview.chromium.org/570253002/diff/180001/components/error_page/c...
File components/error_page/common/error_page_params.h (right):

https://codereview.chromium.org/570253002/diff/180001/components/error_page/c...
components/error_page/common/error_page_params.h:30: // Must not be NULL.
On 2014/10/01 14:39:10, mmenke wrote:
> Sorry, should have provided suggested text for these myself.
> 
> Maybe:
> 
> "Overrides default suggestions.  Each entry must be a DictionaryValuethat
> contains a "header" entry.  A "body" entry may optionally be specified. 
> JSTemplate evaluation will be applied when added to the DOM.  If NULL, the
> default suggestions will be used.

Thanks, done.

[hashimoto, 2014-10-02 12:09:50]

hashimoto@chromium.org changed reviewers:
+ jochen@chromium.org

[hashimoto, 2014-10-02 12:09:51]

jochen, could you review this change as an owner of
chrome/common/localized_error.{cc,h}?
According to git-log, Nico would be the best reviewer for these files, but he's
not available now.

[jochen (gone - plz use gerrit), 2014-10-02 12:45:36]

https://codereview.chromium.org/570253002/diff/220001/components/error_page/c...
File components/error_page/common/error_page_params.h (right):

https://codereview.chromium.org/570253002/diff/220001/components/error_page/c...
components/error_page/common/error_page_params.h:11: #include "url/gurl.h"
you need to depend on //url to use it

https://codereview.chromium.org/570253002/diff/220001/components/error_page/c...
components/error_page/common/error_page_params.h:32: scoped_ptr<base::ListValue>
override_suggestions;
a scoped_ptr and a struct don't really mix well :-/

[hashimoto, 2014-10-02 14:03:32]

https://codereview.chromium.org/570253002/diff/220001/components/error_page/c...
File components/error_page/common/error_page_params.h (right):

https://codereview.chromium.org/570253002/diff/220001/components/error_page/c...
components/error_page/common/error_page_params.h:11: #include "url/gurl.h"
On 2014/10/02 12:45:35, jochen wrote:
> you need to depend on //url to use it

Added depndencies to error_page.gypi and error_page/common/BUILD.gn.

https://codereview.chromium.org/570253002/diff/220001/components/error_page/c...
components/error_page/common/error_page_params.h:32: scoped_ptr<base::ListValue>
override_suggestions;
On 2014/10/02 12:45:35, jochen wrote:
> a scoped_ptr and a struct don't really mix well :-/

Should I change this to a struct?

[hashimoto, 2014-10-02 15:02:45]

https://codereview.chromium.org/570253002/diff/220001/components/error_page/c...
File components/error_page/common/error_page_params.h (right):

https://codereview.chromium.org/570253002/diff/220001/components/error_page/c...
components/error_page/common/error_page_params.h:32: scoped_ptr<base::ListValue>
override_suggestions;
On 2014/10/02 14:03:32, hashimoto wrote:
> On 2014/10/02 12:45:35, jochen wrote:
> > a scoped_ptr and a struct don't really mix well :-/
> 
> Should I change this to a struct?

Sorry, I mean a class, not a struct.

[blundell, 2014-10-06 05:32:38]

https://codereview.chromium.org/570253002/diff/220001/components/error_page/c...
File components/error_page/common/error_page_params.h (right):

https://codereview.chromium.org/570253002/diff/220001/components/error_page/c...
components/error_page/common/error_page_params.h:32: scoped_ptr<base::ListValue>
override_suggestions;
On 2014/10/02 15:02:45, hashimoto wrote:
> On 2014/10/02 14:03:32, hashimoto wrote:
> > On 2014/10/02 12:45:35, jochen wrote:
> > > a scoped_ptr and a struct don't really mix well :-/
> > 
> > Should I change this to a struct?
> 
> Sorry, I mean a class, not a struct.

Why not have override_suggestions just be a base::ListValue?

[jochen (gone - plz use gerrit), 2014-10-06 07:39:34]

sorry for being unclear.

You shouldn't copy a scoped_ptr<> around. it doesn't matter whether it's a
struct or a class, the point is that you can't copy it.

you could make the params a ref counted class for example. or make it a class
that is non-copyable, so you have to pass it around by reference?

[mmenke, 2014-10-06 15:21:17]

On 2014/10/06 07:39:34, jochen wrote:
> sorry for being unclear.
> 
> You shouldn't copy a scoped_ptr<> around. it doesn't matter whether it's a
> struct or a class, the point is that you can't copy it.
> 
> you could make the params a ref counted class for example. or make it a class
> that is non-copyable, so you have to pass it around by reference?

Right now, it's not copyable (Any struct that uses scoped_ptr gives an error if
you try and copy it).  Could add DISALLOW_COPY_AND_ASSIGN to make it explicit. 
Or could make it a class with a dozen getters and setters, but that really seems
like overkill to me, as it doesn't really get us anything.

We can't pass it by reference, since it's modified in localized_error.  Right
now it's only passed around wrapped in a scoped_ptr.

[jochen (gone - plz use gerrit), 2014-10-07 11:23:41]

oh well, you're only moving the code here, so lgtm as is

[hashimoto, 2014-10-09 08:41:19]

hashimoto@chromium.org changed reviewers:
+ sky@chromium.org

[hashimoto, 2014-10-09 08:41:50]

TBRing sky@chromium.org for +ui/base in DEPS.

[hashimoto, 2014-10-09 08:42:20]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-10-09 08:42:54]

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

[commit-bot: I haz the power, 2014-10-09 14:50:08]

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

[commit-bot: I haz the power, 2014-10-09 14:50:15]

Failed to commit the patch.

[commit-bot: I haz the power, 2014-10-09 14:51:13]

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

[hashimoto, 2014-10-10 02:13:09]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-10-10 02:13:48]

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

[commit-bot: I haz the power, 2014-10-10 02:15:23]

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

[commit-bot: I haz the power, 2014-10-10 02:15:25]

Failed to apply patch for
components/error_page/renderer/net_error_helper_core_unittest.cc:
While running git apply --index -3 -p1;
  error: patch failed: chrome/renderer/net/net_error_helper_core_unittest.cc:293
  Falling back to three-way merge...
  Applied patch to
'components/error_page/renderer/net_error_helper_core_unittest.cc' with
conflicts.
  U components/error_page/renderer/net_error_helper_core_unittest.cc

Patch:   NR 
chrome/renderer/net/net_error_helper_core_unittest.cc->components/error_page/renderer/net_error_helper_core_unittest.cc
Index: components/error_page/renderer/net_error_helper_core_unittest.cc
diff --git a/chrome/renderer/net/net_error_helper_core_unittest.cc
b/components/error_page/renderer/net_error_helper_core_unittest.cc
similarity index 99%
rename from chrome/renderer/net/net_error_helper_core_unittest.cc
rename to components/error_page/renderer/net_error_helper_core_unittest.cc
index
3ff1deeca074cf1b8ac48d24ce2b506df8e56665..24e54ebab0ea3f5fe939e173e343568a891f02a9
100644
--- a/chrome/renderer/net/net_error_helper_core_unittest.cc
+++ b/components/error_page/renderer/net_error_helper_core_unittest.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/renderer/net/net_error_helper_core.h"
+#include "components/error_page/renderer/net_error_helper_core.h"
 
 #include <map>
 #include <string>
@@ -17,18 +17,21 @@
 #include "base/timer/mock_timer.h"
 #include "base/timer/timer.h"
 #include "base/values.h"
-#include "chrome/common/net/net_error_info.h"
+#include "components/error_page/common/error_page_params.h"
+#include "components/error_page/common/net_error_info.h"
 #include "content/public/common/url_constants.h"
 #include "net/base/net_errors.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/WebKit/public/platform/WebURLError.h"
 #include "url/gurl.h"
 
+namespace error_page {
 namespace {
 
 using blink::WebURLError;
 using chrome_common_net::DnsProbeStatus;
 using chrome_common_net::DnsProbeStatusToString;
+using error_page::ErrorPageParams;
 
 const char kFailedUrl[] = "http://failed/";
 const char kFailedHttpsUrl[] = "https://failed/";
@@ -213,7 +216,7 @@ class NetErrorHelperCoreTest : public testing::Test,
   const std::string& last_error_html() const { return last_error_html_; }
   int error_html_update_count() const { return error_html_update_count_; }
 
-  const LocalizedError::ErrorPageParams* last_error_page_params() const {
+  const ErrorPageParams* last_error_page_params() const {
     return last_error_page_params_.get();
   }
 
@@ -293,13 +296,12 @@ class NetErrorHelperCoreTest : public testing::Test,
   }
 
   // NetErrorHelperCore::Delegate implementation:
-  virtual void GenerateLocalizedErrorPage(
-      const WebURLError& error,
-      bool is_failed_post,
-      scoped_ptr<LocalizedError::ErrorPageParams> params,
-      bool* reload_button_shown,
-      bool* load_stale_button_shown,
-      std::string* html) const OVERRIDE {
+  virtual void GenerateLocalizedErrorPage(const WebURLError& error,
+                                          bool is_failed_post,
+                                          scoped_ptr<ErrorPageParams> params,
+                                          bool* reload_button_shown,
+                                          bool* load_stale_button_shown,
+                                          std::string* html) const OVERRIDE {
     last_error_page_params_.reset(params.release());
     *reload_button_shown = false;
     *load_stale_button_shown = false;
@@ -403,7 +405,7 @@ class NetErrorHelperCoreTest : public testing::Test,
   int error_html_update_count_;
 
   // Mutable because GenerateLocalizedErrorPage is const.
-  mutable scoped_ptr<LocalizedError::ErrorPageParams> last_error_page_params_;
+  mutable scoped_ptr<ErrorPageParams> last_error_page_params_;
 
   int reload_count_;
   int load_stale_count_;
@@ -2430,4 +2432,5 @@ TEST_F(NetErrorHelperCoreTest, ExplicitLoadStaleSucceeds)
{
   EXPECT_EQ(GURL(kFailedUrl), load_stale_url());
 }
 
-} // namespace
+}  // namespace
+}  // namespace error_page

[commit-bot: I haz the power, 2014-10-10 02:16:37]

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

[hashimoto, 2014-10-15 02:05:49]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-10-15 02:08:05]

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

[commit-bot: I haz the power, 2014-10-15 03:57:02]

Committed patchset #13 (id:350001)

[commit-bot: I haz the power, 2014-10-15 03:58:17]

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