Add other variations (with ids) for NTPSnippets to chrome://flags

Add other variations (with ids) for NTPSnippets to chrome://flags

This CL allows to specify variation ids for variations defined for
chrome://flags. The CL also extends the list of variations for the
NTPSnippets feature (some of the variations specify a variation id).

This issue also updates the iOS code to achieve feature parity w.r.t. issues 2036193002 and 2129543002.

BUG=631442
Committed: https://crrev.com/bf07337daa7e3f20fcb1839f300baab6f1df3f35
Cr-Commit-Position: refs/heads/master@{#408599}

[jkrcal, 2016-07-25 16:20:48]

jkrcal@chromium.org changed reviewers:
+ asvitkine@chromium.org, thestig@chromium.org

[jkrcal, 2016-07-25 16:20:48]

Alexei, PTAL at the whole CL
Lei, PTAL at chrome/browser

Chris, Igor: FYI, the new list of variations is in about_flags.cc.

[Lei Zhang, 2016-07-25 17:37:20]

Looks ok, but I mostly defer to Alexei. Is there a BUG= for this?

https://codereview.chromium.org/2182553002/diff/20001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2182553002/diff/20001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:742: if
(command_line->HasSwitch(switches::kForceVariationIds)) {
The command line can have the switch but its value could be an empty string as
well. i.e. users can pass in --foo, when they should be passing in
--foo=foo_val.

[Alexei Svitkine (slow), 2016-07-25 19:15:50]

https://codereview.chromium.org/2182553002/diff/20001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2182553002/diff/20001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2132: std::string 
RegisterAllFeatureVariationParameters(
Nit: Remove extra space.

https://codereview.chromium.org/2182553002/diff/20001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2182553002/diff/20001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:752: }
Instead of all this extra work in chrome_browser_main, I would instead prefer
passing the extra ids to the SetDefaultVariationIds() API and doing the logic
there. Also, I think you can just return the extra ids as a std::vector from
RegisterAllFeatureVariationParameters() instead of converting to intermediate
string form.

[jkrcal, 2016-07-26 09:00:21]

Thanks! PTAL, again.

https://codereview.chromium.org/2182553002/diff/20001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2182553002/diff/20001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2132: std::string 
RegisterAllFeatureVariationParameters(
On 2016/07/25 19:15:50, Alexei Svitkine (slow) wrote:
> Nit: Remove extra space.

Done.

https://codereview.chromium.org/2182553002/diff/20001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2182553002/diff/20001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:742: if
(command_line->HasSwitch(switches::kForceVariationIds)) {
On 2016/07/25 17:37:20, Lei Zhang wrote:
> The command line can have the switch but its value could be an empty string as
> well. i.e. users can pass in --foo, when they should be passing in
> --foo=foo_val.

Thanks for pointing out! Such a case in handled (as an erroneous input) in
VariationsHttpHeaderProvider::SetDefaultVariationIds().

https://codereview.chromium.org/2182553002/diff/20001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:752: }
On 2016/07/25 19:15:50, Alexei Svitkine (slow) wrote:
> Instead of all this extra work in chrome_browser_main, I would instead prefer
> passing the extra ids to the SetDefaultVariationIds() API and doing the logic
> there. Also, I think you can just return the extra ids as a std::vector from
> RegisterAllFeatureVariationParameters() instead of converting to intermediate
> string form.

I tried to simplify it. Do not like the idea of moving even more into
SetDefaultVariationIds() but feel free to insist :)

[jkrcal, 2016-07-26 11:35:12]

Description was changed from

==========
Add other variations (with ids) for NTPSnippets to chrome://flags

This CL allows to specify variation ids for variations defined for
chrome://flags. The CL also extends the list of variations for the
NTPSnippets feature (some of the variations specify a variation id).
==========

to

==========
Add other variations (with ids) for NTPSnippets to chrome://flags

This CL allows to specify variation ids for variations defined for
chrome://flags. The CL also extends the list of variations for the
NTPSnippets feature (some of the variations specify a variation id).

BUG=631442
==========

[Alexei Svitkine (slow), 2016-07-26 14:44:07]

https://codereview.chromium.org/2182553002/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2182553002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:750: }
I still think all this work should be being done in the variations code rather
than in chrome_browser_main.cc.

It's nice to make SetDefaultVariationIds(), but we should avoid bloating
chrome_browser_main.cc with extra variations code.

So, I still suggest putting this logic in the variations code. To keep it
simple, however, you can make a new function that does this parsing and
combining and then calls SetDefaultVariationIds(). This way,
SetDefaultVariationIds() can stay simple and the extra logic will be in the
wrapper function.

[jkrcal, 2016-07-26 15:50:39]

Thanks, Alexei! I also fixed a unittest. PTAL, again.

https://codereview.chromium.org/2182553002/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2182553002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:750: }
On 2016/07/26 14:44:07, Alexei Svitkine (slow) wrote:
> I still think all this work should be being done in the variations code rather
> than in chrome_browser_main.cc.
> 
> It's nice to make SetDefaultVariationIds(), but we should avoid bloating
> chrome_browser_main.cc with extra variations code.
> 
> So, I still suggest putting this logic in the variations code. To keep it
> simple, however, you can make a new function that does this parsing and
> combining and then calls SetDefaultVariationIds(). This way,
> SetDefaultVariationIds() can stay simple and the extra logic will be in the
> wrapper function.

Done. It was not super clear to me where to cut the logic. Please let me know if
even more should be in variations (which would require moving the command-line
flag into variations::flags and/or adding metrics dependency into variations).

[jkrcal, 2016-07-26 15:50:57]

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

[commit-bot: I haz the power, 2016-07-26 15:51:20]

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 15:57:15]

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

[commit-bot: I haz the power, 2016-07-26 15:57:16]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Alexei Svitkine (slow), 2016-07-26 16:43:35]

https://codereview.chromium.org/2182553002/diff/60001/components/variations/v...
File components/variations/variations_associated_data.cc (right):

https://codereview.chromium.org/2182553002/diff/60001/components/variations/v...
components/variations/variations_associated_data.cc:173: std::string
command_line_variation_ids) {
Pass the string by const& and the |variations_ids| by pointer, to avoid the
extra copy. (i.e. just modify the vector that's on the stack of the caller.
Switch the param order so the pointer is last.

https://codereview.chromium.org/2182553002/diff/60001/components/variations/v...
components/variations/variations_associated_data.cc:175: // Combine the
variation ids from chrome://flags with the ones from the
Seems strange to mention chrome://flags here, since that's not obvious in this
context. I would just mention the variable names instead.

https://codereview.chromium.org/2182553002/diff/60001/components/variations/v...
components/variations/variations_associated_data.cc:193: return nullptr;
Seems like the previous code actually had a bug, since it was only calling
AddSyntheticTrialObserver() when the command-line was specified, but that's
actually not the intention (it should always be called). Can you fix this while
you're here?

I actually suggest instead of making this function return the provider, this can
be a member function of the provider and the calling code could just call it on
the provider instance.

[jkrcal, 2016-07-26 16:54:42]

Thanks, Alexei. I'll get to it tomorrow, again.

Slightly unrelated: 
the build fails for iOS in IOSChromeMainParts::SetUpMetricsAndFieldTrials(). I
was never aware of this being treated differently on iOS, thus
about_flags::RegisterAllFeatureVariationParameters() is not called from that
function. 

Should I add the call? Do chrome://flags work the same on iOS as on android?

[Alexei Svitkine (slow), 2016-07-26 18:07:58]

Ah yes, unfortunately that file is forked on iOS. I suggest syncing them up
to the same code.

On Tue, Jul 26, 2016 at 12:54 PM, <jkrcal@chromium.org> wrote:

> Thanks, Alexei. I'll get to it tomorrow, again.
>
> Slightly unrelated:
> the build fails for iOS in
> IOSChromeMainParts::SetUpMetricsAndFieldTrials(). I
> was never aware of this being treated differently on iOS, thus
> about_flags::RegisterAllFeatureVariationParameters() is not called from
> that
> function.
>
> Should I add the call? Do chrome://flags work the same on iOS as on
> android?
>
> https://codereview.chromium.org/2182553002/
>

-- 
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.

[jkrcal, 2016-07-27 08:24:42]

Description was changed from

==========
Add other variations (with ids) for NTPSnippets to chrome://flags

This CL allows to specify variation ids for variations defined for
chrome://flags. The CL also extends the list of variations for the
NTPSnippets feature (some of the variations specify a variation id).

BUG=631442
==========

to

==========
Add other variations (with ids) for NTPSnippets to chrome://flags

This CL allows to specify variation ids for variations defined for
chrome://flags. The CL also extends the list of variations for the
NTPSnippets feature (some of the variations specify a variation id).

This issue also updates the iOS code to achieve feature parity w.r.t. issues
2036193002 and 2129543002.

BUG=631442
==========

[jkrcal, 2016-07-27 10:06:27]

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

[commit-bot: I haz the power, 2016-07-27 10:06:53]

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

[jkrcal, 2016-07-27 10:09:03]

jkrcal@chromium.org changed reviewers:
+ sdefresne@chromium.org

[jkrcal, 2016-07-27 10:09:04]

Thanks, Alexei! PTAL, again.

Sylvain, could you PTAL on the iOS code?

https://codereview.chromium.org/2182553002/diff/60001/components/variations/v...
File components/variations/variations_associated_data.cc (right):

https://codereview.chromium.org/2182553002/diff/60001/components/variations/v...
components/variations/variations_associated_data.cc:173: std::string
command_line_variation_ids) {
On 2016/07/26 16:43:35, Alexei Svitkine (slow) wrote:
> Pass the string by const& and the |variations_ids| by pointer, to avoid the
> extra copy. (i.e. just modify the vector that's on the stack of the caller.
> Switch the param order so the pointer is last.

Huh, a silly mistake. Done.

https://codereview.chromium.org/2182553002/diff/60001/components/variations/v...
components/variations/variations_associated_data.cc:175: // Combine the
variation ids from chrome://flags with the ones from the
On 2016/07/26 16:43:35, Alexei Svitkine (slow) wrote:
> Seems strange to mention chrome://flags here, since that's not obvious in this
> context. I would just mention the variable names instead.

Done. (Actually, had a bug there, wanted to check non-emptiness of
|command_line_variation_ids|).

https://codereview.chromium.org/2182553002/diff/60001/components/variations/v...
components/variations/variations_associated_data.cc:193: return nullptr;
On 2016/07/26 16:43:35, Alexei Svitkine (slow) wrote:
> Seems like the previous code actually had a bug, since it was only calling
> AddSyntheticTrialObserver() when the command-line was specified, but that's
> actually not the intention (it should always be called). Can you fix this
while
> you're here?
> 
> I actually suggest instead of making this function return the provider, this
can
> be a member function of the provider and the calling code could just call it
on
> the provider instance.

Done.

[commit-bot: I haz the power, 2016-07-27 10:20:34]

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

[commit-bot: I haz the power, 2016-07-27 10:20:35]

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

[jkrcal, 2016-07-27 10:33:29]

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

[commit-bot: I haz the power, 2016-07-27 10:33:52]

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 12:02:22]

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

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

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sdefresne, 2016-07-27 12:06:47]

ios/ lgtm

[Alexei Svitkine (slow), 2016-07-27 15:04:18]

https://codereview.chromium.org/2182553002/diff/160001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2182553002/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:743: // X-Client-Data request header.
This comment was actually about the command line flag logic, so it doesn't make
sense anymore now that this is not in the if.

I think you can just delete it since the comment on line 746 covers this. Do the
same for the iOS file.

https://codereview.chromium.org/2182553002/diff/160001/components/flags_ui/fe...
File components/flags_ui/feature_entry.h (right):

https://codereview.chromium.org/2182553002/diff/160001/components/flags_ui/fe...
components/flags_ui/feature_entry.h:104: // if you do not need to set any
variation_id for this feature variation.
Why empty string and not nullptr?

https://codereview.chromium.org/2182553002/diff/160001/components/variations/...
File components/variations/variations_http_header_provider.h (right):

https://codereview.chromium.org/2182553002/diff/160001/components/variations/...
components/variations/variations_http_header_provider.h:45: // list of
|command_line_variation_ids| passed in a command line flag). This
Can you make this a little bit more explicit about it modifying
|variations_ids|.

e.g. "possible extended" -> "which will be extended by the comma-separated
|command_line_variation_ids|"

https://codereview.chromium.org/2182553002/diff/160001/components/variations/...
components/variations/variations_http_header_provider.h:48:
std::vector<std::string>* variation_ids,
Nit: Non-const params should be last.

[jkrcal, 2016-07-27 17:10:43]

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

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

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

[jkrcal, 2016-07-27 17:12:44]

Thanks! PTAL, again.

(There is still a weird bug in the unittest of variations_http_header_provider
on windows. Need to look closer into it tomorrow.)

https://codereview.chromium.org/2182553002/diff/160001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2182553002/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:743: // X-Client-Data request header.
On 2016/07/27 15:04:18, Alexei Svitkine (slow) wrote:
> This comment was actually about the command line flag logic, so it doesn't
make
> sense anymore now that this is not in the if.
> 
> I think you can just delete it since the comment on line 746 covers this. Do
the
> same for the iOS file.

Done.

https://codereview.chromium.org/2182553002/diff/160001/components/flags_ui/fe...
File components/flags_ui/feature_entry.h (right):

https://codereview.chromium.org/2182553002/diff/160001/components/flags_ui/fe...
components/flags_ui/feature_entry.h:104: // if you do not need to set any
variation_id for this feature variation.
On 2016/07/27 15:04:18, Alexei Svitkine (slow) wrote:
> Why empty string and not nullptr?

Done.
(It was std::string originally, I did not notice the discrepancy when I fixed it
to char*)

https://codereview.chromium.org/2182553002/diff/160001/components/variations/...
File components/variations/variations_http_header_provider.h (right):

https://codereview.chromium.org/2182553002/diff/160001/components/variations/...
components/variations/variations_http_header_provider.h:45: // list of
|command_line_variation_ids| passed in a command line flag). This
On 2016/07/27 15:04:18, Alexei Svitkine (slow) wrote:
> Can you make this a little bit more explicit about it modifying
> |variations_ids|.
> 
> e.g. "possible extended" -> "which will be extended by the comma-separated
> |command_line_variation_ids|"

Done.

https://codereview.chromium.org/2182553002/diff/160001/components/variations/...
components/variations/variations_http_header_provider.h:48:
std::vector<std::string>* variation_ids,
On 2016/07/27 15:04:18, Alexei Svitkine (slow) wrote:
> Nit: Non-const params should be last.

Done.

[Alexei Svitkine (slow), 2016-07-27 17:31:27]

lgtm

https://codereview.chromium.org/2182553002/diff/180001/components/variations/...
File components/variations/variations_http_header_provider.cc (right):

https://codereview.chromium.org/2182553002/diff/180001/components/variations/...
components/variations/variations_http_header_provider.cc:68:
variation_ids_flags.end());
Nit: Align

[commit-bot: I haz the power, 2016-07-27 17:51:40]

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

[commit-bot: I haz the power, 2016-07-27 17:51:41]

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

[jkrcal, 2016-07-28 17:12:51]

Thanks, Alexei!

Btw. there was quite an intriguing bug that only appeared in unit-tests on a
part of Windows bots. Creating a StringPiece from a temporary std::string
resulting in reading the string from invalid memory.

Lei, I still need you lgtm.

[jkrcal, 2016-07-28 17:12:58]

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

[commit-bot: I haz the power, 2016-07-28 17:13:24]

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-28 17:57:10]

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

[commit-bot: I haz the power, 2016-07-28 17:57:11]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2016-07-28 18:07:26]

On 2016/07/28 17:12:51, jkrcal wrote:
> Thanks, Alexei!
> 
> Btw. there was quite an intriguing bug that only appeared in unit-tests on a
> part of Windows bots. Creating a StringPiece from a temporary std::string
> resulting in reading the string from invalid memory.
> 
> Lei, I still need you lgtm.

Yay for the bugfix!

[Lei Zhang, 2016-07-28 18:33:07]

lgtm

[jkrcal, 2016-07-29 07:16:48]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2016-07-29 07:16:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2182553002/#ps210001
(title: "Alexei's comment #5 + fix a bug")

[commit-bot: I haz the power, 2016-07-29 07:17:05]

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-29 07:21:46]

Description was changed from

==========
Add other variations (with ids) for NTPSnippets to chrome://flags

This CL allows to specify variation ids for variations defined for
chrome://flags. The CL also extends the list of variations for the
NTPSnippets feature (some of the variations specify a variation id).

This issue also updates the iOS code to achieve feature parity w.r.t. issues
2036193002 and 2129543002.

BUG=631442
==========

to

==========
Add other variations (with ids) for NTPSnippets to chrome://flags

This CL allows to specify variation ids for variations defined for
chrome://flags. The CL also extends the list of variations for the
NTPSnippets feature (some of the variations specify a variation id).

This issue also updates the iOS code to achieve feature parity w.r.t. issues
2036193002 and 2129543002.

BUG=631442
==========

[commit-bot: I haz the power, 2016-07-29 07:21:47]

Committed patchset #12 (id:210001)

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

Description was changed from

==========
Add other variations (with ids) for NTPSnippets to chrome://flags

This CL allows to specify variation ids for variations defined for
chrome://flags. The CL also extends the list of variations for the
NTPSnippets feature (some of the variations specify a variation id).

This issue also updates the iOS code to achieve feature parity w.r.t. issues
2036193002 and 2129543002.

BUG=631442
==========

to

==========
Add other variations (with ids) for NTPSnippets to chrome://flags

This CL allows to specify variation ids for variations defined for
chrome://flags. The CL also extends the list of variations for the
NTPSnippets feature (some of the variations specify a variation id).

This issue also updates the iOS code to achieve feature parity w.r.t. issues
2036193002 and 2129543002.

BUG=631442

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

[commit-bot: I haz the power, 2016-07-29 07:24:04]

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