Continue to remove CHROME_FRAME_BUILD define from code that goes into chrome....

Continue to remove CHROME_FRAME_BUILD define from code that goes into chrome.dll. This reworks the browser distribution code to use the ChromeFrameBrowserDistribution iff --chrome-frame is present on the command line.

Also,

* At startup, chrome.exe now uses the BrowserDistribution code to determine where the Chromium version key resides (instead of hard coding it).
* The installer now propagates the presence of --verbose-logging to uninstalls.
* The chrome_launcher now allows the --chrome-frame switch through to chrome.
* The installer now accepts a --chrome-frame switch.
* Remove almost all occurences of the CHROME_FRAME_BUILD define from the installer.


BUG=26012, 26603
TEST=Chrome Frame still builds and runs correctly. Chrome Frame builds built without 'branding'='Chrome' now install correctly.



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

[kuchhal, 2009-10-29 17:47:25]

http://codereview.chromium.org/345021/diff/1/5
File chrome/app/chrome_exe_main.cc (right):

http://codereview.chromium.org/345021/diff/1/5#newcode67
Line 67: &version)) {
Did this change increase the size of chrome.exe?

http://codereview.chromium.org/345021/diff/1/6
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/345021/diff/1/6#newcode32
Line 32: extern const char kChromeFrame[];
switch order with line 31.

http://codereview.chromium.org/345021/diff/1/8
File chrome/installer/setup/install.cc (right):

http://codereview.chromium.org/345021/diff/1/8#newcode105
Line 105: #if defined(CHROME_FRAME_BUILD)
change this to BrowserDistribution check?

http://codereview.chromium.org/345021/diff/1/8#newcode119
Line 119: }
What is the reason for this change? I am not opposed to it just want to know the
rationale. Lot of times while troubleshooting I would ask users to run installer
with verbose-logging flag. Not sure if I would want to run the uninstaller with
verbose logging too.

http://codereview.chromium.org/345021/diff/1/10
File chrome/installer/util/browser_distribution.cc (right):

http://codereview.chromium.org/345021/diff/1/10#newcode23
Line 23: // the switch may become necessary.
This sounds very error prone. Is it possible to add a runtime check so that if
someone tries to call this from non-browser process, we do something not very
pretty?

http://codereview.chromium.org/345021/diff/1/10#newcode27
Line 27: BrowserDistribution* BrowserDistribution::GetDistribution(bool
chrome_frame) {
May be change this function to be local since it should not be called from
outside?

[amit, 2009-10-29 18:35:42]

lg

http://codereview.chromium.org/345021/diff/1/5
File chrome/app/chrome_exe_main.cc (right):

http://codereview.chromium.org/345021/diff/1/5#newcode62
Line 62: BrowserDistribution* dist = BrowserDistribution::GetDistribution();
DCHECK?

http://codereview.chromium.org/345021/diff/1/8
File chrome/installer/setup/install.cc (right):

http://codereview.chromium.org/345021/diff/1/8#newcode276
Line 276: const CommandLine& command_line = *CommandLine::ForCurrentProcess();
A lot of code seems to be common here, should be moved in a separate helper.

http://codereview.chromium.org/345021/diff/1/10
File chrome/installer/util/browser_distribution.cc (right):

http://codereview.chromium.org/345021/diff/1/10#newcode30
Line 30: if (chrome_frame) {
I think irrespective of whether this is an official chrome build or chromium
build we need chrome frame specific 'distribution'. In that sense the 'chrome
frame distribution' just overrides a few properties of the base distribution.
Maybe a decorator pattern could be of use here?

[robertshield, 2009-10-30 17:44:18]

Addressed comments and added the use of an environment variable to track
chrome-frame-y-ness down to child processes.

Please take another look, thanks!

http://codereview.chromium.org/345021/diff/1/5
File chrome/app/chrome_exe_main.cc (right):

http://codereview.chromium.org/345021/diff/1/5#newcode62
Line 62: BrowserDistribution* dist = BrowserDistribution::GetDistribution();
On 2009/10/29 18:35:42, amit wrote:
> DCHECK?

I would normally, but nowhere else that GetDistribution is called is there a
DCHECK, and GetDistribution cannot return NULL atm. I'll put one if you really
want :-)

http://codereview.chromium.org/345021/diff/1/5#newcode67
Line 67: &version)) {
On 2009/10/29 17:47:25, kuchhal wrote:
> Did this change increase the size of chrome.exe?

Not in debug mode. Will check with optimizations turned on, I was wrong before
in that chrome.exe already depended on installer_util.lib - I didn't have to add
a new dependency.

http://codereview.chromium.org/345021/diff/1/6
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/345021/diff/1/6#newcode32
Line 32: extern const char kChromeFrame[];
On 2009/10/29 17:47:25, kuchhal wrote:
> switch order with line 31.

Oops, done. /me recites alphabet song again.

http://codereview.chromium.org/345021/diff/1/8
File chrome/installer/setup/install.cc (right):

http://codereview.chromium.org/345021/diff/1/8#newcode105
Line 105: #if defined(CHROME_FRAME_BUILD)
On 2009/10/29 17:47:25, kuchhal wrote:
> change this to BrowserDistribution check?

Done and removed most of the other occurrences of CHROME_FRAME_BUILD as well.

http://codereview.chromium.org/345021/diff/1/8#newcode119
Line 119: }
On 2009/10/29 17:47:25, kuchhal wrote:
> What is the reason for this change? I am not opposed to it just want to know
the
> rationale. Lot of times while troubleshooting I would ask users to run
installer
> with verbose-logging flag. Not sure if I would want to run the uninstaller
with
> verbose logging too.

Oh, I was just debugging stuff and was running the installer with
--verbose-logging but when I went to uninstall, I realised I got very un-verbose
logs unless I ran the uninstaller manually. This seemed like a safe and usually
handy change. (I think the command line would get put back to normal on the next
omaha update if a user ran the installer manually with verbose logging enabled.)

http://codereview.chromium.org/345021/diff/1/8#newcode276
Line 276: const CommandLine& command_line = *CommandLine::ForCurrentProcess();
On 2009/10/29 18:35:42, amit wrote:
> A lot of code seems to be common here, should be moved in a separate helper.

Done.

http://codereview.chromium.org/345021/diff/1/10
File chrome/installer/util/browser_distribution.cc (right):

http://codereview.chromium.org/345021/diff/1/10#newcode23
Line 23: // the switch may become necessary.
On 2009/10/29 17:47:25, kuchhal wrote:
> This sounds very error prone. Is it possible to add a runtime check so that if
> someone tries to call this from non-browser process, we do something not very
> pretty?

So this doesn't actually work as originally stated - I need to propagate the
command line to all child processes. Actually doing this looks cumbersome, so
I'm going to try setting an env var in the main browser process instead.

http://codereview.chromium.org/345021/diff/1/10#newcode27
Line 27: BrowserDistribution* BrowserDistribution::GetDistribution(bool
chrome_frame) {
On 2009/10/29 17:47:25, kuchhal wrote:
> May be change this function to be local since it should not be called from
> outside?

Done.

http://codereview.chromium.org/345021/diff/1/10#newcode30
Line 30: if (chrome_frame) {
On 2009/10/29 18:35:42, amit wrote:
> I think irrespective of whether this is an official chrome build or chromium
> build we need chrome frame specific 'distribution'. In that sense the 'chrome
> frame distribution' just overrides a few properties of the base distribution.
> Maybe a decorator pattern could be of use here?

My understanding of the decorator pattern is that it is mainly intended to be a
way of adding extra properties / methods to decorated classes. Here, the
ChromeFrameDistribution overrides the majority of methods from
BrowserDistribution (and adds no new ones), so straight subclassing seemed the
way to go.

The TODO comment mainly referred to whether Chrome Frame itself needs to do
something different for official release builds vs. 'chromium' type builds as
opposed to having just a single Chrome Frame specific Distribution as we do now.

[kuchhal, 2009-10-30 22:20:08]

lgtm.

http://codereview.chromium.org/345021/diff/5002/8016
File chrome/browser/browser_main_win.cc (right):

http://codereview.chromium.org/345021/diff/5002/8016#newcode43
Line 43: DCHECK(result) << "Failed to set Chrome Frame environment variable";
This should probably be a more serious error?

http://codereview.chromium.org/345021/diff/5002/8009
File chrome/installer/util/browser_distribution.cc (right):

http://codereview.chromium.org/345021/diff/5002/8009#newcode49
Line 49: dist = new GoogleChromeDistribution();
indent by two more spaces, also line 51.

http://codereview.chromium.org/345021/diff/5002/8014
File chrome/installer/util/install_util.cc (right):

http://codereview.chromium.org/345021/diff/5002/8014#newcode136
Line 136: return
command_line->HasSwitch(installer_util::switches::kChromeFrame);
Since this method is not checking for CHROME_FRAME env var, you should probably
mention in header comments to never use it from Chrome.

[robertshield, 2009-11-03 22:42:27]

I reworked this with to not use environment variables, based on the general
community feedback that env vars are bad. 

I now propagate the chrome frame command line switch down to child processes in
places where said processes are spawned. I added this propagation to the sandbox
policy code as well as to all code paths I could find that didn't go through the
sandbox.

Please could you take another look?

http://codereview.chromium.org/345021/diff/5002/8009
File chrome/installer/util/browser_distribution.cc (right):

http://codereview.chromium.org/345021/diff/5002/8009#newcode49
Line 49: dist = new GoogleChromeDistribution();
On 2009/10/30 22:20:08, kuchhal wrote:
> indent by two more spaces, also line 51.

Done.

[amit, 2009-11-04 17:09:11]

lg

http://codereview.chromium.org/345021/diff/6020/7023
File chrome/browser/first_run_win.cc (right):

http://codereview.chromium.org/345021/diff/6020/7023#newcode124
Line 124: CommandLine& browser_command_line = *CommandLine::ForCurrentProcess();
nit: reads odd, maybe use CommandLine*?

http://codereview.chromium.org/345021/diff/6020/7016
File chrome/installer/util/browser_distribution.cc (right):

http://codereview.chromium.org/345021/diff/6020/7016#newcode21
Line 21: // flag or if we're a non-browser process and we have the CHROME_FRAME
env. var.
pl update the comment

http://codereview.chromium.org/345021/diff/6020/7021
File chrome/installer/util/install_util.cc (right):

http://codereview.chromium.org/345021/diff/6020/7021#newcode132
Line 132: bool InstallUtil::IsChromeFrameInstall() {
Isn't this is exactly the same as IsChromeProcess?

[kuchhal, 2009-11-04 18:04:06]

I believe yesterday Carlos committed a change in chrome.exe code that will
not merge cleanly with this change. May be you can merge with ToT and
re-upload the new change?

On Tue, Nov 3, 2009 at 2:42 PM, <robertshield@chromium.org> wrote:

> I reworked this with to not use environment variables, based on the general
> community feedback that env vars are bad.
>
> I now propagate the chrome frame command line switch down to child
> processes in
> places where said processes are spawned. I added this propagation to the
> sandbox
> policy code as well as to all code paths I could find that didn't go
> through the
> sandbox.
>
> Please could you take another look?
>
>
>
> http://codereview.chromium.org/345021/diff/5002/8009
> File chrome/installer/util/browser_distribution.cc (right):
>
> http://codereview.chromium.org/345021/diff/5002/8009#newcode49
> Line 49: dist = new GoogleChromeDistribution();
> On 2009/10/30 22:20:08, kuchhal wrote:
>
>> indent by two more spaces, also line 51.
>>
>
> Done.
>
>
> http://codereview.chromium.org/345021
>

[robertshield, 2009-11-04 20:09:51]

Rahul, I updated to merge with Carlos' new stuff.

Please take one more look :-)

http://codereview.chromium.org/345021/diff/6020/7023
File chrome/browser/first_run_win.cc (right):

http://codereview.chromium.org/345021/diff/6020/7023#newcode124
Line 124: CommandLine& browser_command_line = *CommandLine::ForCurrentProcess();
On 2009/11/04 17:09:12, amit wrote:
> nit: reads odd, maybe use CommandLine*?

Done.

http://codereview.chromium.org/345021/diff/6020/7016
File chrome/installer/util/browser_distribution.cc (right):

http://codereview.chromium.org/345021/diff/6020/7016#newcode21
Line 21: // flag or if we're a non-browser process and we have the CHROME_FRAME
env. var.
On 2009/11/04 17:09:12, amit wrote:
> pl update the comment

Removed this function.

http://codereview.chromium.org/345021/diff/6020/7021
File chrome/installer/util/install_util.cc (right):

http://codereview.chromium.org/345021/diff/6020/7021#newcode132
Line 132: bool InstallUtil::IsChromeFrameInstall() {
On 2009/11/04 17:09:12, amit wrote:
> Isn't this is exactly the same as IsChromeProcess?

Ugh, yeah, too close to the trees. I removed the IsChromeFrameProcess() call
from browser_distribution.cc.
Thanks!

[kuchhal, 2009-11-04 20:55:07]

lgtm but see a comment below.

http://codereview.chromium.org/345021/diff/14001/14020
File chrome/app/client_util.cc (right):

http://codereview.chromium.org/345021/diff/14001/14020#newcode179
Line 179: key.append(L"\\").append(google_update::kChromeGuid);
You probably need to change this too, to pick up CF GUID?

[robertshield, 2009-11-04 22:15:39]

http://codereview.chromium.org/345021/diff/14001/14020
File chrome/app/client_util.cc (right):

http://codereview.chromium.org/345021/diff/14001/14020#newcode179
Line 179: key.append(L"\\").append(google_update::kChromeGuid);
On 2009/11/04 20:55:08, kuchhal wrote:
> You probably need to change this too, to pick up CF GUID?

kChromeGuid is defined in a generated header that is specified at build time.
This will need to be updated, so that it can be given on the command line too,
but I am going to do that in a different patch. Thanks!