Changed mac and win Official Chrome build to drop default app CRXs in the build/installer output.

Changed Mac, Linux and Win Official Chrome build to drop default app CRXs in the build/installer output.

BUG=94920
TEST=Do an Official build in mac, linux and win. Ensure that after installing the app, you can find the default apps under the install's directory structure. For win, this is under a default_apps directory, for mac, this is under a "Default Apps" directory.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99440

[SteveT, 2011-09-01 20:02:26]

Letting you guys know about this ahead of time - I named the directory based on
what you are expecting in your crx installation patch.

This will go out for review as soon as I finish testing this against Official vs
Unofficial builds.

[SteveT, 2011-09-01 20:36:37]

Hi guys,

Robert - Could you look at chrome_dll.gypi top hunk, and chrome.release?

Mark - Could you look at chrome_dll.gypi bottom hunk?

Thanks!


Steve

[Mark Mentovai, 2011-09-01 20:38:41]

Please don’t use the *, it screws with all sorts of things. List the
files you need explicitly.

[SteveT, 2011-09-01 20:48:24]

Using explicit paths instead. Over to you.

[robertshield, 2011-09-01 20:49:31]

On 2011/09/01 20:38:41, Mark Mentovai wrote:
> Please don’t use the *, it screws with all sorts of things. List the
> files you need explicitly.

I was going to say use *.crx instead of *. 

Mark, what sorts of things does * in the chrome.release file screw with?

As long as the files it is pulling in are from a dedicated directory (here
default_apps), there isn't too much risk of accidentally pulling in more stuff
than intended.

[Mark Mentovai, 2011-09-01 20:53:38]

http://codereview.chromium.org/7827021/diff/6/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

http://codereview.chromium.org/7827021/diff/6/chrome/chrome_dll.gypi#newcode169
chrome/chrome_dll.gypi:169: 'destination': '<(PRODUCT_DIR)/default_apps',
Why do you need to copy things to <(PRODUCT_DIR)/default_apps? I don’t think you
ever reference anything in this location.

http://codereview.chromium.org/7827021/diff/6/chrome/chrome_dll.gypi#newcode608
chrome/chrome_dll.gypi:608: '<(PRODUCT_DIR)/$(CONTENTS_FOLDER_PATH)/Default
Apps',
I commented in 7811020 about this. I don’t think the path you’re using there is
correct.

[Mark Mentovai, 2011-09-01 20:55:03]

robertshield@chromium.org wrote:
> On 2011/09/01 20:38:41, Mark Mentovai wrote:
>>
>> Please don’t use the *, it screws with all sorts of things. List the
>> files you need explicitly.
>
> I was going to say use *.crx instead of *.
>
> Mark, what sorts of things does * in the chrome.release file screw with?
>
> As long as the files it is pulling in are from a dedicated directory (here
> default_apps), there isn't too much risk of accidentally pulling in more
> stuff than intended.

It messes with dependency tracking and it pulls unintended garbage in
from the filesystem. A properly-specified build never uses *.

[robertshield, 2011-09-01 20:58:18]

On 2011/09/01 20:55:03, Mark Mentovai wrote:
> mailto:robertshield@chromium.org wrote:
> > On 2011/09/01 20:38:41, Mark Mentovai wrote:
> >>
> >> Please don’t use the *, it screws with all sorts of things. List the
> >> files you need explicitly.
> >
> > I was going to say use *.crx instead of *.
> >
> > Mark, what sorts of things does * in the chrome.release file screw with?
> >
> > As long as the files it is pulling in are from a dedicated directory (here
> > default_apps), there isn't too much risk of accidentally pulling in more
> > stuff than intended.
> 
> It messes with dependency tracking and it pulls unintended garbage in
> from the filesystem. A properly-specified build never uses *.

Ok, fair enough. I was seeing the precedent for e.g. the locale dlls. 

If we avoid using the wildcard though, I suggest adding a comment or similar to
the file list in the gyp file indicating that changes also need to be made to
the .release file. It is non-obvious from reading one or the other that the two
must be kept in sync.

[Mark Mentovai, 2011-09-01 21:03:43]

I don’t know anything about chrome.release.

Although looking at it, I see that the “other” copy in stevet’s change
was for non-Mac.

Steve, since it seems we need to keep both “copies” sections, I
recommend one of two things:

1. Merge them and have the output directory vary based on a conditional, or
2. Keep them distinct, but have a default_apps_files GYP variable that
the two copies blocks can share. (The variable name must end in _file,
_dir, or _path, or their plural forms, so GYP knows they’re
pathnames.)

robertshield@chromium.org wrote:
> On 2011/09/01 20:55:03, Mark Mentovai wrote:
>>
>> mailto:robertshield@chromium.org wrote:
>> > On 2011/09/01 20:38:41, Mark Mentovai wrote:
>> >>
>> >> Please don’t use the *, it screws with all sorts of things. List the
>> >> files you need explicitly.
>> >
>> > I was going to say use *.crx instead of *.
>> >
>> > Mark, what sorts of things does * in the chrome.release file screw with?
>> >
>> > As long as the files it is pulling in are from a dedicated directory
>> > (here
>> > default_apps), there isn't too much risk of accidentally pulling in more
>> > stuff than intended.
>
>> It messes with dependency tracking and it pulls unintended garbage in
>> from the filesystem. A properly-specified build never uses *.
>
> Ok, fair enough. I was seeing the precedent for e.g. the locale dlls.
>
> If we avoid using the wildcard though, I suggest adding a comment or similar
> to
> the file list in the gyp file indicating that changes also need to be made
> to
> the .release file. It is non-obvious from reading one or the other that the
> two
> must be kept in sync.
>
> http://codereview.chromium.org/7827021/
>

[SteveT, 2011-09-01 21:19:57]

I've added comments to make the two sections more clear as to what platform they
are copying files for, given how long this gypi file is.

Robert - Should I also change chrome.release to use explicit file names, or
*.crx instead of *.*?

PTAL.

http://codereview.chromium.org/7827021/diff/6/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

http://codereview.chromium.org/7827021/diff/6/chrome/chrome_dll.gypi#newcode169
chrome/chrome_dll.gypi:169: 'destination': '<(PRODUCT_DIR)/default_apps',
This is the expected location for win, according to 7811020. I'll add a comment
here for Robert about how these are pulled in by chrome.release, and also
explicitly mention that this is for Windows.

http://codereview.chromium.org/7827021/diff/6/chrome/chrome_dll.gypi#newcode608
chrome/chrome_dll.gypi:608: '<(PRODUCT_DIR)/$(CONTENTS_FOLDER_PATH)/Default
Apps',
Discussed with Cait. I thought they had implemented that bit with the correct
location, but that's not the case yet. They'll address that before committing.

[SteveT, 2011-09-01 21:25:01]

Missed Mark's last comment. Will have to address that first...

[SteveT, 2011-09-01 21:49:52]

Okay, still have to test this with Official/unofficial builds, but changes are
done. PTAL.

[Mark Mentovai, 2011-09-01 21:55:34]

LGTM pending the results of your test.

http://codereview.chromium.org/7827021/diff/4006/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

http://codereview.chromium.org/7827021/diff/4006/chrome/chrome_dll.gypi#newco...
chrome/chrome_dll.gypi:170: ['buildtype=="Official" and branding=="Chrome"', {
I am positive that this condition is correct for the Mac. Your testing will
reveal whether it’s correct for Windows too.

http://codereview.chromium.org/7827021/diff/4006/chrome/chrome_dll.gypi#newco...
chrome/chrome_dll.gypi:173: # This location is for the win build. The
chrome.release
win = Windows.

http://codereview.chromium.org/7827021/diff/4006/chrome/chrome_dll.gypi#newco...
chrome/chrome_dll.gypi:613: # This location is for the mac build.
Mac has a capital M.

[SteveT, 2011-09-01 22:39:55]

Nits addressed.

Robert - over to you. Still wondering if it's cool to use *.* in chrome.release
or if I should use *.crx or the explicit file paths.

http://codereview.chromium.org/7827021/diff/4006/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

http://codereview.chromium.org/7827021/diff/4006/chrome/chrome_dll.gypi#newco...
chrome/chrome_dll.gypi:170: ['buildtype=="Official" and branding=="Chrome"', {
Yup - it's correct for both. I've actually already tested this for both
platforms for both cases (Official Chrome and not). I'm just re-testing to make
sure my latest gyp changes do indeed work.

http://codereview.chromium.org/7827021/diff/4006/chrome/chrome_dll.gypi#newco...
chrome/chrome_dll.gypi:173: # This location is for the win build. The
chrome.release
On 2011/09/01 21:55:34, Mark Mentovai wrote:
> win = Windows.

Done.

http://codereview.chromium.org/7827021/diff/4006/chrome/chrome_dll.gypi#newco...
chrome/chrome_dll.gypi:613: # This location is for the mac build.
On 2011/09/01 21:55:34, Mark Mentovai wrote:
> Mac has a capital M.

Done.

[robertshield, 2011-09-01 23:37:29]

On Thu, Sep 1, 2011 at 6:39 PM, <stevet@chromium.org> wrote:

> Nits addressed.
>
> Robert - over to you. Still wondering if it's cool to use *.* in
> chrome.release
> or if I should use *.crx or the explicit file paths.


If you're only packaging .crx files, then use *.crx imo. *.* feels broader
than needed.

One more note, I noticed that there is already a line in the .release file
that reads:

Extensions\*.*: %(VersionDir)s\Extensions\

I assume this different from default_apps?


>
>
>
> http://codereview.chromium.**org/7827021/diff/4006/chrome/**
>
chrome_dll.gypi<http://codereview.chromium.org/7827021/diff/4006/chrome/chrome_dll.gypi>
> File chrome/chrome_dll.gypi (right):
>
> http://codereview.chromium.**org/7827021/diff/4006/chrome/**
>
chrome_dll.gypi#newcode170<http://codereview.chromium.org/7827021/diff/4006/chrome/chrome_dll.gypi#newcode170>
> chrome/chrome_dll.gypi:170: ['buildtype=="Official" and
> branding=="Chrome"', {
> Yup - it's correct for both. I've actually already tested this for both
> platforms for both cases (Official Chrome and not). I'm just re-testing
> to make sure my latest gyp changes do indeed work.
>
>
> http://codereview.chromium.**org/7827021/diff/4006/chrome/**
>
chrome_dll.gypi#newcode173<http://codereview.chromium.org/7827021/diff/4006/chrome/chrome_dll.gypi#newcode173>
> chrome/chrome_dll.gypi:173: # This location is for the win build. The
> chrome.release
> On 2011/09/01 21:55:34, Mark Mentovai wrote:
>
>> win = Windows.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/7827021/diff/4006/chrome/**
>
chrome_dll.gypi#newcode613<http://codereview.chromium.org/7827021/diff/4006/chrome/chrome_dll.gypi#newcode613>
> chrome/chrome_dll.gypi:613: # This location is for the mac build.
> On 2011/09/01 21:55:34, Mark Mentovai wrote:
>
>> Mac has a capital M.
>>
>
> Done.
>
>
>
http://codereview.chromium.**org/7827021/<http://codereview.chromium.org/7827...
>

[robertshield, 2011-09-01 23:38:39]

On 2011/09/01 23:37:29, robertshield wrote:
> On Thu, Sep 1, 2011 at 6:39 PM, <mailto:stevet@chromium.org> wrote:
> 
> > Nits addressed.
> >
> > Robert - over to you. Still wondering if it's cool to use *.* in
> > chrome.release
> > or if I should use *.crx or the explicit file paths.
> 
> 
> If you're only packaging .crx files, then use *.crx imo. *.* feels broader
> than needed.
> 
> One more note, I noticed that there is already a line in the .release file
> that reads:
> 
> Extensions\*.*: %(VersionDir)s\Extensions\
> 
> I assume this different from default_apps?
> 

Oh and if so and with the switch to *.crx, LGTM :-)

> 
> >
> >
> >
> > http://codereview.chromium.**org/7827021/diff/4006/chrome/**
> >
>
chrome_dll.gypi<http://codereview.chromium.org/7827021/diff/4006/chrome/chrome_dll.gypi>
> > File chrome/chrome_dll.gypi (right):
> >
> > http://codereview.chromium.**org/7827021/diff/4006/chrome/**
> >
>
chrome_dll.gypi#newcode170<http://codereview.chromium.org/7827021/diff/4006/chrome/chrome_dll.gypi#newcode170>
> > chrome/chrome_dll.gypi:170: ['buildtype=="Official" and
> > branding=="Chrome"', {
> > Yup - it's correct for both. I've actually already tested this for both
> > platforms for both cases (Official Chrome and not). I'm just re-testing
> > to make sure my latest gyp changes do indeed work.
> >
> >
> > http://codereview.chromium.**org/7827021/diff/4006/chrome/**
> >
>
chrome_dll.gypi#newcode173<http://codereview.chromium.org/7827021/diff/4006/chrome/chrome_dll.gypi#newcode173>
> > chrome/chrome_dll.gypi:173: # This location is for the win build. The
> > chrome.release
> > On 2011/09/01 21:55:34, Mark Mentovai wrote:
> >
> >> win = Windows.
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.**org/7827021/diff/4006/chrome/**
> >
>
chrome_dll.gypi#newcode613<http://codereview.chromium.org/7827021/diff/4006/chrome/chrome_dll.gypi#newcode613>
> > chrome/chrome_dll.gypi:613: # This location is for the mac build.
> > On 2011/09/01 21:55:34, Mark Mentovai wrote:
> >
> >> Mac has a capital M.
> >>
> >
> > Done.
> >
> >
> >
>
http://codereview.chromium.**org/7827021/%3Chttp://codereview.chromium.org/78...>
> >

[SteveT, 2011-09-02 13:28:30]

Thanks, guys.

Robert - I've switched to *.crx. And yes, that Extensions\*.* line is from
something different - the old default_extensions mechanism.

[SteveT, 2011-09-02 19:12:41]

Mike - PTAL, as I've made the changes we discussed:
 - Move windows changes to chrome.gyp where they'll work with linux as well.
 - Only check branding==Chrome.

[SteveT, 2011-09-02 20:18:13]

Mike - Made changes to chrome_installer.gyp as we discussed. Over to you.

[mmoss, 2011-09-02 20:18:49]

LGTM

[mmoss, 2011-09-02 20:19:16]

Nit: Change the CL description to include Linux now.

[Mark Mentovai, 2011-09-02 20:24:40]

http://codereview.chromium.org/7827021/diff/5009/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/7827021/diff/5009/chrome/chrome.gyp#newcode1759
chrome/chrome.gyp:1759: # directories.
Just say “inside the framework” (and not “directories.”) There is nothing
specifically named Versions/Frameworks. Versions is actually kind of meaningless
to this discussion. Frameworks isn’t where these need to wind up either. As
written the comment is more confusing than helpful. “Inside the framework” is
accurate and helpful.

http://codereview.chromium.org/7827021/diff/5009/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

http://codereview.chromium.org/7827021/diff/5009/chrome/chrome_dll.gypi#newco...
chrome/chrome_dll.gypi:599: # location under the Versions/Frameworks
directories.
Revise this comment as discussed in the other file.

[SteveT, 2011-09-02 20:30:22]

All done.

http://codereview.chromium.org/7827021/diff/5009/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/7827021/diff/5009/chrome/chrome.gyp#newcode1759
chrome/chrome.gyp:1759: # directories.
On 2011/09/02 20:24:40, Mark Mentovai wrote:
> Just say “inside the framework” (and not “directories.”) There is nothing
> specifically named Versions/Frameworks. Versions is actually kind of
meaningless
> to this discussion. Frameworks isn’t where these need to wind up either. As
> written the comment is more confusing than helpful. “Inside the framework” is
> accurate and helpful.

Done.

http://codereview.chromium.org/7827021/diff/5009/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

http://codereview.chromium.org/7827021/diff/5009/chrome/chrome_dll.gypi#newco...
chrome/chrome_dll.gypi:599: # location under the Versions/Frameworks
directories.
On 2011/09/02 20:24:40, Mark Mentovai wrote:
> Revise this comment as discussed in the other file.

Done.

[Mark Mentovai, 2011-09-02 20:31:44]

LGTM again.