CrOS online EULA: support localization, change timeout

CrOS online EULA: added mapping from Chrome languages constants to Google URL constants.
Changed fetching timeout to 7 seconds.

BUG=174271
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199692

[vasilii, 2013-05-06 13:53:57]

Hi,

please do a code review.

[Nikita (slow), 2013-05-06 14:22:24]

lgtm

https://codereview.chromium.org/14990003/diff/1/chrome/browser/ui/webui/about...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/14990003/diff/1/chrome/browser/ui/webui/about...
chrome/browser/ui/webui/about_ui.cc:150: struct Chrome2GoogleLanguageMapping {
nit: Please rename language to locale to match terms that are used in Chrome.

https://codereview.chromium.org/14990003/diff/1/chrome/browser/ui/webui/about...
chrome/browser/ui/webui/about_ui.cc:168: // Returns language in Google format if
it's different from the Chrome one.
nit: Change "Google format" to something like "server side locale"

https://codereview.chromium.org/14990003/diff/1/chrome/browser/ui/webui/about...
chrome/browser/ui/webui/about_ui.cc:174: return
language_mapping[i].google_language;
nit: Add {}

[vasilii, 2013-05-06 15:36:33]

Fixed.

https://codereview.chromium.org/14990003/diff/1/chrome/browser/ui/webui/about...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/14990003/diff/1/chrome/browser/ui/webui/about...
chrome/browser/ui/webui/about_ui.cc:150: struct Chrome2GoogleLanguageMapping {
On 2013/05/06 14:22:25, Nikita Kostylev wrote:
> nit: Please rename language to locale to match terms that are used in Chrome.

Done.

https://codereview.chromium.org/14990003/diff/1/chrome/browser/ui/webui/about...
chrome/browser/ui/webui/about_ui.cc:168: // Returns language in Google format if
it's different from the Chrome one.
On 2013/05/06 14:22:25, Nikita Kostylev wrote:
> nit: Change "Google format" to something like "server side locale"

Done.

https://codereview.chromium.org/14990003/diff/1/chrome/browser/ui/webui/about...
chrome/browser/ui/webui/about_ui.cc:174: return
language_mapping[i].google_language;
On 2013/05/06 14:22:25, Nikita Kostylev wrote:
> nit: Add {}

Done.

[vasilii, 2013-05-06 15:43:16]

Evan, can you review and CQ if you don't find a bug?

[Evan Stade, 2013-05-06 18:06:04]

https://codereview.chromium.org/14990003/diff/9001/chrome/browser/ui/webui/ab...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/14990003/diff/9001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:156: const Chrome2GoogleLocaleMapping
locale_mapping[] = {
When I launch (desktop) Chrome in a foreign locale, URLs like
https://support.google.com/chromeos/?p=simple_scrolling (which don't have any
kind of embedded locale) automatically redirect me to the correctly localized
site. It's handled server-side using the accept-language HTTP header. The same
should be done here.

[Nikita (slow), 2013-05-06 18:10:15]

https://codereview.chromium.org/14990003/diff/9001/chrome/browser/ui/webui/ab...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/14990003/diff/9001/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:156: const Chrome2GoogleLocaleMapping
locale_mapping[] = {
On 2013/05/06 18:06:05, Evan Stade wrote:
> When I launch (desktop) Chrome in a foreign locale, URLs like
> https://support.google.com/chromeos/?p=simple_scrolling (which don't have any
> kind of embedded locale) automatically redirect me to the correctly localized
> site. It's handled server-side using the accept-language HTTP header. The same
> should be done here.

Evan, per my understanding it seems to be that this EULA content is served from
a different  frontend that doesn't support such behavior (static content).

For instance hl=[locale] works for online help center (your example) but doesn't
work here.

Of course we would like to get there and have server side support this the same
way it is supported on online Help center frontend but we need to have some
solution on Chrome side for now (M28).

[Evan Stade, 2013-05-06 18:37:27]

On 2013/05/06 18:10:15, Nikita Kostylev wrote:
>
https://codereview.chromium.org/14990003/diff/9001/chrome/browser/ui/webui/ab...
> File chrome/browser/ui/webui/about_ui.cc (right):
> 
>
https://codereview.chromium.org/14990003/diff/9001/chrome/browser/ui/webui/ab...
> chrome/browser/ui/webui/about_ui.cc:156: const Chrome2GoogleLocaleMapping
> locale_mapping[] = {
> On 2013/05/06 18:06:05, Evan Stade wrote:
> > When I launch (desktop) Chrome in a foreign locale, URLs like
> > https://support.google.com/chromeos/?p=simple_scrolling (which don't have
any
> > kind of embedded locale) automatically redirect me to the correctly
localized
> > site. It's handled server-side using the accept-language HTTP header. The
same
> > should be done here.
> 
> Evan, per my understanding it seems to be that this EULA content is served
from
> a different  frontend that doesn't support such behavior (static content).
> 
> For instance hl=[locale] works for online help center (your example) but
doesn't
> work here.
> 
> Of course we would like to get there and have server side support this the
same
> way it is supported on online Help center frontend but we need to have some
> solution on Chrome side for now (M28).

why is it easier/better to do this on the Chrome side than to have the webmaster
create a page that redirects to the correct locale?

[vasilii, 2013-05-07 17:35:38]

Hi guys,

I got rid of client-server mapping. Please review again.

[Evan Stade, 2013-05-07 21:52:17]

https://codereview.chromium.org/14990003/diff/18001/chrome/browser/ui/webui/a...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/14990003/diff/18001/chrome/browser/ui/webui/a...
chrome/browser/ui/webui/about_ui.cc:158: std::string eula_URL =
base::StringPrintf(chrome::kOnlineEulaURLPath,
I thought the idea was to rely on acceptlanguages

[vasilii, 2013-05-08 07:36:30]

https://codereview.chromium.org/14990003/diff/18001/chrome/browser/ui/webui/a...
File chrome/browser/ui/webui/about_ui.cc (right):

https://codereview.chromium.org/14990003/diff/18001/chrome/browser/ui/webui/a...
chrome/browser/ui/webui/about_ui.cc:158: std::string eula_URL =
base::StringPrintf(chrome::kOnlineEulaURLPath,
On 2013/05/07 21:52:17, Evan Stade wrote:
> I thought the idea was to rely on acceptlanguages

No, they have prepared a list of URL in Chrome format.

[Evan Stade, 2013-05-09 04:46:47]

On 2013/05/08 07:36:30, vasilii wrote:
>
https://codereview.chromium.org/14990003/diff/18001/chrome/browser/ui/webui/a...
> File chrome/browser/ui/webui/about_ui.cc (right):
> 
>
https://codereview.chromium.org/14990003/diff/18001/chrome/browser/ui/webui/a...
> chrome/browser/ui/webui/about_ui.cc:158: std::string eula_URL =
> base::StringPrintf(chrome::kOnlineEulaURLPath,
> On 2013/05/07 21:52:17, Evan Stade wrote:
> > I thought the idea was to rely on acceptlanguages
> 
> No, they have prepared a list of URL in Chrome format.

why not just give us one URL to hit?

[vasilii, 2013-05-10 16:16:13]

On 2013/05/09 04:46:47, Evan Stade wrote:
> On 2013/05/08 07:36:30, vasilii wrote:
> >
>
https://codereview.chromium.org/14990003/diff/18001/chrome/browser/ui/webui/a...
> > File chrome/browser/ui/webui/about_ui.cc (right):
> > 
> >
>
https://codereview.chromium.org/14990003/diff/18001/chrome/browser/ui/webui/a...
> > chrome/browser/ui/webui/about_ui.cc:158: std::string eula_URL =
> > base::StringPrintf(chrome::kOnlineEulaURLPath,
> > On 2013/05/07 21:52:17, Evan Stade wrote:
> > > I thought the idea was to rely on acceptlanguages
> > 
> > No, they have prepared a list of URL in Chrome format.
> 
> why not just give us one URL to hit?

From the answer of webmaster "SCS (static content service) is serving this
document, and since we cannot do the detection/redirection client-side via JS,
we can't do it."

[Evan Stade, 2013-05-10 17:52:54]

I don't know how SCS works or why it's impossible to serve a page that redirects
based on a header. so lgtm I guess.

[commit-bot: I haz the power, 2013-05-13 07:23:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/14990003/18001

[commit-bot: I haz the power, 2013-05-13 09:35:04]

Change committed as 199692