add code to propagate finch config TranslateServerStudy server_params to translate element code

add code to propagate finch config TranslateServerStudy server_params to translate element code

BUG=624569
Committed: https://crrev.com/b87bcc4dc960e4bb88da944fc0768abc9268015b
Cr-Commit-Position: refs/heads/master@{#409704}

[ftang, 2016-07-01 18:32:04]

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

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

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

[commit-bot: I haz the power, 2016-07-01 19:07:38]

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

[commit-bot: I haz the power, 2016-07-01 19:07:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ftang, 2016-07-26 20:40:57]

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

[commit-bot: I haz the power, 2016-07-26 20:41:33]

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

[commit-bot: I haz the power, 2016-07-26 21:22:00]

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

[commit-bot: I haz the power, 2016-07-26 21:22:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ftang, 2016-07-27 01:36:24]

fix browser_tests

[ftang, 2016-07-27 01:36:46]

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

[commit-bot: I haz the power, 2016-07-27 01:37:13]

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

[commit-bot: I haz the power, 2016-07-27 02:29:07]

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

[commit-bot: I haz the power, 2016-07-27 02:29:07]

Dry run: This issue passed the CQ dry run.

[ftang, 2016-07-27 03:06:07]

ftang@chromium.org changed reviewers:
+ groby@chromium.org

[ftang, 2016-07-27 03:06:08]

Design doc at go/chrome2twsfe_exp

[ftang, 2016-07-29 20:35:50]

Dear groby:
Please review. Thanks See detail in go/chrome2twsfe_exp

[groby-ooo-7-16, 2016-08-01 23:00:19]

On 2016/07/29 20:35:50, ftang wrote:
> Dear groby:
> Please review. Thanks See detail in go/chrome2twsfe_exp

Note that this allows you a _single_ study that utilizes server params. (I.e.
you will need to partition it into sufficiently small groups if you have
multiple experiments, and you can't use it for rollout if there's more than one
experiment)

[chromium-reviews, 2016-08-01 23:08:20]

Is there other way that we can get param config not tight to a "study"? but
let the config control the value of a variable inside chrome?

On 1 August 2016 at 16:00, <groby@chromium.org> wrote:

> On 2016/07/29 20:35:50, ftang wrote:
> > Dear groby:
> > Please review. Thanks See detail in go/chrome2twsfe_exp
> <https://goto.google.com/chrome2twsfe_exp>
>
> Note that this allows you a _single_ study that utilizes server params.
> (I.e.
> you will need to partition it into sufficiently small groups if you have
> multiple experiments, and you can't use it for rollout if there's more
> than one
> experiment)
>
> https://codereview.chromium.org/2108053004/
>



-- 
Frank Yung-Fong Tang
譚永鋒
Sr. Software Engineer / *Google Ţråñšļåţé*

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[groby-ooo-7-16, 2016-08-02 00:50:07]

On 2016/08/01 23:08:20, chromium-reviews wrote:
> Is there other way that we can get param config not tight to a "study"? but
> let the config control the value of a variable inside chrome?

You can tie it to a feature instead. The caveat there is that a feature can only
be associated with a single experiment across all experiment groups for each
client. They can differ across clients, but you'll still need to carefully
coordinate, so you might as well keep a single study.

(The Omnibox is doing this for a long time - there's a whole list of experiments
within a big one)

[chromium-reviews, 2016-08-02 04:20:52]

so not much advantage to change to that, right?

On 1 August 2016 at 17:50, <groby@chromium.org> wrote:

> On 2016/08/01 23:08:20, chromium-reviews wrote:
> > Is there other way that we can get param config not tight to a "study"?
> but
> > let the config control the value of a variable inside chrome?
>
> You can tie it to a feature instead. The caveat there is that a feature
> can only
> be associated with a single experiment across all experiment groups for
> each
> client. They can differ across clients, but you'll still need to carefully
> coordinate, so you might as well keep a single study.
>
> (The Omnibox is doing this for a long time - there's a whole list of
> experiments
> within a big one)
>
> https://codereview.chromium.org/2108053004/
>



-- 
Frank Yung-Fong Tang
譚永鋒
Sr. Software Engineer / *Google Ţråñšļåţé*

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[groby-ooo-7-16, 2016-08-02 21:14:01]

No - just wanted you to be aware of it. You can certainly leave as-is.

On Mon, Aug 1, 2016 at 9:20 PM, Frank Tang (譚永鋒) <ftang@google.com> wrote:

> so not much advantage to change to that, right?
>
> On 1 August 2016 at 17:50, <groby@chromium.org> wrote:
>
>> On 2016/08/01 23:08:20, chromium-reviews wrote:
>> > Is there other way that we can get param config not tight to a "study"?
>> but
>> > let the config control the value of a variable inside chrome?
>>
>> You can tie it to a feature instead. The caveat there is that a feature
>> can only
>> be associated with a single experiment across all experiment groups for
>> each
>> client. They can differ across clients, but you'll still need to carefully
>> coordinate, so you might as well keep a single study.
>>
>> (The Omnibox is doing this for a long time - there's a whole list of
>> experiments
>> within a big one)
>>
>> https://codereview.chromium.org/2108053004/
>>
>
>
>
> --
> Frank Yung-Fong Tang
> 譚永鋒
> Sr. Software Engineer / *Google Ţråñšļåţé*
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[groby-ooo-7-16, 2016-08-02 21:14:11]

Sorry - I replied, instead of sending the comments.

https://codereview.chromium.org/2108053004/diff/20001/components/translate/co...
File components/translate/core/browser/translate_script.cc (right):

https://codereview.chromium.org/2108053004/diff/20001/components/translate/co...
components/translate/core/browser/translate_script.cc:140: std::string
server_params = "";
No need to initialize - the default ctor automatically creates an empty string.

https://codereview.chromium.org/2108053004/diff/20001/components/translate/co...
components/translate/core/browser/translate_script.cc:147: &data_, "var
serverParams = '%s';\n", server_params.c_str());
I assume this is intentional, and you always want serverParams set, even if the
study didn't have them? (Empty string in that case)

https://codereview.chromium.org/2108053004/diff/20001/components/translate/co...
File components/translate/core/browser/translate_script.h (right):

https://codereview.chromium.org/2108053004/diff/20001/components/translate/co...
components/translate/core/browser/translate_script.h:30: static const char
kTranslateServerStudy[];
Please don't expose in the header - I think we can keep this private, correct?

[ftang, 2016-08-02 22:54:58]

remove header changes and unncessary initialization

[ftang, 2016-08-02 22:55:33]

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

[commit-bot: I haz the power, 2016-08-02 22:56:15]

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

[commit-bot: I haz the power, 2016-08-02 23:45:57]

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

[commit-bot: I haz the power, 2016-08-02 23:45:58]

Dry run: This issue passed the CQ dry run.

[ftang, 2016-08-02 23:46:23]

PTAL

https://codereview.chromium.org/2108053004/diff/20001/components/translate/co...
File components/translate/core/browser/translate_script.cc (right):

https://codereview.chromium.org/2108053004/diff/20001/components/translate/co...
components/translate/core/browser/translate_script.cc:140: std::string
server_params = "";
On 2016/08/02 21:14:10, groby wrote:
> No need to initialize - the default ctor automatically creates an empty
string. 

Done.

https://codereview.chromium.org/2108053004/diff/20001/components/translate/co...
components/translate/core/browser/translate_script.cc:147: &data_, "var
serverParams = '%s';\n", server_params.c_str());
On 2016/08/02 21:14:10, groby wrote:
> I assume this is intentional, and you always want serverParams set, even if
the
> study didn't have them? (Empty string in that case)

YES, I think that will make thing much easier. I can easily check the value in
the js side.

https://codereview.chromium.org/2108053004/diff/20001/components/translate/co...
File components/translate/core/browser/translate_script.h (right):

https://codereview.chromium.org/2108053004/diff/20001/components/translate/co...
components/translate/core/browser/translate_script.h:30: static const char
kTranslateServerStudy[];
On 2016/08/02 21:14:10, groby wrote:
> Please don't expose in the header - I think we can keep this private, correct?

Done.

[groby-ooo-7-16, 2016-08-04 01:31:36]

LGTM unless you need to escape the string

https://codereview.chromium.org/2108053004/diff/40001/components/translate/co...
File components/translate/core/browser/translate_script.cc (right):

https://codereview.chromium.org/2108053004/diff/40001/components/translate/co...
components/translate/core/browser/translate_script.cc:145:
base::StringAppendF(&data_, "var serverParams = '%s';\n",
Last question, and then I promise I let this go. Do we need to escape the data?
(If so, use base::GetQuotedJSONString or the escape function from the same
header)

[ftang, 2016-08-04 01:34:29]

On 2016/08/04 01:31:36, groby wrote:
> LGTM unless you need to escape the string
> 
>
https://codereview.chromium.org/2108053004/diff/40001/components/translate/co...
> File components/translate/core/browser/translate_script.cc (right):
> 
>
https://codereview.chromium.org/2108053004/diff/40001/components/translate/co...
> components/translate/core/browser/translate_script.cc:145:
> base::StringAppendF(&data_, "var serverParams = '%s';\n",
> Last question, and then I promise I let this go. Do we need to escape the
data?
> (If so, use base::GetQuotedJSONString or the escape function from the same
> header)

no, we should not need to.  the value should be within [a-zA-Z0-9,-:_] and no
need to escape them for JS.

[ftang, 2016-08-04 01:34:38]

The CQ bit was checked by ftang@chromium.org

[commit-bot: I haz the power, 2016-08-04 01:35:00]

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

[commit-bot: I haz the power, 2016-08-04 02:16:37]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-04 02:17:56]

Description was changed from

==========
add code to propagate finch config TranslateServerStudy server_params to
translate element code

BUG=624569
==========

to

==========
add code to propagate finch config TranslateServerStudy server_params to
translate element code

BUG=624569

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

[commit-bot: I haz the power, 2016-08-04 02:17:57]

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