Detect new instance of the browser when running in the background in persiste...

Detect new instance of the browser when running in the background in persistent
mode, shutdown and restart the new instance.  This is already done for Windows,
this CL enables the functionality for Linux.

We don't yet have a unit test for this.  Local testing is done by:
1) Reducing the timer to 30 seconds.
2) Changing BrowserList::IsInPersistentMode to return true.
3) Setting BrowserProcessImpl::autoupdate_timer_ to 30 seconds interval.
4) Running "touch" command on chrome exe to pretend there is an update.

BUG=40975
TEST=none


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

[Andrew T Wilson (Slow), 2010-04-21 05:22:40]

Sorry this took me so long to get to - I usually try to turn these around much
quicker, but for some reason I didn't see the original review email.

The code itself looks fine - nice work! I had a few style comments as well as
some concerns about including this code on ChromeOS.

http://codereview.chromium.org/1633021/diff/21002/31006
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/1633021/diff/21002/31006#newcode1172
chrome/browser/browser_main.cc:1172: #if defined(OS_WIN) || defined(OS_LINUX)
I don't think we need to do this on ChromeOS, but I believe that ChromeOS
#defines OS_LINUX so this would be executed for ChromeOS also. See if you can
find an example of some place that is #if LINUX without including ChromeOS.

http://codereview.chromium.org/1633021/diff/21002/31006#newcode1184
chrome/browser/browser_main.cc:1184: #endif  // OS_LINUX
I think this would be cleaner as two separate #if statements instead of as a
nested #if.

http://codereview.chromium.org/1633021/diff/21002/31002
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/1633021/diff/21002/31002#newcode142
chrome/browser/browser_process.h:142: #if defined(OS_WIN) || defined(OS_LINUX)
See previous comment about avoiding ChromeOS (assume that applies to the rest of
the #if statements).

http://codereview.chromium.org/1633021/diff/21002/31005
File chrome/browser/first_run.h (right):

http://codereview.chromium.org/1633021/diff/21002/31005#newcode185
chrome/browser/first_run.h:185: // upgrade becomes available while the browser
kept alive with no open windows
nit: should probably read "while the browser *is* kept alive"

http://codereview.chromium.org/1633021/diff/21002/31005#newcode187
chrome/browser/first_run.h:187: class Upgrade {
What do you think about combining this with the OS_WIN class above? I think it's
OK to be separate, but if we add more APIs down the road we'll have to duplicate
them in two places.

http://codereview.chromium.org/1633021/diff/21002/31005#newcode197
chrome/browser/first_run.h:197: static double last_modified_time_of_exe_;
Perhaps we should name this "original_modification_time_of_exe_" or something
similar, to differentiate it from what GetLastModifiedTimeOfExe() returns. We
might also want to change "SaveLastModifiedTimeOfExe" to be
"SaveOriginalModified..."

It's your call - it's probably OK as-is, but as a casual reader you might expect
GetLastModifiedTimeOfExe() to return the last_modified_time_of_exe_ instance
variable.

http://codereview.chromium.org/1633021/diff/21002/31003
File chrome/browser/first_run_gtk.cc (right):

http://codereview.chromium.org/1633021/diff/21002/31003#newcode120
chrome/browser/first_run_gtk.cc:120: return false;
It's odd to return false from a function defined to return double. Also, if for
some reason we get a transient error trying to fetch the modification time, it
will force us to restart. Perhaps if we get an error we should return
last_modified_time_of_exe_ instead (after logging something appropriate) to
avoid spurious restarts?

Also, it seems to be preferred chrome style to omit braces from one-line control
statements. So:

if (something)
    return false;

Both are technically correct, as long as you maintain consistency in each file -
since this file already is omitting braces, you should follow that style in your
new code also.

[prasadt_google, 2010-04-21 21:42:33]

Thanks for the review.  Addressed all feedback.

http://codereview.chromium.org/1633021/diff/21002/31006
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/1633021/diff/21002/31006#newcode1172
chrome/browser/browser_main.cc:1172: #if defined(OS_WIN) || defined(OS_LINUX)
On 2010/04/21 05:22:40, Andrew T Wilson wrote:
> I don't think we need to do this on ChromeOS, but I believe that ChromeOS
> #defines OS_LINUX so this would be executed for ChromeOS also. See if you can
> find an example of some place that is #if LINUX without including ChromeOS.

Done.

http://codereview.chromium.org/1633021/diff/21002/31006#newcode1184
chrome/browser/browser_main.cc:1184: #endif  // OS_LINUX
On 2010/04/21 05:22:40, Andrew T Wilson wrote:
> I think this would be cleaner as two separate #if statements instead of as a
> nested #if.

Done.

http://codereview.chromium.org/1633021/diff/21002/31002
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/1633021/diff/21002/31002#newcode142
chrome/browser/browser_process.h:142: #if defined(OS_WIN) || defined(OS_LINUX)
On 2010/04/21 05:22:40, Andrew T Wilson wrote:
> See previous comment about avoiding ChromeOS (assume that applies to the rest
of
> the #if statements).

Done.  Fixed up the #if all other places too.

http://codereview.chromium.org/1633021/diff/21002/31005
File chrome/browser/first_run.h (right):

http://codereview.chromium.org/1633021/diff/21002/31005#newcode185
chrome/browser/first_run.h:185: // upgrade becomes available while the browser
kept alive with no open windows
On 2010/04/21 05:22:40, Andrew T Wilson wrote:
> nit: should probably read "while the browser *is* kept alive"

Done.

http://codereview.chromium.org/1633021/diff/21002/31005#newcode187
chrome/browser/first_run.h:187: class Upgrade {
On 2010/04/21 05:22:40, Andrew T Wilson wrote:
> What do you think about combining this with the OS_WIN class above? I think
it's
> OK to be separate, but if we add more APIs down the road we'll have to
duplicate
> them in two places.

Done.

I initially created a new class just to figure what I needed and when I was
done, it seemed cleaner than to combine.  But  I see that the comments are
scattered across two classes.  So, combined now.

http://codereview.chromium.org/1633021/diff/21002/31005#newcode197
chrome/browser/first_run.h:197: static double last_modified_time_of_exe_;
On 2010/04/21 05:22:40, Andrew T Wilson wrote:
> Perhaps we should name this "original_modification_time_of_exe_" or something
> similar, to differentiate it from what GetLastModifiedTimeOfExe() returns. We
> might also want to change "SaveLastModifiedTimeOfExe" to be
> "SaveOriginalModified..."
> 
> It's your call - it's probably OK as-is, but as a casual reader you might
expect
> GetLastModifiedTimeOfExe() to return the last_modified_time_of_exe_ instance
> variable.

I think {Save,Get}LastModifiedTimeOfExe are fairly intuitive.  I agreed
last_modified_time_of_exe_ variable name is not gelling well with these function
names.  Changed that to saved_last_modified_time_of_exe_.  Hope that makes it
easier to read.

http://codereview.chromium.org/1633021/diff/21002/31003
File chrome/browser/first_run_gtk.cc (right):

http://codereview.chromium.org/1633021/diff/21002/31003#newcode120
chrome/browser/first_run_gtk.cc:120: return false;
On 2010/04/21 05:22:40, Andrew T Wilson wrote:
> It's odd to return false from a function defined to return double. Also, if
for
> some reason we get a transient error trying to fetch the modification time, it
> will force us to restart. Perhaps if we get an error we should return
> last_modified_time_of_exe_ instead (after logging something appropriate) to
> avoid spurious restarts?
> 
> Also, it seems to be preferred chrome style to omit braces from one-line
control
> statements. So:
> 
> if (something)
>     return false;
> 
> Both are technically correct, as long as you maintain consistency in each file
-
> since this file already is omitting braces, you should follow that style in
your
> new code also.

Done.

That boolean return was messed up - I'm surprised that the compiler didn't warn
me.

[Andrew T Wilson (Slow), 2010-04-21 23:50:41]

LGTM, but I've added Lei to the reviewer list, as I'd like someone more familiar
with the linux startup/update code to take a peek before we land this.

[Lei Zhang, 2010-04-22 00:57:53]

I'm trying out this patch. Meanwhile, I have the following nits.

http://codereview.chromium.org/1633021/diff/40001/41004
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/1633021/diff/40001/41004#newcode70
chrome/browser/browser_process_impl.cc:70: static const int
kUpdateCheckInvervalHours = 6;
typo, s/Inverval/Interval/

http://codereview.chromium.org/1633021/diff/40001/41004#newcode459
chrome/browser/browser_process_impl.cc:459: void
BrowserProcessImpl::StartAutoupdateTimer() {
Just wondering, would changing StartAutoupdateTimer() to
StartAutoupdateTimer(const int timeoutHours) make it easier to write a test for
this?

http://codereview.chromium.org/1633021/diff/40001/41003
File chrome/browser/first_run_gtk.cc (right):

http://codereview.chromium.org/1633021/diff/40001/41003#newcode104
chrome/browser/first_run_gtk.cc:104: return saved_last_modified_time_of_exe_ !=
Upgrade::GetLastModifiedTimeOfExe();
nit: 80 chars per line.

http://codereview.chromium.org/1633021/diff/40001/41003#newcode127
chrome/browser/first_run_gtk.cc:127: << exe_file_path.ToWStringHack();
exe_file_path.value();

[prasadt_google, 2010-04-22 01:05:44]

Thanks for the review.

http://codereview.chromium.org/1633021/diff/40001/41004
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/1633021/diff/40001/41004#newcode70
chrome/browser/browser_process_impl.cc:70: static const int
kUpdateCheckInvervalHours = 6;
On 2010/04/22 00:57:53, Lei Zhang wrote:
> typo, s/Inverval/Interval/

Done.

http://codereview.chromium.org/1633021/diff/40001/41004#newcode459
chrome/browser/browser_process_impl.cc:459: void
BrowserProcessImpl::StartAutoupdateTimer() {
On 2010/04/22 00:57:53, Lei Zhang wrote:
> Just wondering, would changing StartAutoupdateTimer() to
> StartAutoupdateTimer(const int timeoutHours) make it easier to write a test
for
> this?

I'm sure it will and there may be a few other changes needed too.  My next task
is to write unit tests for this feature.  And I'll do this change and others as
part of that.

http://codereview.chromium.org/1633021/diff/40001/41003
File chrome/browser/first_run_gtk.cc (right):

http://codereview.chromium.org/1633021/diff/40001/41003#newcode104
chrome/browser/first_run_gtk.cc:104: return saved_last_modified_time_of_exe_ !=
Upgrade::GetLastModifiedTimeOfExe();
On 2010/04/22 00:57:53, Lei Zhang wrote:
> nit: 80 chars per line.

Done.

http://codereview.chromium.org/1633021/diff/40001/41003#newcode127
chrome/browser/first_run_gtk.cc:127: << exe_file_path.ToWStringHack();
On 2010/04/22 00:57:53, Lei Zhang wrote:
> exe_file_path.value();

Done.

[Lei Zhang, 2010-04-22 01:20:37]

+matt

http://codereview.chromium.org/1633021/diff/10002/14004
File chrome/browser/first_run_gtk.cc (right):

http://codereview.chromium.org/1633021/diff/10002/14004#newcode115
chrome/browser/first_run_gtk.cc:115: return base::LaunchApp(command_line, false,
false, NULL);
I don't like this. Simultaneously launching Chrome while shutting it down is
prone to race conditions.

Using the test method described in the CL, I've gotten 3 different results so
far:

a) old browser shuts down first, new browser starts. Yay!
b) New browser detects old browser. I see: "Created new window in existing
browser session." on the console. Then since the browser didn't properly
restart, it still has the old saved time, and tries over and over again to
restart itself every 30 seconds.
c) New browser dies in process singleton code and never starts up:
ERROR:chrome/browser/process_singleton_linux.cc(174)] read() failed: Connection
reset by peer

[Andrew T Wilson (Slow), 2010-04-22 17:44:28]

On 2010/04/22 01:20:37, Lei Zhang wrote:
> I don't like this. Simultaneously launching Chrome while shutting it down is
> prone to race conditions.

Agreed - this was the trickiest part of landing the windows version of this
patch. I don't see any good way around it (and if there are race conditions
around starting new instances of Chrome while the old one is exiting, we should
shake them out anyway).

I think we need to make sure we're shutting down the process singleton (or at
least setting a flag so we don't respond to the RPC from the new session) before
firing off the new session.

Prasad, you were also seeing that failed assertion in step c) - what did you do
to address that (I dimly recall you were going to remove that assertion/change
the code to deal with it gracefully, since it's quite reasonable to have the
remote holder of the connection exit while you're trying to talk to it).

[prasadt_google, 2010-04-22 18:05:35]

On 2010/04/22 17:44:28, Andrew T Wilson wrote:
> On 2010/04/22 01:20:37, Lei Zhang wrote:
> > I don't like this. Simultaneously launching Chrome while shutting it down is
> > prone to race conditions.
> 
> Agreed - this was the trickiest part of landing the windows version of this
> patch. I don't see any good way around it (and if there are race conditions
> around starting new instances of Chrome while the old one is exiting, we
should
> shake them out anyway).
> 
> I think we need to make sure we're shutting down the process singleton (or at
> least setting a flag so we don't respond to the RPC from the new session)
before
> firing off the new session.
> 
> Prasad, you were also seeing that failed assertion in step c) - what did you
do
> to address that (I dimly recall you were going to remove that assertion/change
> the code to deal with it gracefully, since it's quite reasonable to have the
> remote holder of the connection exit while you're trying to talk to it).

I haven't seen either of the issues that Lei is seeing i.e his b) and c).  The
failed assertion I was seeing was in a different place and happens when the new
browser tries to kill the old instance and the old instance exits before the
kill gets there resulting in an ESRCH error from kill().  My fix was to look for
that specific error.  With that fix I couldn't repro it anymore.  When I did a
sync, I saw that someone checked in the same fix already.  This is in
process_singleton_linux.cc::KillProcessByLockPath().  Its also possible that I
picked up some new changes after the sync that I haven't exercised. 

Lei - Thanks for catching these issues.  I'll try this more rigorously on my
machine and see if I can repro your failures.

[Lei Zhang, 2010-04-22 20:58:45]

FWIW, I think the process singleton code mostly guards against races between two
chrome processes both starting up at the same time. It probably was not designed
for simultaneous shutdown + startup.

On my local machine, I can reproduce (c) fairly regularly. I haven't been able
to reproduce (b).

[prasadt_google, 2010-04-22 21:12:29]

On 2010/04/22 20:58:45, Lei Zhang wrote:
> FWIW, I think the process singleton code mostly guards against races between
two
> chrome processes both starting up at the same time. It probably was not
designed
> for simultaneous shutdown + startup.
> 
> On my local machine, I can reproduce (c) fairly regularly. I haven't been able
> to reproduce (b).

I've been trying but yet to repro either b) or c).  Following our off-line
conversation, I'm doing a sync to bring my sources more up to date to see if
that'll help repro the issue.

[prasadt_google, 2010-04-23 23:59:50]

On 2010/04/22 21:12:29, prasadt wrote:
> On 2010/04/22 20:58:45, Lei Zhang wrote:
> > FWIW, I think the process singleton code mostly guards against races between
> two
> > chrome processes both starting up at the same time. It probably was not
> designed
> > for simultaneous shutdown + startup.
> > 
> > On my local machine, I can reproduce (c) fairly regularly. I haven't been
able
> > to reproduce (b).
> 
> I've been trying but yet to repro either b) or c).  Following our off-line
> conversation, I'm doing a sync to bring my sources more up to date to see if 
> that'll help repro the issue.

I could never repro the issue but I did a general code walk through around start
up and how we're trying to restart the browser on update.  I noticed a few
issues.

Relaunching and then shutting down clearly seems backwards.  I tried reversing
the sequence.  That didn't much good because calling shutdown doesn't quite shut
the browser down rather posts events to the queue.  In the mean time the new
instance starts and we're back in the same situation.

I changed the code to call shutdown but remember that we need to restart.  And
when shutdown truly completes, that's when I restart.  I tested this and haven't
run into any issues (but then I didn't run into any before either).  But just
looking at the code, I don't see any possible timing issues with the new
approach.

Lei - Please take a look and give it a try.  I appreciate you trying this out
and identifying these tricky issues.

Drew - Please take a look too.

Thanks.

[Andrew T Wilson (Slow), 2010-04-24 00:53:31]

This change makes sense to me - I'll try it out locally to see if I'm able to
repro any issues on restart.

http://codereview.chromium.org/1633021/diff/38003/41016
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/1633021/diff/38003/41016#newcode1243
chrome/browser/browser_main.cc:1243:
Upgrade::RelaunchChromeBrowserWithNewCommandLine();
Maybe add "IfNeeded" to the end of the method name, to make it clear it doesn't
actually restart the browser unless necessary?

http://codereview.chromium.org/1633021/diff/38003/41014
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/1633021/diff/38003/41014#newcode543
chrome/browser/browser_process_impl.cc:543: scoped_ptr<CommandLine>
new_cl(new_cl_ptr);
Combine into one line?:

scoped_ptr<CommandLine> new_cl(new CommandLine(old_cl->GetProgram()));

http://codereview.chromium.org/1633021/diff/38003/41014#newcode574
chrome/browser/browser_process_impl.cc:574: new_cl.release();
Do this instead (combined with my previous suggestion to get rid of new_cl_ptr):

Upgrade::SetNewCommandLine(new_cl.release());

In fact, I'm not completely sure why we need a scoped_ptr anyway since nothing
can exit in this block of code to cause a leak, but I guess it's safer that way.

An alternative which might be better would be to just allocate this as a stack
variable, and make a copy in SetNewCommandLine() instead.

http://codereview.chromium.org/1633021/diff/38003/41015
File chrome/browser/first_run.h (right):

http://codereview.chromium.org/1633021/diff/38003/41015#newcode172
chrome/browser/first_run.h:172: // Combines the two methods, 
RelaunchChromeBrowser and
nit: remove extra space after the comma.

[Lei Zhang, 2010-04-24 03:50:43]

http://codereview.chromium.org/1633021/diff/38003/41016
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/1633021/diff/38003/41016#newcode1243
chrome/browser/browser_main.cc:1243:
Upgrade::RelaunchChromeBrowserWithNewCommandLine();
This is definitely a better place to relaunch the browser. We are actually not
finished yet though. We still have to return to ChromeMain, which has an
AtExitManager that has to run. It seems AtExitManager has to run all the dtors
for all the Singletons, and I don't know what that involves. Maybe it's slightly
safer to do this there, after the AtExitManager goes out of scope? AKA when we
are "really" finished.

[Andrew T Wilson (Slow), 2010-04-27 00:47:26]

FWIW, I ran with this patch (changed to poll for updates every 6 seconds instead
of 6 hours), and didn't see any restart problems after 100-or-so restarts. So
whatever problems were happening before don't seem to be reproducible on my
system with the latest patch.

[Lei Zhang, 2010-04-27 00:51:35]

On 2010/04/27 00:47:26, Andrew T Wilson wrote:
> FWIW, I ran with this patch (changed to poll for updates every 6 seconds
instead
> of 6 hours), and didn't see any restart problems after 100-or-so restarts. So
> whatever problems were happening before don't seem to be reproducible on my
> system with the latest patch.

Agreed. This patch is much better. I'm just worried the AtExitManager might
cause a subtle problem. So I'm encouraging Prasad to make the code do the
restart as late as possible.

[Andrew T Wilson (Slow), 2010-04-27 16:31:59]

On 2010/04/27 00:51:35, Lei Zhang wrote:
> Agreed. This patch is much better. I'm just worried the AtExitManager might
> cause a subtle problem. So I'm encouraging Prasad to make the code do the
> restart as late as possible.
No arguments here :) I just wanted to give him a data point because I promised
him last week (before our review) that I'd test this on my system. I agree that
we should make the change you suggested.

[prasadt_google, 2010-04-28 21:13:25]

Please note that I uploaded one too many times.  I thought it failed to upload
the first time...

http://codereview.chromium.org/1633021/diff/38003/41016
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/1633021/diff/38003/41016#newcode1243
chrome/browser/browser_main.cc:1243:
Upgrade::RelaunchChromeBrowserWithNewCommandLine();
On 2010/04/24 00:53:31, Andrew T Wilson wrote:
> Maybe add "IfNeeded" to the end of the method name, to make it clear it
doesn't
> actually restart the browser unless necessary?

Done.  Now that's one heck of a function name :-)

http://codereview.chromium.org/1633021/diff/38003/41016#newcode1243
chrome/browser/browser_main.cc:1243:
Upgrade::RelaunchChromeBrowserWithNewCommandLine();
On 2010/04/24 03:50:43, Lei Zhang wrote:
> This is definitely a better place to relaunch the browser. We are actually not
> finished yet though. We still have to return to ChromeMain, which has an
> AtExitManager that has to run. It seems AtExitManager has to run all the dtors
> for all the Singletons, and I don't know what that involves. Maybe it's
slightly
> safer to do this there, after the AtExitManager goes out of scope? AKA when we
> are "really" finished.

Done.  AtExitManager does cleanup in its destruction which runs when the object
goes out of scope at the end of ChromeMain.  So I have to do the relaunch from
the caller of Chrome which is main() for Linux and wWinMain for Windows.

http://codereview.chromium.org/1633021/diff/38003/41014
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/1633021/diff/38003/41014#newcode543
chrome/browser/browser_process_impl.cc:543: scoped_ptr<CommandLine>
new_cl(new_cl_ptr);
On 2010/04/24 00:53:31, Andrew T Wilson wrote:
> Combine into one line?:
> 
> scoped_ptr<CommandLine> new_cl(new CommandLine(old_cl->GetProgram()));

Done.

http://codereview.chromium.org/1633021/diff/38003/41014#newcode574
chrome/browser/browser_process_impl.cc:574: new_cl.release();
On 2010/04/24 00:53:31, Andrew T Wilson wrote:
> Do this instead (combined with my previous suggestion to get rid of
new_cl_ptr):
> 
> Upgrade::SetNewCommandLine(new_cl.release());
> 
> In fact, I'm not completely sure why we need a scoped_ptr anyway since nothing
> can exit in this block of code to cause a leak, but I guess it's safer that
way.
> 
> An alternative which might be better would be to just allocate this as a stack
> variable, and make a copy in SetNewCommandLine() instead.

Done.  I considered making a copy in the SetNewCommandLine() but there is no
copy constructor defined for CommandLine class.  And it seemed cleaner and more
importantly less fragile to take ownership of the pointer than to implement code
to make a copy of the object.

http://codereview.chromium.org/1633021/diff/38003/41015
File chrome/browser/first_run.h (right):

http://codereview.chromium.org/1633021/diff/38003/41015#newcode172
chrome/browser/first_run.h:172: // Combines the two methods, 
RelaunchChromeBrowser and
On 2010/04/24 00:53:31, Andrew T Wilson wrote:
> nit: remove extra space after the comma.

Done.

[Lei Zhang, 2010-04-29 00:47:30]

Patch set 10 looks ok, but it didn't compile on the Windows try bot.

[prasadt_google, 2010-04-29 01:48:24]

On 2010/04/29 00:47:30, Lei Zhang wrote:
> Patch set 10 looks ok, but it didn't compile on the Windows try bot.

Yup.  I'm working on fixing that.  The issue is that the Upgrade class is built
into a different dll than the exe on windows and hence the symbol is unresolved.
 The solution I'm working on is to put a wrapper around the relaunch function
and export it, and then use GetProcAddress to get the function pointer from the
exe and invoke it...

[prasadt_google, 2010-04-29 21:03:11]

On 2010/04/29 01:48:24, prasadt wrote:
> On 2010/04/29 00:47:30, Lei Zhang wrote:
> > Patch set 10 looks ok, but it didn't compile on the Windows try bot.
> 
> Yup.  I'm working on fixing that.  The issue is that the Upgrade class is
built
> into a different dll than the exe on windows and hence the symbol is
unresolved.
>  The solution I'm working on is to put a wrapper around the relaunch function
> and export it, and then use GetProcAddress to get the function pointer from
the
> exe and invoke it...

Done fixing the Windows issue.  Please take a look.

[Lei Zhang, 2010-04-29 23:27:29]

On 2010/04/29 21:03:11, prasadt wrote:
> Done fixing the Windows issue.  Please take a look.

LGTM

[Andrew T Wilson (Slow), 2010-04-29 23:37:00]

LGTM, with one tiny comment.

http://codereview.chromium.org/1633021/diff/73002/83014
File chrome/app/client_util.h (right):

http://codereview.chromium.org/1633021/diff/73002/83014#newcode33
chrome/app/client_util.h:33: void
RelaunchChromeBrowserWithNewCommandLineIfNeeded();
Add a comment describing what this does/when it's used.

[prasadt_google, 2010-04-30 00:48:53]

http://codereview.chromium.org/1633021/diff/73002/83014
File chrome/app/client_util.h (right):

http://codereview.chromium.org/1633021/diff/73002/83014#newcode33
chrome/app/client_util.h:33: void
RelaunchChromeBrowserWithNewCommandLineIfNeeded();
On 2010/04/29 23:37:00, Andrew T Wilson wrote:
> Add a comment describing what this does/when it's used.

Done.