Extract locking behaviour from ProcessSingleton.

Extract locking behaviour from ProcessSingleton.

This refactoring continues the division of the behaviour of ProcessSingleton into two parts:
* The protocol for establishing a server process and communicating between the client and server.
* How the server processes command-line invocations.

Very small behavioural change:
* If an error occurs while parsing the command-line received via COPY_DATA, the modal dialog (if any) is no longer flashed and raised to foreground.

The motivation for this change is that I wish to introduce some more sophisticated behaviour when queuing messages during startup. See the follow-up CL (in-progress) at https://codereview.chromium.org/12674028/ .

BUG=170726, 170734, 225693
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195264

[erikwright (departed), 2013-03-27 02:21:40]

robert, gab: PTAL.

It's notable that removing all of this functionality from ProcessSingleton did
not break any tests.

This is CL 2/3.

[robertshield, 2013-03-27 14:51:37]

Change looks good. Would it be possible to add a test to validate the behaviour
when a modal dialog is up. 

Also, please update the description to describe the benefits of this refactoring
as it isn't immediately clear from the linked bug descriptions.

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/modal_dial...
File chrome/browser/modal_dialog_lock.h (right):

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/modal_dial...
chrome/browser/modal_dialog_lock.h:27: class ModalDialogLock {
Minor nit, but would prefer naming this with "ProcessSingleton" in the name to
make it immediately obvious to readers that this part of the ProcessSingleton
paraphernalia.

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
File chrome/browser/process_singleton_win.cc (left):

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
chrome/browser/process_singleton_win.cc:548:
SetForegroundWindow(foreground_window_);
This is at least a small change in behaviour in that the SetForegroundWindow()
call may no longer happen if e.g. ParseCommandLine fails. Please mention this in
the CL description.

[gab, 2013-03-27 18:03:26]

This is honestly the best piece of C++ code I have ever reviewed, extremely well
structured, very very nice work sir!

I feel I've learnt a lot from the possibilities offered by Callbacks through
reading this CL :).

I have been playing around process_singleton for a while already and hadn't
realized some of the subtleties it could do with bringing modal windows to the
foreground -- hadn't touch the "notify" side very much FWIW -- (we should use
this for the Win8 first run dialog); thanks for restructuring this code in such
a more readable fashion :)!

LGTM w/ some nits/questions :)!

Cheers!
Gab

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/first_run/...
File chrome/browser/first_run/try_chrome_dialog_view.cc (right):

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/first_run/...
chrome/browser/first_run/try_chrome_dialog_view.cc:302:
MessageLoop::current()->Run();
We shouldn't run the MessageLoop before the end of PreMainMessageLoopRun (and
this seems to be called before that... i.e., right after PreProfileInit()).

Would there be a way to CHECK if the MessageLoop is ever ran while the
process_singleton is locked? We could then fix all instances of this paradigm
and prevent further introductions of it.

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/modal_dial...
File chrome/browser/modal_dialog_lock.cc (right):

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/modal_dial...
chrome/browser/modal_dialog_lock.cc:24: #if !defined(OS_WIN) ||
defined(USE_AURA)
If this is only meant to be used on Windows, why not make this entire class
_win.cc?

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/modal_dial...
chrome/browser/modal_dialog_lock.cc:24: #if !defined(OS_WIN) ||
defined(USE_AURA)
Why is this a NOTREACHED() on win-Aura?

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/modal_dial...
chrome/browser/modal_dialog_lock.cc:25: NOTREACHED();
nit: indent two less spaces.

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/modal_dial...
File chrome/browser/modal_dialog_lock.h (right):

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/modal_dial...
chrome/browser/modal_dialog_lock.h:18: // Implements a
ProcessSingleton::NotificationCallback that prevents
I don't like the use of "Implements" here since it not a subclass of a
ProcessSingleton::NotificationCallback, how about "Serves"?

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
File chrome/browser/process_singleton_lock.cc (right):

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
chrome/browser/process_singleton_lock.cc:18:
ProcessSingletonLock::AsNotificationCallback() {
nit: indent 4 spaces

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
chrome/browser/process_singleton_lock.cc:34: replayed_messages.end())
nit: indent 4 more spaces.

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
File chrome/browser/process_singleton_lock.h (right):

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
chrome/browser/process_singleton_lock.h:18: // Implements a
ProcessSingleton::NotificationCallback that can queue up
Same comment here about "Implements".

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
chrome/browser/process_singleton_lock.h:24: class ProcessSingletonLock : public
base::NonThreadSafe {
Add to the documentation that ProcessSingletonLock is initialized in its locked
state.

[erikwright (departed), 2013-03-27 18:28:43]

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
File chrome/browser/process_singleton_lock.h (right):

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
chrome/browser/process_singleton_lock.h:24: class ProcessSingletonLock : public
base::NonThreadSafe {
On 2013/03/27 18:03:26, gab wrote:
> Add to the documentation that ProcessSingletonLock is initialized in its
locked
> state.

Is the comment on line 20 ("The object starts in a locked state.") sufficient or
does the fact that you overlooked it imply that it should be made a separate
paragraph (i.e., insert LF-LF after "completes." on line 20)?

[gab, 2013-03-27 18:33:20]

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
File chrome/browser/process_singleton_lock.h (right):

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
chrome/browser/process_singleton_lock.h:24: class ProcessSingletonLock : public
base::NonThreadSafe {
On 2013/03/27 18:28:44, erikwright wrote:
> On 2013/03/27 18:03:26, gab wrote:
> > Add to the documentation that ProcessSingletonLock is initialized in its
> locked
> > state.
> 
> Is the comment on line 20 ("The object starts in a locked state.") sufficient
or
> does the fact that you overlooked it imply that it should be made a separate
> paragraph (i.e., insert LF-LF after "completes." on line 20)?

Oh, I did overlook that, although I read the comment more than once... so yes I
guess having it in a new paragraph would help :).

[gab, 2013-03-27 19:49:08]

Oh, also, can you write part#/3 in each CL's description; it will be easier for
someone re-reading this later to see everything that went in to this :).

Cheers!
Gab

[erikwright (departed), 2013-03-28 03:16:34]

thakis: PTAL.

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/first_run/...
File chrome/browser/first_run/try_chrome_dialog_view.cc (right):

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/first_run/...
chrome/browser/first_run/try_chrome_dialog_view.cc:302:
MessageLoop::current()->Run();
On 2013/03/27 18:03:26, gab wrote:
> We shouldn't run the MessageLoop before the end of PreMainMessageLoopRun (and
> this seems to be called before that... i.e., right after PreProfileInit()).
> 
> Would there be a way to CHECK if the MessageLoop is ever ran while the
> process_singleton is locked? We could then fix all instances of this paradigm
> and prevent further introductions of it.

I'm sure a way could be found to do that, but keep in mind that, at the moment,
that would be a bad thing to do because there is existing code that does run the
message loop before the end of PreMainMessageLoopRun.

Note that this is another advantage of the SequencedTaskRunner API. It doesn't
expose Run(), so by eliminating the global static MessageLoop::current() you
would prevent people from calling MessageLoop::Run when you don't want them to.

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/modal_dial...
File chrome/browser/modal_dialog_lock.cc (right):

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/modal_dial...
chrome/browser/modal_dialog_lock.cc:24: #if !defined(OS_WIN) ||
defined(USE_AURA)
On 2013/03/27 18:03:26, gab wrote:
> If this is only meant to be used on Windows, why not make this entire class
> _win.cc?

I didn't want to force too may #ifdef's in the client code. In any case, it's
much less win-specific now, although it would fail if actually used because the
platform-specific SetForegroundWindow is not supplied yet.

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/modal_dial...
chrome/browser/modal_dialog_lock.cc:24: #if !defined(OS_WIN) ||
defined(USE_AURA)
On 2013/03/27 18:03:26, gab wrote:
> Why is this a NOTREACHED() on win-Aura?

It was never actually called and eventually we would fail because we don't have
a SetForegroundWindow equivalent. But I've reworked the failure mode a bit.

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/modal_dial...
chrome/browser/modal_dialog_lock.cc:25: NOTREACHED();
On 2013/03/27 18:03:26, gab wrote:
> nit: indent two less spaces.

n/a.

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/modal_dial...
File chrome/browser/modal_dialog_lock.h (right):

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/modal_dial...
chrome/browser/modal_dialog_lock.h:18: // Implements a
ProcessSingleton::NotificationCallback that prevents
On 2013/03/27 18:03:26, gab wrote:
> I don't like the use of "Implements" here since it not a subclass of a
> ProcessSingleton::NotificationCallback, how about "Serves"?

Changed to Provides.

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/modal_dial...
chrome/browser/modal_dialog_lock.h:27: class ModalDialogLock {
On 2013/03/27 14:51:38, robertshield wrote:
> Minor nit, but would prefer naming this with "ProcessSingleton" in the name to
> make it immediately obvious to readers that this part of the ProcessSingleton
> paraphernalia.

Done.

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
File chrome/browser/process_singleton_lock.cc (right):

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
chrome/browser/process_singleton_lock.cc:18:
ProcessSingletonLock::AsNotificationCallback() {
On 2013/03/27 18:03:26, gab wrote:
> nit: indent 4 spaces

I believe the current incarnation to be correct per style guide.

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
chrome/browser/process_singleton_lock.cc:34: replayed_messages.end())
On 2013/03/27 18:03:26, gab wrote:
> nit: indent 4 more spaces.

n/a.

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
File chrome/browser/process_singleton_lock.h (right):

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
chrome/browser/process_singleton_lock.h:18: // Implements a
ProcessSingleton::NotificationCallback that can queue up
On 2013/03/27 18:03:26, gab wrote:
> Same comment here about "Implements".

Done.

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
chrome/browser/process_singleton_lock.h:24: class ProcessSingletonLock : public
base::NonThreadSafe {
On 2013/03/27 18:33:20, gab wrote:
> On 2013/03/27 18:28:44, erikwright wrote:
> > On 2013/03/27 18:03:26, gab wrote:
> > > Add to the documentation that ProcessSingletonLock is initialized in its
> > locked
> > > state.
> > 
> > Is the comment on line 20 ("The object starts in a locked state.")
sufficient
> or
> > does the fact that you overlooked it imply that it should be made a separate
> > paragraph (i.e., insert LF-LF after "completes." on line 20)?
> 
> Oh, I did overlook that, although I read the comment more than once... so yes
I
> guess having it in a new paragraph would help :).

Done.

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
File chrome/browser/process_singleton_win.cc (left):

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
chrome/browser/process_singleton_win.cc:548:
SetForegroundWindow(foreground_window_);
On 2013/03/27 14:51:38, robertshield wrote:
> This is at least a small change in behaviour in that the SetForegroundWindow()
> call may no longer happen if e.g. ParseCommandLine fails. Please mention this
in
> the CL description.

Done.

[Nico, 2013-03-28 04:50:09]

I only skimmed. I'll take a real look tomorrow morning.

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:1355: DCHECK(startup_lock_.locked());
How bad is it if this fails? Should this be a CHECK instead?

[gab, 2013-03-28 13:45:53]

Additional comments on new patch set.

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:1355: DCHECK(startup_lock_.locked());
On 2013/03/28 04:50:09, Nico wrote:
> How bad is it if this fails? Should this be a CHECK instead?

The process singleton is already locked all the way until the end of
PostBrowserStart().

In fact the extra locking in AutoImport was wrong as the Lock() didn't have a
stacking behavior (and thus unlocking would fully unlock right at the end of
AutoImport; way too early -- this refactoring might in fact fix some
mysterious/undiagnosed/hard-to-repro rendez-vous crashes).

It's a good thing this extra locking/unlocking was removed, but I don't feel
replacing it by a DCHECK here has much value (otherwise might as well DCHECK for
the lock in every other part of the Chrome initialization...).

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
File chrome/browser/process_singleton_modal_dialog_lock.cc (right):

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_modal_dialog_lock.cc:18: #if !defined(OS_WIN)
|| defined(USE_AURA)
Are you splitting the method in two ifdef'd impls because of compile issues?

I thought gfx::NativeWindow was typedef'd to HWND on Win (non-Aura) anyways..?

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_modal_dialog_lock.cc:60:
set_foreground_window_handler_.Run(active_dialog_);
I don't really see a use-case for using a callback here, I feel calling
DoSetForegroundWindow() directly is clearer and avoids an unnecessary layer of
indirection.

I also don't like having two constructors only to support this unnecessary
use-case.

Consider removing the use of a callback here and calling DoSetForegroundWindow()
directly.

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
File chrome/browser/process_singleton_modal_dialog_lock.h (right):

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_modal_dialog_lock.h:22: // While a dialog is
active, the ProcessSingleton notification
Explain what "a dialog is active" means as far as this class is concerned (i.e.,
|active_dialog_| is set; it's far from monitoring all dialogs shown by Chrome).

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_modal_dialog_lock.h:23: // callback will handle
but ignore notifications (i.e., neither this process
will handle notifications by bringing the dialog to the foreground (ignoring the
provided command line).

?

I don't like that the important part (bringing to foreground) seems like a
detail in the current wording; I think it should be the main point and the fact
that the command line is ignored the "detail".

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_modal_dialog_lock.h:46: //
ProcessSingletonModalDialogLock instance.
Same comments on the comment here (ref. identical comment on
ProcessSingletonStartupLock).

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
File chrome/browser/process_singleton_startup_lock.cc (right):

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_startup_lock.cc:31:
saved_startup_messages_.begin();
See, this is what I mean by 4 spaces indent when breaking the previous line
before the end of the statement.
Why would it make sense here, but not if the operator is '==' (ref. one of our
recent argument).

To me it only makes sense to wrap to the same indent as the previous line if the
previous line ends with:
;
,
&&
||
(and I might be forgetting some clear statement ending operators)

Where as if the previous line ends with '==', '>', is the middle of a method
definition, etc. I feel 4 spaces indent makes sense if only to indicate to the
reader: "hey, we weren't done, this is the remainder of the previous line".

(couldn't find a clear statement of that in the style-guide though... :( )

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
File chrome/browser/process_singleton_startup_lock.h (right):

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_startup_lock.h:32: // Returns the callback that
should be supplied to ProcessSingleton.
I don't think this comment should state whom this callback should be passed to
(plus this is not even true in practice as you pass this callback to
ProcessSingletonModalDialogLock, not to ProcessSingleton).

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_startup_lock.h:34: //
ProcessSingletonStartupLock instance.
I think this "validity" part of the comment is superfluous/confusing as it makes
me feel like the caller should do something about it, but in the use-case this
covers the 2 locks and the ProcessSingleton have the same life span (i.e. the
same as ChromeBrowserMainParts --> forever...).

[erikwright (departed), 2013-04-04 01:39:50]

thakis, gab, robertshield: PTAL.

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:1355: DCHECK(startup_lock_.locked());
On 2013/03/28 13:45:53, gab wrote:
> On 2013/03/28 04:50:09, Nico wrote:
> > How bad is it if this fails? Should this be a CHECK instead?
> 
> The process singleton is already locked all the way until the end of
> PostBrowserStart().
> 
> In fact the extra locking in AutoImport was wrong as the Lock() didn't have a
> stacking behavior (and thus unlocking would fully unlock right at the end of
> AutoImport; way too early -- this refactoring might in fact fix some
> mysterious/undiagnosed/hard-to-repro rendez-vous crashes).
> 
> It's a good thing this extra locking/unlocking was removed, but I don't feel
> replacing it by a DCHECK here has much value (otherwise might as well DCHECK
for
> the lock in every other part of the Chrome initialization...).

I added this because I wanted to demonstrate that the code change is safe (i.e.,
that it's not necessary to call Lock() inside AutoImport()).

As gab says, you could argue for putting such a check all over the
initialization code, so I'll remove it.

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
File chrome/browser/process_singleton_modal_dialog_lock.cc (right):

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_modal_dialog_lock.cc:18: #if !defined(OS_WIN)
|| defined(USE_AURA)
On 2013/03/28 13:45:53, gab wrote:
> Are you splitting the method in two ifdef'd impls because of compile issues?
> 
> I thought gfx::NativeWindow was typedef'd to HWND on Win (non-Aura) anyways..?

On Windows, these two signatures are equivalent. On all other platforms, only
the first one will compile. So, yes, this is because we have to delegate to a
platform specific call (which we only actually use on Windows anyway, and for
which I don't have an equivalent on other platforms).

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_modal_dialog_lock.cc:60:
set_foreground_window_handler_.Run(active_dialog_);
On 2013/03/28 13:45:53, gab wrote:
> I don't really see a use-case for using a callback here, I feel calling
> DoSetForegroundWindow() directly is clearer and avoids an unnecessary layer of
> indirection.
> 
> I also don't like having two constructors only to support this unnecessary
> use-case.
> 
> Consider removing the use of a callback here and calling
DoSetForegroundWindow()
> directly.

This mechanism support unit-testing. Please see the unit-test in
process_singleton_unittest_win.cc .

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
File chrome/browser/process_singleton_modal_dialog_lock.h (right):

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_modal_dialog_lock.h:22: // While a dialog is
active, the ProcessSingleton notification
On 2013/03/28 13:45:53, gab wrote:
> Explain what "a dialog is active" means as far as this class is concerned
(i.e.,
> |active_dialog_| is set; it's far from monitoring all dialogs shown by
Chrome).

I've added "during startup". I think that the definition of a modal dialog being
active is pretty self-defined (modal meaning that it blocks interaction with the
app).

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_modal_dialog_lock.h:23: // callback will handle
but ignore notifications (i.e., neither this process
On 2013/03/28 13:45:53, gab wrote:
> will handle notifications by bringing the dialog to the foreground (ignoring
the
> provided command line).
> 
> ?
> 
> I don't like that the important part (bringing to foreground) seems like a
> detail in the current wording; I think it should be the main point and the
fact
> that the command line is ignored the "detail".

Reformatted.

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_modal_dialog_lock.h:46: //
ProcessSingletonModalDialogLock instance.
On 2013/03/28 13:45:53, gab wrote:
> Same comments on the comment here (ref. identical comment on
> ProcessSingletonStartupLock).

See my response there.

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
File chrome/browser/process_singleton_startup_lock.cc (right):

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_startup_lock.cc:31:
saved_startup_messages_.begin();
On 2013/03/28 13:45:53, gab wrote:
> See, this is what I mean by 4 spaces indent when breaking the previous line
> before the end of the statement.
> Why would it make sense here, but not if the operator is '==' (ref. one of our
> recent argument).
> 
> To me it only makes sense to wrap to the same indent as the previous line if
the
> previous line ends with:
> ;
> ,
> &&
> ||
> (and I might be forgetting some clear statement ending operators)
> 
> Where as if the previous line ends with '==', '>', is the middle of a method
> definition, etc. I feel 4 spaces indent makes sense if only to indicate to the
> reader: "hey, we weren't done, this is the remainder of the previous line".
> 
> (couldn't find a clear statement of that in the style-guide though... :( )

I honestly don't know. In the absence of anything else, though, I'll trust
google-c-style.el, which is presumably what many other developers are using and
therefore less likely to be constantly flip-flopping than me overriding its
default behaviour.

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
File chrome/browser/process_singleton_startup_lock.h (right):

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_startup_lock.h:32: // Returns the callback that
should be supplied to ProcessSingleton.
On 2013/03/28 13:45:53, gab wrote:
> I don't think this comment should state whom this callback should be passed to
> (plus this is not even true in practice as you pass this callback to
> ProcessSingletonModalDialogLock, not to ProcessSingleton).

Done.

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_startup_lock.h:34: //
ProcessSingletonStartupLock instance.
On 2013/03/28 13:45:53, gab wrote:
> I think this "validity" part of the comment is superfluous/confusing as it
makes
> me feel like the caller should do something about it, but in the use-case this
> covers the 2 locks and the ProcessSingleton have the same life span (i.e. the
> same as ChromeBrowserMainParts --> forever...).

The caller must be aware of this. It is an error to invoke the callback after
this object destroyed.

[gab, 2013-04-04 02:31:45]

re-lgtm w/ comments below.

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
File chrome/browser/process_singleton_modal_dialog_lock.cc (right):

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_modal_dialog_lock.cc:60:
set_foreground_window_handler_.Run(active_dialog_);
On 2013/04/04 01:39:51, erikwright wrote:
> On 2013/03/28 13:45:53, gab wrote:
> > I don't really see a use-case for using a callback here, I feel calling
> > DoSetForegroundWindow() directly is clearer and avoids an unnecessary layer
of
> > indirection.
> > 
> > I also don't like having two constructors only to support this unnecessary
> > use-case.
> > 
> > Consider removing the use of a callback here and calling
> DoSetForegroundWindow()
> > directly.
> 
> This mechanism support unit-testing. Please see the unit-test in
> process_singleton_unittest_win.cc .

Ah nice, had somehow missed the test.

QQ, why not make the test-only constructor private and make the test a friend of
this class?

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
File chrome/browser/process_singleton_modal_dialog_lock.h (right):

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_modal_dialog_lock.h:22: // While a dialog is
active, the ProcessSingleton notification
On 2013/04/04 01:39:51, erikwright wrote:
> On 2013/03/28 13:45:53, gab wrote:
> > Explain what "a dialog is active" means as far as this class is concerned
> (i.e.,
> > |active_dialog_| is set; it's far from monitoring all dialogs shown by
> Chrome).
> 
> I've added "during startup". I think that the definition of a modal dialog
being
> active is pretty self-defined (modal meaning that it blocks interaction with
the
> app).

Well that's better, but what I mean is that this comment makes it feel like this
class is globally aware of all modal dialogs active during startup which is not
true.

I would instead change it to something like "when |active_dialog_| is set." so
that readers explicitly know this class has to be notified somehow for this to
take action.

[grt (UTC plus 2), 2013-04-04 15:17:18]

drive-by, y'all

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
File chrome/browser/process_singleton_lock.cc (right):

https://codereview.chromium.org/12096114/diff/27001/chrome/browser/process_si...
chrome/browser/process_singleton_lock.cc:18:
ProcessSingletonLock::AsNotificationCallback() {
On 2013/03/28 03:16:34, erikwright wrote:
> On 2013/03/27 18:03:26, gab wrote:
> > nit: indent 4 spaces
> 
> I believe the current incarnation to be correct per style guide.

@gab's right: 4-space indent here; see
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/3SrDWEOwGIg/FdfIp...

[erikwright (departed), 2013-04-07 02:27:28]

thakis: Ping.

gab: PTAL.

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
File chrome/browser/process_singleton_modal_dialog_lock.cc (right):

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_modal_dialog_lock.cc:60:
set_foreground_window_handler_.Run(active_dialog_);
On 2013/04/04 02:31:45, gab wrote:
> On 2013/04/04 01:39:51, erikwright wrote:
> > On 2013/03/28 13:45:53, gab wrote:
> > > I don't really see a use-case for using a callback here, I feel calling
> > > DoSetForegroundWindow() directly is clearer and avoids an unnecessary
layer
> of
> > > indirection.
> > > 
> > > I also don't like having two constructors only to support this unnecessary
> > > use-case.
> > > 
> > > Consider removing the use of a callback here and calling
> > DoSetForegroundWindow()
> > > directly.
> > 
> > This mechanism support unit-testing. Please see the unit-test in
> > process_singleton_unittest_win.cc .
> 
> Ah nice, had somehow missed the test.
> 
> QQ, why not make the test-only constructor private and make the test a friend
of
> this class?

I find friend declarations harmful and distasteful. It opens the entire class up
in order to reveal a single function. On the other hand, there is basically zero
harm in exposing a constructor like this, especially given that this class is
analogous to a concrete implementation of a pure-virtual interface (i.e., the
end user only gets access to a single Callback, so the other public details of
the implementing class are not exposed).

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
File chrome/browser/process_singleton_modal_dialog_lock.h (right):

https://codereview.chromium.org/12096114/diff/66001/chrome/browser/process_si...
chrome/browser/process_singleton_modal_dialog_lock.h:22: // While a dialog is
active, the ProcessSingleton notification
On 2013/04/04 02:31:45, gab wrote:
> On 2013/04/04 01:39:51, erikwright wrote:
> > On 2013/03/28 13:45:53, gab wrote:
> > > Explain what "a dialog is active" means as far as this class is concerned
> > (i.e.,
> > > |active_dialog_| is set; it's far from monitoring all dialogs shown by
> > Chrome).
> > 
> > I've added "during startup". I think that the definition of a modal dialog
> being
> > active is pretty self-defined (modal meaning that it blocks interaction with
> the
> > app).
> 
> Well that's better, but what I mean is that this comment makes it feel like
this
> class is globally aware of all modal dialogs active during startup which is
not
> true.
> 
> I would instead change it to something like "when |active_dialog_| is set." so
> that readers explicitly know this class has to be notified somehow for this to
> take action.

OK, now your concern is clear to me. I added some content - WDYT?

[gab, 2013-04-09 14:34:29]

https://codereview.chromium.org/12096114/diff/99001/chrome/browser/process_si...
File chrome/browser/process_singleton_modal_dialog_lock.h (right):

https://codereview.chromium.org/12096114/diff/99001/chrome/browser/process_si...
chrome/browser/process_singleton_modal_dialog_lock.h:22: // dialogs are
displayed or dismissed.
s/when dialogs/when such dialogs
?
Since it's only for dialogs for which we want this lock to have an impact on?

[Nico, 2013-04-17 21:04:24]

Sorry for the delay.

https://codereview.chromium.org/12096114/diff/99001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.h (right):

https://codereview.chromium.org/12096114/diff/99001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.h:174: scoped_ptr<ProcessSingleton>
process_singleton_;
I started reviewing this a few times, and always got confused somewhere down the
road. I think my main problem is that I don't understand what all these locks
mean.

Can you add comments here that explain:
a) What each lock is for and when it's held
b) The locking hierarchy, i.e. the order these locks have to be obtained in
?

[erikwright (departed), 2013-04-18 16:14:11]

On 2013/04/17 21:04:24, Nico wrote:
> Sorry for the delay.
> 
>
https://codereview.chromium.org/12096114/diff/99001/chrome/browser/chrome_bro...
> File chrome/browser/chrome_browser_main.h (right):
> 
>
https://codereview.chromium.org/12096114/diff/99001/chrome/browser/chrome_bro...
> chrome/browser/chrome_browser_main.h:174: scoped_ptr<ProcessSingleton>
> process_singleton_;
> I started reviewing this a few times, and always got confused somewhere down
the
> road. I think my main problem is that I don't understand what all these locks
> mean.
> 
> Can you add comments here that explain:
> a) What each lock is for and when it's held
> b) The locking hierarchy, i.e. the order these locks have to be obtained in
> ?

I encapsulated all of the composition into chrome_process_singleton.{cc,h}.
Hopefully the isolation from the rest of chrome_browser_main plus the comments
will help clarify things.

[Nico, 2013-04-18 18:12:13]

I find this much easier to understand now, thanks.

I'm still confused though: The CL description says that this moves locking
functionality out of ProcessSingleton, but from what I understand the actual
locking bits (the code that creates a hidden dialog on win or the domain socket
on linux or the lockfile on mac) stay there and instead other code is moved into
two new classes whose names end in "Lock" but which aren't actually locks in the
concurrency sense. What am I missing?

https://codereview.chromium.org/12096114/diff/116001/chrome/browser/chrome_pr...
File chrome/browser/chrome_process_singleton.h (right):

https://codereview.chromium.org/12096114/diff/116001/chrome/browser/chrome_pr...
chrome/browser/chrome_process_singleton.h:59: void Unlock();
"// This only has an effect the first time it's called."

https://codereview.chromium.org/12096114/diff/116001/chrome/browser/first_run...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/12096114/diff/116001/chrome/browser/first_run...
chrome/browser/first_run/first_run.cc:680: #if !defined(USE_AURA)
Maybe add a comment like "// This function assumes that it's called from at most
one chrome process at once. The caller has to make sure to take the process lock
before calling this."

https://codereview.chromium.org/12096114/diff/116001/chrome/browser/process_s...
File chrome/browser/process_singleton_startup_lock.h (right):

https://codereview.chromium.org/12096114/diff/116001/chrome/browser/process_s...
chrome/browser/process_singleton_startup_lock.h:26: class
ProcessSingletonStartupLock : public base::NonThreadSafe {
I think part of my confusing was caused by this being named "Lock" but not being
a lock in the usual sense.

[erikwright (departed), 2013-04-18 18:15:52]

On 2013/04/18 18:12:13, Nico wrote:
> I find this much easier to understand now, thanks.
> 
> I'm still confused though: The CL description says that this moves locking
> functionality out of ProcessSingleton, but from what I understand the actual
> locking bits (the code that creates a hidden dialog on win or the domain
socket
> on linux or the lockfile on mac) stay there and instead other code is moved
into
> two new classes whose names end in "Lock" but which aren't actually locks in
the
> concurrency sense. What am I missing?
> 
>
https://codereview.chromium.org/12096114/diff/116001/chrome/browser/chrome_pr...
> File chrome/browser/chrome_process_singleton.h (right):
> 
>
https://codereview.chromium.org/12096114/diff/116001/chrome/browser/chrome_pr...
> chrome/browser/chrome_process_singleton.h:59: void Unlock();
> "// This only has an effect the first time it's called."
> 
>
https://codereview.chromium.org/12096114/diff/116001/chrome/browser/first_run...
> File chrome/browser/first_run/first_run.cc (right):
> 
>
https://codereview.chromium.org/12096114/diff/116001/chrome/browser/first_run...
> chrome/browser/first_run/first_run.cc:680: #if !defined(USE_AURA)
> Maybe add a comment like "// This function assumes that it's called from at
most
> one chrome process at once. The caller has to make sure to take the process
lock
> before calling this."
> 
>
https://codereview.chromium.org/12096114/diff/116001/chrome/browser/process_s...
> File chrome/browser/process_singleton_startup_lock.h (right):
> 
>
https://codereview.chromium.org/12096114/diff/116001/chrome/browser/process_s...
> chrome/browser/process_singleton_startup_lock.h:26: class
> ProcessSingletonStartupLock : public base::NonThreadSafe {
> I think part of my confusing was caused by this being named "Lock" but not
being
> a lock in the usual sense.

Perhaps the names should be changed. What is extracted is all of the
functionality that was previously implemented in ProcessSingleton::Lock/Unlock.

That functionality wasn't really locking in the concurrency sense, as you say.

The functionality that is extracted is related to blocking handling of
command-lines when modal dialogs are active as well as queuing incoming
command-lines when the process is not yet fully started.

I will endeavour to find better names for things, but hopefully you will
otherwise be able to understand the CL now.

[Nico, 2013-04-18 18:19:01]

Ok, then lgtm, thanks for explaining.

+1 on trying to find better names, but that can be done in a follow-up.

[erikwright (departed), 2013-04-18 18:23:54]

On 2013/04/18 18:19:01, Nico wrote:
> Ok, then lgtm, thanks for explaining.
> 
> +1 on trying to find better names, but that can be done in a follow-up.

Name suggestions:
- StartupCommandLineQueue: Queues incoming command-line invocations during
process startup.
- ProcessSingletonModalDialogWatcher: Drops incoming command-line invocations
when a modal dialog is active.

[gab, 2013-04-19 15:11:49]

still lgtm, drive-by on latest changes

https://codereview.chromium.org/12096114/diff/149001/chrome/browser/chrome_pr...
File chrome/browser/chrome_process_singleton_win_unittest.cc (right):

https://codereview.chromium.org/12096114/diff/149001/chrome/browser/chrome_pr...
chrome/browser/chrome_process_singleton_win_unittest.cc:67: ps2.Unlock();
ps2 shouldn't need to be unlocked to call NotifyOtherProcessOrCreate() (which is
all it does in this test).

I think this is a slightly better test if this also makes sure this works even
if ps2 is locked (which it is when this happens in chrome_browser_main.cc).

Or are you trying to make sure ClientCallback() doesn't get called?

(Same comment for other test in this file)

https://codereview.chromium.org/12096114/diff/149001/chrome/browser/process_s...
File chrome/browser/process_singleton_modal_dialog_lock.h (right):

https://codereview.chromium.org/12096114/diff/149001/chrome/browser/process_s...
chrome/browser/process_singleton_modal_dialog_lock.h:35: explicit
ProcessSingletonModalDialogLock(
nit: I feel an empty line between the typdefs and methods is more readable :)

[commit-bot: I haz the power, 2013-04-19 17:43:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/12096114/149001

[commit-bot: I haz the power, 2013-04-19 20:33:57]

Change committed as 195264