Enable brand codes and master preferences on the Mac.

Enable brand codes and master preferences on the Mac.

BUG=93421
TEST=alter brand codes and master prefs files; should be read

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

[Avi (use Gerrit), 2011-08-19 00:18:43]

Not quite ready to be committed. Actually, totally untested (though it builds).
This is where I'm going with it. Thoughts?

[Miranda Callahan, 2011-08-19 09:03:24]

First run stuff looks right (with comment); I'll let Mark comment on the
Keystone stuff.

http://codereview.chromium.org/7655056/diff/3001/chrome/browser/first_run/fir...
File chrome/browser/first_run/first_run_mac.mm (right):

http://codereview.chromium.org/7655056/diff/3001/chrome/browser/first_run/fir...
chrome/browser/first_run/first_run_mac.mm:20: StartsWithASCII(brand, "EU",
true);
Unless we go to a model where we're actually getting brand codes through
downloads, shouldn't we be ignoring all brand codes except those we know are
inorganic? (or maybe just add a clause returning true in the case of no brand
code?)

[Miranda Callahan, 2011-08-19 10:03:43]

On 2011/08/19 09:03:24, Miranda Callahan wrote:
> First run stuff looks right (with comment); I'll let Mark comment on the
> Keystone stuff.
> 
>
http://codereview.chromium.org/7655056/diff/3001/chrome/browser/first_run/fir...
> File chrome/browser/first_run/first_run_mac.mm (right):
> 
>
http://codereview.chromium.org/7655056/diff/3001/chrome/browser/first_run/fir...
> chrome/browser/first_run/first_run_mac.mm:20: StartsWithASCII(brand, "EU",
> true);
> Unless we go to a model where we're actually getting brand codes through
> downloads, shouldn't we be ignoring all brand codes except those we know are
> inorganic? (or maybe just add a clause returning true in the case of no brand
> code?)

Hmm -- I'm beginning to think I shouldn't go to meetings and try to say
reasonable things at 11 pm.

I was conflating two separate things last night when I talked about
master_preferences and brand coding; let me see if I can sort this out.

The master_preferences file does indeed allow us to steer things for first_run,
and can be used to force the browser to have a certain default search engine,
and to not show the search ballot. However, I misspoke about the brand code --
this is *not* part of the master preferences file; it's read in a different part
of the install process on Windows, and saved to the Registry.  

Specifically, the brand is written to the registry in
chrome/installer/setup/install_worker.cc, as part of the
AddGoogleUpdateWorkItems function. That function gets the brand as part of an
InstallationState variable passed in, |original_state| -- which is passed
through AddInstallWorkItems from install.cc, and set up all the way back in
wWinMain as part of the first steps in setup_main.cc (this is also where
master_preferences is set up, btw). This is where my knowledge ends -- it looks
to me like ddorwin is somebody that would know how the brand code is set through
Omaha. And already we're in the realm of "much more complicated than things
seemed at first".

If we can avoid the brand code on Mac, and instead pass the default search
engine preference through a master preferences file, it would certainly make
things easier for the client code, and this patch would be great; I'm not sure
how well this scales, but if we are controlling the package to be released
through DivX, it seems like there would be no problem with just packing along a
master_preferences file for the ride...?

Summary: there are two distinct ways to control the first run:
1. Explicitly and in detail through the master preferences file, through which
you can set default search engine, home page, etc. (see master_preferences.h for
a sample).
2. Implicitly by passing an "organic" brand code, which seems to be written to
the registry through Omaha magic at install time. When we see this brand code
during first run, we trigger a set of first run preferences (see
go/chromefirstrun for a table showing exactly what is set).

[Miranda Callahan, 2011-08-19 10:14:10]

On 2011/08/19 10:03:43, Miranda Callahan wrote:
> On 2011/08/19 09:03:24, Miranda Callahan wrote:
> > First run stuff looks right (with comment); I'll let Mark comment on the
> > Keystone stuff.
> > 
> >
>
http://codereview.chromium.org/7655056/diff/3001/chrome/browser/first_run/fir...
> > File chrome/browser/first_run/first_run_mac.mm (right):
> > 
> >
>
http://codereview.chromium.org/7655056/diff/3001/chrome/browser/first_run/fir...
> > chrome/browser/first_run/first_run_mac.mm:20: StartsWithASCII(brand, "EU",
> > true);
> > Unless we go to a model where we're actually getting brand codes through
> > downloads, shouldn't we be ignoring all brand codes except those we know are
> > inorganic? (or maybe just add a clause returning true in the case of no
brand
> > code?)
> 
> Hmm -- I'm beginning to think I shouldn't go to meetings and try to say
> reasonable things at 11 pm.
> 
> I was conflating two separate things last night when I talked about
> master_preferences and brand coding; let me see if I can sort this out.
> 
> The master_preferences file does indeed allow us to steer things for
first_run,
> and can be used to force the browser to have a certain default search engine,
> and to not show the search ballot. However, I misspoke about the brand code --
> this is *not* part of the master preferences file; it's read in a different
part
> of the install process on Windows, and saved to the Registry.  
> 
> Specifically, the brand is written to the registry in
> chrome/installer/setup/install_worker.cc, as part of the
> AddGoogleUpdateWorkItems function. That function gets the brand as part of an
> InstallationState variable passed in, |original_state| -- which is passed
> through AddInstallWorkItems from install.cc, and set up all the way back in
> wWinMain as part of the first steps in setup_main.cc (this is also where
> master_preferences is set up, btw). This is where my knowledge ends -- it
looks
> to me like ddorwin is somebody that would know how the brand code is set
through
> Omaha. And already we're in the realm of "much more complicated than things
> seemed at first".
> 
> If we can avoid the brand code on Mac, and instead pass the default search
> engine preference through a master preferences file, it would certainly make
> things easier for the client code, and this patch would be great; I'm not sure
> how well this scales, but if we are controlling the package to be released
> through DivX, it seems like there would be no problem with just packing along
a
> master_preferences file for the ride...?
> 
> Summary: there are two distinct ways to control the first run:
> 1. Explicitly and in detail through the master preferences file, through which
> you can set default search engine, home page, etc. (see master_preferences.h
for
> a sample).
> 2. Implicitly by passing an "organic" brand code, which seems to be written to
> the registry through Omaha magic at install time. When we see this brand code
> during first run, we trigger a set of first run preferences (see
> go/chromefirstrun for a table showing exactly what is set).

Another specific thought on first run stuff: I think we're going to need master
preferences reading in Mac no matter how we handle the brand code stuff, because
of the fine-tuning it allows. I think that every Windows build that's part of a
non-organic build has a master preferences file to tune first run variables (see
chart from my previous comment).

[Avi (use Gerrit), 2011-08-19 12:18:48]

> However, I misspoke about the brand code --
> this is *not* part of the master preferences file; it's read in a different
part
> of the install process on Windows, and saved to the Registry.  

Yes, I saw that. The Mac already has brand code infrastructure, and I'm adding
Master Prefs support in a parallel way, but different from, the brand code.

> If we can avoid the brand code on Mac

It's already too late for that. In our build process we build four versions of
Chrome, one with each brand code baked into the info.plist, and those are the
versions we distribute through the different channels. On first startup, Chrome
looks to see if there is a brand code, and if so it squirrels it away in a safe
location, and passes it to the Keystone updating system.

Right now I'm at home and can't point you to the part where we brand the disk
images, but the mechanism for handling it is
src/chrome/browser/mac/keystone_glue.h|mm, which are touched in this very CL.

> 1. Explicitly and in detail through the master preferences file
> 2. Implicitly by passing an "organic" brand code

We're definitely going to need the second; we're likely going to need the first.

> Another specific thought on first run stuff: I think we're going to need
master
> preferences reading in Mac no matter how we handle the brand code stuff,
because
> of the fine-tuning it allows. I think that every Windows build that's part of
a
> non-organic build has a master preferences file to tune first run variables
(see
> chart from my previous comment).

Yep. Thanks for the reference; I'll look at the chart.

http://codereview.chromium.org/7655056/diff/3001/chrome/browser/first_run/fir...
File chrome/browser/first_run/first_run_mac.mm (right):

http://codereview.chromium.org/7655056/diff/3001/chrome/browser/first_run/fir...
chrome/browser/first_run/first_run_mac.mm:20: StartsWithASCII(brand, "EU",
true);
The Mac already _has_ brand codes. Four of 'em, actually. This logic is swiped
from the Windows version. I looked at the four brand codes and their meaning,
and this prescription of GG/EU vs everything else actually really makes sense.
Still, I'd like to discuss it with the people in charge of brand coding. Do you
remember who you talked to the last time about this?

[Miranda Callahan, 2011-08-19 12:23:21]

On 2011/08/19 12:18:48, Avi wrote:
> > However, I misspoke about the brand code --
> > this is *not* part of the master preferences file; it's read in a different
> part
> > of the install process on Windows, and saved to the Registry.  
> 
> Yes, I saw that. The Mac already has brand code infrastructure, and I'm adding
> Master Prefs support in a parallel way, but different from, the brand code.
> 
> > If we can avoid the brand code on Mac
> 
> It's already too late for that. In our build process we build four versions of
> Chrome, one with each brand code baked into the info.plist, and those are the
> versions we distribute through the different channels. On first startup,
Chrome
> looks to see if there is a brand code, and if so it squirrels it away in a
safe
> location, and passes it to the Keystone updating system.
> 
> Right now I'm at home and can't point you to the part where we brand the disk
> images, but the mechanism for handling it is
> src/chrome/browser/mac/keystone_glue.h|mm, which are touched in this very CL.
> 
> > 1. Explicitly and in detail through the master preferences file
> > 2. Implicitly by passing an "organic" brand code
> 
> We're definitely going to need the second; we're likely going to need the
first.
> 
> > Another specific thought on first run stuff: I think we're going to need
> master
> > preferences reading in Mac no matter how we handle the brand code stuff,
> because
> > of the fine-tuning it allows. I think that every Windows build that's part
of
> a
> > non-organic build has a master preferences file to tune first run variables
> (see
> > chart from my previous comment).
> 
> Yep. Thanks for the reference; I'll look at the chart.
> 
>
http://codereview.chromium.org/7655056/diff/3001/chrome/browser/first_run/fir...
> File chrome/browser/first_run/first_run_mac.mm (right):
> 
>
http://codereview.chromium.org/7655056/diff/3001/chrome/browser/first_run/fir...
> chrome/browser/first_run/first_run_mac.mm:20: StartsWithASCII(brand, "EU",
> true);
> The Mac already _has_ brand codes. Four of 'em, actually. This logic is swiped
> from the Windows version. I looked at the four brand codes and their meaning,
> and this prescription of GG/EU vs everything else actually really makes sense.
> Still, I'd like to discuss it with the people in charge of brand coding. Do
you
> remember who you talked to the last time about this?

Oh, I'm very glad that Mac has the brand codes already baked in -- I was afraid
of a giant infrastructural mess trying to get this implemented.  *whew*

For brand codes and their meaning, Min Li was my contact last time; she'll
certainly know who else to contact if she's not on this any more...

[Mark Mentovai, 2011-08-19 12:59:44]

Why do we need to check the brand code to avoid first run? Why can’t
we just check master_preferences.

We don’t need to mimic Windows on this, the implementations are
already different enough. Since this deal that we’re doing will come
with a master_preferences file, why not just do the easy thing and
have master_preferences dictate “no first run, use these settings?” as
opposed to adding a second redundant path to avoid first-run UI?

[Avi (use Gerrit), 2011-08-19 14:12:59]

On 2011/08/19 12:59:44, Mark Mentovai wrote:
> Why do we need to check the brand code to avoid first run? Why can’t
> we just check master_preferences.
> 
> We don’t need to mimic Windows on this, the implementations are
> already different enough.

Mostly because the processes are in place, and I'd rather have two different
implementations that do the same thing than two different implementations that
do two different things.

[Avi (use Gerrit), 2011-08-22 20:34:14]

OK, good with it. For your review.

[Mark Mentovai, 2011-08-22 20:50:08]

Considering the zombies I’m fighting, I probably won’t be able to
review this today. Maybe Miranda can make a first pass before I get a
chance.

[Miranda Callahan, 2011-08-23 07:14:41]

On 2011/08/22 20:50:08, Mark Mentovai wrote:
> Considering the zombies I’m fighting, I probably won’t be able to
> review this today. Maybe Miranda can make a first pass before I get a
> chance.

LGTM for the first run stuff, and thank you for the intelligent refactoring of
the code to get the master prefs path.

[Avi (use Gerrit), 2011-08-23 15:27:07]

Mark, you OK with the keystone glue changes?

[Mark Mentovai, 2011-08-23 16:25:12]

TVL did lots of brand code stuff. Maybe he has time for a more thorough review.
I have 69k M15 P1s. I only looked at the keystone* files.

http://codereview.chromium.org/7655056/diff/11001/chrome/browser/mac/keystone...
File chrome/browser/mac/keystone_glue.h (right):

http://codereview.chromium.org/7655056/diff/11001/chrome/browser/mac/keystone...
chrome/browser/mac/keystone_glue.h:203: FilePath MasterPrefsPath();
I’m unconvinced that this belongs as part of the keystone glue interface.

http://codereview.chromium.org/7655056/diff/11001/chrome/browser/mac/keystone...
chrome/browser/mac/keystone_glue.h:206: std::string BrandCode();
As opposed to this, which does.

http://codereview.chromium.org/7655056/diff/11001/chrome/browser/mac/keystone...
File chrome/browser/mac/keystone_glue.mm (right):

http://codereview.chromium.org/7655056/diff/11001/chrome/browser/mac/keystone...
chrome/browser/mac/keystone_glue.mm:932: return
FilePath([[kMasterPreferencesFile stringByStandardizingPath]
stringByStandardizingPath doesn’t gain anything here, since we know the string
is pretty standard already.

stringByStandardizingPath can talk to the filesystem and perform blocking
operations.

http://codereview.chromium.org/7655056/diff/11001/chrome/browser/mac/keystone...
chrome/browser/mac/keystone_glue.mm:937: KeystoneGlue* keystoneGlue =
[KeystoneGlue defaultKeystoneGlue];
Name things like keystone_glue. You‘re not in an @objc thing here.

http://codereview.chromium.org/7655056/diff/11001/chrome/browser/mac/keystone...
chrome/browser/mac/keystone_glue.mm:943: NSString* brand_code =
On which thread will this be called? Remember, we want to be careful about
things that talk to the filesystem.

Maybe a better question isn’t actually about the thread that this runs on, but
when it runs. First-run-only on the main thread ought to be tolerable. “Main
thread during each browser startup” may not be.

[TVL, 2011-08-23 16:51:31]

Should canary share the master prefs or use a different one?

http://codereview.chromium.org/7655056/diff/11001/chrome/browser/mac/keystone...
File chrome/browser/mac/keystone_glue.h (right):

http://codereview.chromium.org/7655056/diff/11001/chrome/browser/mac/keystone...
chrome/browser/mac/keystone_glue.h:203: FilePath MasterPrefsPath();
Second, this really doesn't seem like it should go in keystone_glue.

http://codereview.chromium.org/7655056/diff/11001/chrome/browser/mac/keystone...
File chrome/browser/mac/keystone_glue.mm (right):

http://codereview.chromium.org/7655056/diff/11001/chrome/browser/mac/keystone...
chrome/browser/mac/keystone_glue.mm:950: return
base::SysNSStringToUTF8(brand_code);
We're also paying a lot in conversions  ASC -> NSString -> UTF8.
file_util::ReadFileToString?

[Mark Mentovai, 2011-08-23 16:57:30]

thomasvl@chromium.org wrote:
> Should canary share the master prefs or use a different one?

Does Windows Chrome set any precedent?

[Avi (use Gerrit), 2011-08-23 17:03:47]

I put the master prefs file code in keystone glue because to me it seems related
to the brand code. Windows code puts the master prefs file as a sibling file to
the executable, which solves the problem with the canary.

[Avi (use Gerrit), 2011-08-23 17:04:52]

On 2011/08/23 17:03:47, Avi wrote:
> I put the master prefs file code in keystone glue because to me it seems
related
> to the brand code. Windows code puts the master prefs file as a sibling file
to
> the executable, which solves the problem with the canary.

Plus master prefs files are used for distribution deals. No one is distributing
the canary.

[Mark Mentovai, 2011-08-24 01:10:49]

avi@chromium.org wrote:
> I put the master prefs file code in keystone glue because to me it seems
related
> to the brand code.

It’s not. It doesn’t belong in keystone_glue.mm, which is already big
enough as it is without adding unrelated stuff. It’s fine to add a
small new file for Mac-specific master prefs utilities.

> Windows code puts the master prefs file as a sibling file to
> the executable, which solves the problem with the canary.

Meaning that the canary has a distinct master preferences file, then?
We should follow suit.

> Plus master prefs files are used for distribution deals. No one is
distributing

This thought may have been incompl

[Avi (use Gerrit), 2011-08-24 04:54:14]

On Tue, Aug 23, 2011 at 9:10 PM, Mark Mentovai <mark@chromium.org> wrote:

> > Plus master prefs files are used for distribution deals. No one is
> distributing
>
> This thought may have been incompl
>

Flip open the flippy triangle to reveal the rest of that email. Gmail makes
a fool of us all.

Avi

[Avi (use Gerrit), 2011-08-24 20:38:36]

http://codereview.chromium.org/7655056/diff/11001/chrome/browser/mac/keystone...
File chrome/browser/mac/keystone_glue.mm (right):

http://codereview.chromium.org/7655056/diff/11001/chrome/browser/mac/keystone...
chrome/browser/mac/keystone_glue.mm:943: NSString* brand_code =
This is called only on the first run, on the main thread.

[Avi (use Gerrit), 2011-08-24 21:19:28]

On Tue, Aug 23, 2011 at 9:10 PM, Mark Mentovai <mark@chromium.org> wrote:

> Meaning that the canary has a distinct master preferences file, then?
> We should follow suit.


The canary has a distinct file because of the accident of how it's
implemented. You already agreed that master prefs files should be
system-only, not local. I don't see it as reasonable to handle the case
where we have a distribution deal for a system-level canary.

Avi

[Mark Mentovai, 2011-08-24 21:37:31]

http://codereview.chromium.org/7655056/diff/11001/chrome/browser/mac/keystone...
File chrome/browser/mac/keystone_glue.mm (right):

http://codereview.chromium.org/7655056/diff/11001/chrome/browser/mac/keystone...
chrome/browser/mac/keystone_glue.mm:45: #define kMasterPreferencesFileName
@"Google Chrome Master Preferences";
You equipped this for a separate master preferences file for Chromium and Google
Chrome. Once you do that, you need to treat the canary separately as well.

[Avi (use Gerrit), 2011-08-24 21:47:46]

ptal

[Mark Mentovai, 2011-08-24 21:57:19]

http://codereview.chromium.org/7655056/diff/24003/chrome/browser/mac/master_p...
File chrome/browser/mac/master_prefs.mm (right):

http://codereview.chromium.org/7655056/diff/24003/chrome/browser/mac/master_p...
chrome/browser/mac/master_prefs.mm:12: const char kMasterPreferencesFileName[] =
"Google Chrome Master Preferences";
This is not a solution.

Here’s my concern: download a distribution deal package, install it, get Chrome.
Download Chromium or the Canary, launch it. Master preferences applied?
Inappropriate.

[Avi (use Gerrit), 2011-08-24 22:10:50]

Done.

[Mark Mentovai, 2011-08-25 13:35:08]

http://codereview.chromium.org/7655056/diff/29001/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run_mac.mm (right):

http://codereview.chromium.org/7655056/diff/29001/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run_mac.mm:20: return StartsWithASCII(brand,
"GG", true) ||
Caution! No brand code at all is also organic.

http://codereview.chromium.org/7655056/diff/29001/chrome/browser/mac/keystone...
File chrome/browser/mac/keystone_glue.h (right):

http://codereview.chromium.org/7655056/diff/29001/chrome/browser/mac/keystone...
chrome/browser/mac/keystone_glue.h:201: // Returns the brand code of the
installation.
Specify in the comment that this can be an empty string.

http://codereview.chromium.org/7655056/diff/29001/chrome/browser/mac/master_p...
File chrome/browser/mac/master_prefs.h (right):

http://codereview.chromium.org/7655056/diff/29001/chrome/browser/mac/master_p...
chrome/browser/mac/master_prefs.h:13: // Returns the path to the master
preferences file.
Specify in the comment:
 • that this might return an empty FilePath
 • that the path returned might not exist

There’s no need to really go into detail in this comment or in the one in the
other file the specific conditions under which this might return an empty file
or  path to a nonexistent file. It’s sufficient to merely document that this
might happen.

[Avi (use Gerrit), 2011-08-25 15:24:52]

http://codereview.chromium.org/7655056/diff/29001/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run_mac.mm (right):

http://codereview.chromium.org/7655056/diff/29001/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run_mac.mm:20: return StartsWithASCII(brand,
"GG", true) ||
On 2011/08/25 13:35:08, Mark Mentovai wrote:
> Caution! No brand code at all is also organic.

Done.

http://codereview.chromium.org/7655056/diff/29001/chrome/browser/mac/keystone...
File chrome/browser/mac/keystone_glue.h (right):

http://codereview.chromium.org/7655056/diff/29001/chrome/browser/mac/keystone...
chrome/browser/mac/keystone_glue.h:201: // Returns the brand code of the
installation.
On 2011/08/25 13:35:08, Mark Mentovai wrote:
> Specify in the comment that this can be an empty string.

Done.

http://codereview.chromium.org/7655056/diff/29001/chrome/browser/mac/master_p...
File chrome/browser/mac/master_prefs.h (right):

http://codereview.chromium.org/7655056/diff/29001/chrome/browser/mac/master_p...
chrome/browser/mac/master_prefs.h:13: // Returns the path to the master
preferences file.
On 2011/08/25 13:35:08, Mark Mentovai wrote:
> Specify in the comment:
>  • that this might return an empty FilePath
>  • that the path returned might not exist
> 
> There’s no need to really go into detail in this comment or in the one in the
> other file the specific conditions under which this might return an empty file
> or  path to a nonexistent file. It’s sufficient to merely document that this
> might happen.

Done.

http://codereview.chromium.org/7655056/diff/29001/chrome/browser/mac/master_p...
chrome/browser/mac/master_prefs.h:13: // Returns the path to the master
preferences file.
On 2011/08/25 13:35:08, Mark Mentovai wrote:
> Specify in the comment:
>  • that this might return an empty FilePath
>  • that the path returned might not exist
> 
> There’s no need to really go into detail in this comment or in the one in the
> other file the specific conditions under which this might return an empty file
> or  path to a nonexistent file. It’s sufficient to merely document that this
> might happen.

Done.

[Mark Mentovai, 2011-08-26 23:28:53]

I see that you uploaded a new version and responded, but I never got the e-mail
with your responses. I’m sending this mail to keep this fresh.

[Mark Mentovai, 2011-08-27 00:07:19]

LGTM