Include the language (locale) info in the update ping

Include the language (locale) info in the update ping
BUG=336614
TBR=thakis@chromium.org (since I "deleted" [read:moved] c/c/omaha/OWNERS, it needs an owner for those changes. cpu@ reviewed, deferred to sorin, who lg'd. But need an owner for CQ. Sorry for the convolution).

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

[Devlin, 2014-02-04 00:18:39]

https://codereview.chromium.org/153273002/diff/1/chrome/browser/extensions/up...
File chrome/browser/extensions/updater/manifest_fetch_data.cc (right):

https://codereview.chromium.org/153273002/diff/1/chrome/browser/extensions/up...
chrome/browser/extensions/updater/manifest_fetch_data.cc:81:
parts.push_back("lang=" + g_browser_process->GetApplicationLocale());
This is also done here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

Would it make sense to put this into OmahaQueryParams at all?

[Devlin, 2014-02-04 00:19:03]

Marijn, mind taking a look when you get a chance?

[Marijn Kruisselbrink, 2014-02-10 21:06:46]

Sorry, for the delay, lgtm

https://codereview.chromium.org/153273002/diff/1/chrome/browser/extensions/up...
File chrome/browser/extensions/updater/manifest_fetch_data.cc (right):

https://codereview.chromium.org/153273002/diff/1/chrome/browser/extensions/up...
chrome/browser/extensions/updater/manifest_fetch_data.cc:81:
parts.push_back("lang=" + g_browser_process->GetApplicationLocale());
On 2014/02/04 00:18:39, D Cronin wrote:
> This is also done here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> 
> Would it make sense to put this into OmahaQueryParams at all?
I'm not entirely sure what you're suggesting putting in OmahaQueryParams? But
yeah, it definitely looks like it might make sense to share something between
this place and WebstoreInstaller.

[Devlin, 2014-02-10 23:08:15]

https://codereview.chromium.org/153273002/diff/1/chrome/browser/extensions/up...
File chrome/browser/extensions/updater/manifest_fetch_data.cc (right):

https://codereview.chromium.org/153273002/diff/1/chrome/browser/extensions/up...
chrome/browser/extensions/updater/manifest_fetch_data.cc:81:
parts.push_back("lang=" + g_browser_process->GetApplicationLocale());
On 2014/02/10 21:06:46, Marijn Kruisselbrink wrote:
> On 2014/02/04 00:18:39, D Cronin wrote:
> > This is also done here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> > 
> > Would it make sense to put this into OmahaQueryParams at all?
> I'm not entirely sure what you're suggesting putting in OmahaQueryParams? But
> yeah, it definitely looks like it might make sense to share something between
> this place and WebstoreInstaller.

The OmahaQueryParams class is where we're getting most of the parameters we're
using, like architecture, os, etc.  It seems like "lang" might be a good fit
there, since it's now used in both places.

+cpu for his thoughts.

[Marijn Kruisselbrink, 2014-02-10 23:35:30]

On 2014/02/10 23:08:15, D Cronin wrote:
>
https://codereview.chromium.org/153273002/diff/1/chrome/browser/extensions/up...
> File chrome/browser/extensions/updater/manifest_fetch_data.cc (right):
> 
>
https://codereview.chromium.org/153273002/diff/1/chrome/browser/extensions/up...
> chrome/browser/extensions/updater/manifest_fetch_data.cc:81:
> parts.push_back("lang=" + g_browser_process->GetApplicationLocale());
> On 2014/02/10 21:06:46, Marijn Kruisselbrink wrote:
> > On 2014/02/04 00:18:39, D Cronin wrote:
> > > This is also done here:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> > > 
> > > Would it make sense to put this into OmahaQueryParams at all?
> > I'm not entirely sure what you're suggesting putting in OmahaQueryParams?
But
> > yeah, it definitely looks like it might make sense to share something
between
> > this place and WebstoreInstaller.
> 
> The OmahaQueryParams class is where we're getting most of the parameters we're
> using, like architecture, os, etc.  It seems like "lang" might be a good fit
> there, since it's now used in both places.
> 
> +cpu for his thoughts.
The difference is that currently all the parameters got from OmahaQueryParams
are actual query params in the query sent to omaha. The lang= part, at least as
it was in your original patch was repeated in each x= query parameter (where the
x= query parameters are themselves url-encoded query parameters). I'm not sure
if Omaha will even handle it properly if there is a toplevel lang= parameter
instead of one nested instead the extension specific update check parameter?

[Devlin, 2014-02-11 19:43:10]

On 2014/02/10 23:35:30, Marijn Kruisselbrink wrote:
> On 2014/02/10 23:08:15, D Cronin wrote:
> >
>
https://codereview.chromium.org/153273002/diff/1/chrome/browser/extensions/up...
> > File chrome/browser/extensions/updater/manifest_fetch_data.cc (right):
> > 
> >
>
https://codereview.chromium.org/153273002/diff/1/chrome/browser/extensions/up...
> > chrome/browser/extensions/updater/manifest_fetch_data.cc:81:
> > parts.push_back("lang=" + g_browser_process->GetApplicationLocale());
> > On 2014/02/10 21:06:46, Marijn Kruisselbrink wrote:
> > > On 2014/02/04 00:18:39, D Cronin wrote:
> > > > This is also done here:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> > > > 
> > > > Would it make sense to put this into OmahaQueryParams at all?
> > > I'm not entirely sure what you're suggesting putting in OmahaQueryParams?
> But
> > > yeah, it definitely looks like it might make sense to share something
> between
> > > this place and WebstoreInstaller.
> > 
> > The OmahaQueryParams class is where we're getting most of the parameters
we're
> > using, like architecture, os, etc.  It seems like "lang" might be a good fit
> > there, since it's now used in both places.
> > 
> > +cpu for his thoughts.
> The difference is that currently all the parameters got from OmahaQueryParams
> are actual query params in the query sent to omaha. The lang= part, at least
as
> it was in your original patch was repeated in each x= query parameter (where
the
> x= query parameters are themselves url-encoded query parameters). I'm not sure
> if Omaha will even handle it properly if there is a toplevel lang= parameter
> instead of one nested instead the extension specific update check parameter?

Ah, good point... From the bug, it actually looks like a top-level parameter
would be preferred, I think (like os, or architecture).

[cpu_(ooo_6.6-7.5), 2014-02-13 03:36:04]

sorry for the delay as well. My thoughts is that sorin should look at this. I
will follow his lead.

[Sorin Jianu, 2014-02-13 19:06:55]

Thank you!

I will change the component updater code to send the same language information,
since the component updater is now using a different version of the update
protocol.

https://codereview.chromium.org/153273002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_installer_unittest.cc (right):

https://codereview.chromium.org/153273002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/webstore_installer_unittest.cc:24:
TEST(WebstoreInstallerTest, PlatformParams) {
Should this test be updated to include the lang query string parameter?

https://codereview.chromium.org/153273002/diff/100001/chrome/browser/omaha_qu...
File chrome/browser/omaha_query_params/omaha_query_params.h (right):

https://codereview.chromium.org/153273002/diff/100001/chrome/browser/omaha_qu...
chrome/browser/omaha_query_params/omaha_query_params.h:23: // prod, prodchannel,
prodversion.
The comment must be updated to include the lang field.

https://codereview.chromium.org/153273002/diff/100001/chrome/browser/omaha_qu...
chrome/browser/omaha_query_params/omaha_query_params.h:24: static std::string
Get(ProdId prod);
I noticed this module does not have unit tests. Would you like to add a unit
test to verify that the arguments listed in the comment are actually sent?

[Devlin, 2014-02-14 00:26:42]

Also fixed getOS, getArch, getNaclArch to be styled (GetOS, GetArch,
GetNaclArch.

And sorry for the seemingly-weird formatting things in some of these files - git
cl format seems to be overly anal regarding whitespace and linewrapping.

https://codereview.chromium.org/153273002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_installer_unittest.cc (right):

https://codereview.chromium.org/153273002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/webstore_installer_unittest.cc:24:
TEST(WebstoreInstallerTest, PlatformParams) {
On 2014/02/13 19:06:55, Sorin Jianu wrote:
> Should this test be updated to include the lang query string parameter?

Done.

https://codereview.chromium.org/153273002/diff/100001/chrome/browser/omaha_qu...
File chrome/browser/omaha_query_params/omaha_query_params.h (right):

https://codereview.chromium.org/153273002/diff/100001/chrome/browser/omaha_qu...
chrome/browser/omaha_query_params/omaha_query_params.h:23: // prod, prodchannel,
prodversion.
On 2014/02/13 19:06:55, Sorin Jianu wrote:
> The comment must be updated to include the lang field.

Done.

https://codereview.chromium.org/153273002/diff/100001/chrome/browser/omaha_qu...
chrome/browser/omaha_query_params/omaha_query_params.h:24: static std::string
Get(ProdId prod);
On 2014/02/13 19:06:55, Sorin Jianu wrote:
> I noticed this module does not have unit tests. Would you like to add a unit
> test to verify that the arguments listed in the comment are actually sent?

Done.

[Sorin Jianu, 2014-02-14 02:54:25]

lgtm

Thank you!

[Devlin, 2014-02-14 17:12:21]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-02-14 17:14:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/153273002/...

[Marijn Kruisselbrink, 2014-02-14 17:19:51]

Just to make sure, somebody actually checked with the omaha server to make sure
it understands lang passed like this?

[Devlin, 2014-02-14 17:27:45]

On 2014/02/14 17:19:51, Marijn Kruisselbrink wrote:
> Just to make sure, somebody actually checked with the omaha server to make
sure
> it understands lang passed like this?

Sorin (Omaha team) - can you confirm?  (I'll be sitting on the abort for the
commit if we don't hear back in time.)

[Sorin Jianu, 2014-02-14 17:32:37]

I am from Omaha team, however, I am on the client side.

At the minimum, we expect the current production server not to break when the
lang parameter is passed, as it is implemented in this change list. Of course, I
also assume that the author of this change list has tested the code so that we
don't break canary or the dev build.

Going forward, once the change lands, Chary (or someone from the CWS team) in
coordination with the Omaha server team must write the correct update
configuration files to serve updates based on the lang parameter and ensure the
lang parameter is correctly logged.

I suggest bouncing the bug back to Chary after the code lands so that she can
take it from there.

[Devlin, 2014-02-14 17:43:46]

On 2014/02/14 17:32:37, Sorin Jianu wrote:
> I am from Omaha team, however, I am on the client side.
> 
> At the minimum, we expect the current production server not to break when the
> lang parameter is passed, as it is implemented in this change list. Of course,
I
> also assume that the author of this change list has tested the code so that we
> don't break canary or the dev build.
> 
> Going forward, once the change lands, Chary (or someone from the CWS team) in
> coordination with the Omaha server team must write the correct update
> configuration files to serve updates based on the lang parameter and ensure
the
> lang parameter is correctly logged.
> 
> I suggest bouncing the bug back to Chary after the code lands so that she can
> take it from there.

Yep, tested it all locally, and sending it through the normal rounds of trybots
to do the best we can to ensure it doesn't break anything.  Just wanted to
confirm that this won't break anything behind the scenes, to the best of our
knowledge.

Thanks for your help on this. :)

[commit-bot: I haz the power, 2014-02-14 20:00:08]

Change committed as 251403