[Sync Promo UI] Changing sync promo to honor brand codes.

[Sync Promo UI] Changing sync promo to honor brand codes.

Also, this CL moves chrome/browser/ui/webui/sync_promo_* to
chrome/browser/ui/webui/sync_promo/sync_promo_*.

BUG=107223
R=stevet@chromium.org,rogerta@chromium.org,sail@chromium.org
TEST=The sync promo responds to brand code and records results of the field
trial correctly.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114608

[Dan Beam, 2011-12-13 06:45:07]

Hey guys, take a look at my first stab at what we talked about this morning.

[SteveT, 2011-12-13 15:19:03]

Hey Dan,

Nice work! Looking pretty good... a couple comments for you (and Roger).


Steve

http://codereview.chromium.org/8933003/diff/1005/chrome/browser/ui/webui/sync...
File chrome/browser/ui/webui/sync_promo_trial.cc (right):

http://codereview.chromium.org/8933003/diff/1005/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo_trial.cc:52: LOG(ERROR) << "brand: " <<
brand;
Don't forget to clean this up at the end of the review. I believe you can also
set the brand code through configuration, for testing purposes. Not sure how it
works on linux, but in Windows there is a reg key that contains the brand value.
Roger - do you remember where this is set in win and perhaps linux?

Some digging through GetBrand may also help.

http://codereview.chromium.org/8933003/diff/1005/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo_trial.cc:158:
SYNC_PROMO_AND_DEFAULT_APPS_BOUNDARY);
A couple questions/points about this UMA histogram...

First:
This gives us a count for the number of users with a given brand code that sign
in using the startup sync promo page. How do we normalize this value across the
total number of browsers with this brand code? I'm guessing we have some
RLZ-related facility for getting the number of installs and first runs for a
given brand code?

In my experiment, I had a second UMA stat (SyncPromo.MessageDisplayed, seen
above), that we used to normalize values. Maybe we don't need something like
that for this new experiment since we have our own counts of installs/first-runs
by brand code.

Second:
If you haven't already, don't forget to the corresponding enum values and
descriptions in src/tools/histograms/histograms.xml, and have jar@ review it.
This has to be checked in before we see changes reported in our histograms dash.

My corresponding patch was here: https://chromereviews.googleplex.com/3739013/

[Roger Tawa OOO till Jul 10th, 2011-12-13 16:00:53]

Looks good.  Since you have not changed any of the default apps code to
enable/disable based on these new brand codes, then you are depending on the
master prefs being set correctly for each brand.  I would normally suggest
adding an assert that but that won't do anything once released in the wild.

Might be worth adding an extra if statement in ShouldInstallInProfile() in 
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/def...
just to be sure, just like is done for the default apps trial.  What do you
think?

http://codereview.chromium.org/8933003/diff/1005/chrome/browser/ui/webui/sync...
File chrome/browser/ui/webui/sync_promo_trial.cc (right):

http://codereview.chromium.org/8933003/diff/1005/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo_trial.cc:52: LOG(ERROR) << "brand: " <<
brand;
On 2011/12/13 15:19:03, SteveT wrote:
> Don't forget to clean this up at the end of the review. I believe you can also
> set the brand code through configuration, for testing purposes. Not sure how
it
> works on linux, but in Windows there is a reg key that contains the brand
value.
> Roger - do you remember where this is set in win and perhaps linux?
> 
> Some digging through GetBrand may also help.

See GetBrand() for details, but for linux, the brand is always empty.

[sail, 2011-12-13 19:38:27]

lgtm

[Dan Beam, 2011-12-14 00:08:50]

On 2011/12/13 16:00:53, Roger Tawa wrote:
> Looks good.  Since you have not changed any of the default apps code to
> enable/disable based on these new brand codes, then you are depending on the
> master prefs being set correctly for each brand.  I would normally suggest
> adding an assert that but that won't do anything once released in the wild.
>
> Might be worth adding an extra if statement in ShouldInstallInProfile() in 
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/def...
> just to be sure, just like is done for the default apps trial.  What do you
> think?

Well, this is kind of still an open issue I've been thinking about.  At the
moment, I'm checking both.  If you either have a master pref from the
installation process we honor it *after* checking to see if your brand code is
part of the experiment.  So basically, if you're in any of the 4 brand codes
we're checking, this overrides, and those installs that happened to have been
tagged with a master pref will also honor it.  Realistically, any browser tagged
with a trial brandcode will also be tagged with a master pref, so it won't
matter (as these should be the exact same set), but I don't assert() that this
is the case (just in case somebody edited their preference file, we want to make
people have the sync promo on by default without messing with their brand
someday, or some other strange reason...?).

Does this sound OK?

>
http://codereview.chromium.org/8933003/diff/1005/chrome/browser/ui/webui/sync...
> File chrome/browser/ui/webui/sync_promo_trial.cc (right):
> 
>
http://codereview.chromium.org/8933003/diff/1005/chrome/browser/ui/webui/sync...
> chrome/browser/ui/webui/sync_promo_trial.cc:52: LOG(ERROR) << "brand: " <<
> brand;
> On 2011/12/13 15:19:03, SteveT wrote:
> > Don't forget to clean this up at the end of the review. I believe you can
also
> > set the brand code through configuration, for testing purposes. Not sure how
> it
> > works on linux, but in Windows there is a reg key that contains the brand
> value.
> > Roger - do you remember where this is set in win and perhaps linux?
> > 
> > Some digging through GetBrand may also help.
> 
> See GetBrand() for details, but for linux, the brand is always empty.

Yes, you see why I had to hardcode one to check the behavior, :P.

[Dan Beam, 2011-12-14 00:16:54]

http://codereview.chromium.org/8933003/diff/1005/chrome/browser/ui/webui/sync...
File chrome/browser/ui/webui/sync_promo_trial.cc (right):

http://codereview.chromium.org/8933003/diff/1005/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo_trial.cc:52: LOG(ERROR) << "brand: " <<
brand;
On 2011/12/13 15:19:03, SteveT wrote:
> Don't forget to clean this up at the end of the review. I believe you can also
> set the brand code through configuration, for testing purposes. Not sure how
it
> works on linux, but in Windows there is a reg key that contains the brand
value.
> Roger - do you remember where this is set in win and perhaps linux?
> 
> Some digging through GetBrand may also help.

Done.

http://codereview.chromium.org/8933003/diff/1005/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo_trial.cc:158:
SYNC_PROMO_AND_DEFAULT_APPS_BOUNDARY);
On 2011/12/13 15:19:03, SteveT wrote:
> A couple questions/points about this UMA histogram...
> 
> First:
> This gives us a count for the number of users with a given brand code that
sign
> in using the startup sync promo page. How do we normalize this value across
the
> total number of browsers with this brand code? I'm guessing we have some
> RLZ-related facility for getting the number of installs and first runs for a
> given brand code?
> 
> In my experiment, I had a second UMA stat (SyncPromo.MessageDisplayed, seen
> above), that we used to normalize values. Maybe we don't need something like
> that for this new experiment since we have our own counts of
installs/first-runs
> by brand code.

Done.

> Second:
> If you haven't already, don't forget to the corresponding enum values and
> descriptions in src/tools/histograms/histograms.xml, and have jar@ review it.
> This has to be checked in before we see changes reported in our histograms
dash.
> 
> My corresponding patch was here: https://chromereviews.googleplex.com/3739013/

Yeah, I've done this before (in fact, I created all the other SyncPromo.*
histograms around your CL, :P).  Thank you for the reminder, though.

[Dan Beam, 2011-12-14 01:22:12]

http://codereview.chromium.org/8933003/diff/1006/chrome/browser/ui/webui/sync...
File chrome/browser/ui/webui/sync_promo_trial.cc (right):

http://codereview.chromium.org/8933003/diff/1006/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo_trial.cc:64: }
By the way, the reason I'm not interlacing these with the default apps
conditionals is that I wanted them to be separate in the case that one
experiment goes longer than the other (so if this experiment happens to continue
into M17 for some reason or after the potential M16 refresh) the code is
decoupled so that this still acts normally if this method gets deleted -
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chrome...

[SteveT, 2011-12-14 01:24:26]

Cool. I do prefer this over hunting for RLZ values.

LGTM

[Dan Beam, 2011-12-14 04:01:53]

Still seeking LG from rogerta.  sail & stevet - modified the CL a tiny bit,
rubber stamp/re-LG/ask questions as you see fit.

[SteveT, 2011-12-14 04:04:53]

Stamp.

[Dan Beam, 2011-12-14 05:14:25]

http://codereview.chromium.org/8933003/diff/7004/chrome/browser/ui/webui/sync...
File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right):

http://codereview.chromium.org/8933003/diff/7004/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:149: return true;
This will be:

  return false;

on M16, btw (http://goo.gl/uBgLB).

[Roger Tawa OOO till Jul 10th, 2011-12-14 12:37:05]

lgtm

If your OK with checking both the brand and the master prefs, then all is
fine.
 On Dec 13, 2011 7:08 PM, <dbeam@chromium.org> wrote:

> On 2011/12/13 16:00:53, Roger Tawa wrote:
>
>> Looks good.  Since you have not changed any of the default apps code to
>> enable/disable based on these new brand codes, then you are depending on
>> the
>> master prefs being set correctly for each brand.  I would normally suggest
>> adding an assert that but that won't do anything once released in the
>> wild.
>>
>
>  Might be worth adding an extra if statement in ShouldInstallInProfile() in
>>
>
> http://src.chromium.org/**viewvc/chrome/trunk/src/**
>
chrome/browser/extensions/**default_apps.cc<http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/default_apps.cc>
>
>> just to be sure, just like is done for the default apps trial.  What do
>> you
>> think?
>>
>
> Well, this is kind of still an open issue I've been thinking about.  At the
> moment, I'm checking both.  If you either have a master pref from the
> installation process we honor it *after* checking to see if your brand
> code is
> part of the experiment.  So basically, if you're in any of the 4 brand
> codes
> we're checking, this overrides, and those installs that happened to have
> been
> tagged with a master pref will also honor it.  Realistically, any browser
> tagged
> with a trial brandcode will also be tagged with a master pref, so it won't
> matter (as these should be the exact same set), but I don't assert() that
> this
> is the case (just in case somebody edited their preference file, we want
> to make
> people have the sync promo on by default without messing with their brand
> someday, or some other strange reason...?).
>
> Does this sound OK?
>
>
> http://codereview.chromium.**org/8933003/diff/1005/chrome/**
>
browser/ui/webui/sync_promo_**trial.cc<http://codereview.chromium.org/8933003/diff/1005/chrome/browser/ui/webui/sync_promo_trial.cc>
>
>> File chrome/browser/ui/webui/sync_**promo_trial.cc (right):
>>
>
>
> http://codereview.chromium.**org/8933003/diff/1005/chrome/**
>
browser/ui/webui/sync_promo_**trial.cc#newcode52<http://codereview.chromium.org/8933003/diff/1005/chrome/browser/ui/webui/sync_promo_trial.cc#newcode52>
>
>> chrome/browser/ui/webui/sync_**promo_trial.cc:52: LOG(ERROR) << "brand:
>> " <<
>> brand;
>> On 2011/12/13 15:19:03, SteveT wrote:
>> > Don't forget to clean this up at the end of the review. I believe you
>> can
>>
> also
>
>> > set the brand code through configuration, for testing purposes. Not
>> sure how
>> it
>> > works on linux, but in Windows there is a reg key that contains the
>> brand
>> value.
>> > Roger - do you remember where this is set in win and perhaps linux?
>> >
>> > Some digging through GetBrand may also help.
>>
>
>  See GetBrand() for details, but for linux, the brand is always empty.
>>
>
> Yes, you see why I had to hardcode one to check the behavior, :P.
>
>
http://codereview.chromium.**org/8933003/<http://codereview.chromium.org/8933...
>

[Dan Beam, 2011-12-14 23:33:55]

+jhawkins for review

[James Hawkins, 2011-12-14 23:59:43]

lgtm

[James Hawkins, 2011-12-15 01:11:26]

SLGTM