Move many ProcessSingleton methods to "protected" visibility as an upcoming refactoring of ProcessS…

Move many ProcessSingleton methods to "protected" visibility as an upcoming refactoring of ProcessSingleton on Windows requires that only NotifyOtherProcessOrCreate() be callable from the public interface.

This doesn't change any code logic, just moves things around.

BUG=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170805

[gab, 2012-11-30 21:12:49]

Sir, please take a look.

The code didn't actually changed, just moved around to match the new header
format.

Cheers,
Gab

[robertshield, 2012-11-30 21:21:52]

Should probably forward the parameters in the mock class methods in the test
file :)

https://codereview.chromium.org/11415237/diff/3001/chrome/browser/process_sin...
File chrome/browser/process_singleton.h (right):

https://codereview.chromium.org/11415237/diff/3001/chrome/browser/process_sin...
chrome/browser/process_singleton.h:118: // TODO(brettw): this will not handle
all cases. If two process start up too
nit: processes

https://codereview.chromium.org/11415237/diff/3001/chrome/browser/process_sin...
File chrome/browser/process_singleton_linux_unittest.cc (right):

https://codereview.chromium.org/11415237/diff/3001/chrome/browser/process_sin...
chrome/browser/process_singleton_linux_unittest.cc:56:
ProcessSingleton::NotifyOtherProcessWithTimeout();
might not build?

[gab, 2012-11-30 21:35:36]

Hehe :)

https://codereview.chromium.org/11415237/diff/3001/chrome/browser/process_sin...
File chrome/browser/process_singleton.h (right):

https://codereview.chromium.org/11415237/diff/3001/chrome/browser/process_sin...
chrome/browser/process_singleton.h:118: // TODO(brettw): this will not handle
all cases. If two process start up too
On 2012/11/30 21:21:52, robertshield wrote:
> nit: processes

Done.

https://codereview.chromium.org/11415237/diff/3001/chrome/browser/process_sin...
File chrome/browser/process_singleton_linux_unittest.cc (right):

https://codereview.chromium.org/11415237/diff/3001/chrome/browser/process_sin...
chrome/browser/process_singleton_linux_unittest.cc:56:
ProcessSingleton::NotifyOtherProcessWithTimeout();
On 2012/11/30 21:21:52, robertshield wrote:
> might not build?

Never heard of implicit parameter passing ;)?!

[robertshield, 2012-11-30 21:52:01]

The trybots look unfathomably miserable, but the change LGTM w/ one minor nit.

https://codereview.chromium.org/11415237/diff/5002/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11415237/diff/5002/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1050: // and has lauched chrome to show
the experiment UI.
nit: launched

[gab, 2012-11-30 23:46:15]

https://codereview.chromium.org/11415237/diff/5002/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11415237/diff/5002/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1050: // and has lauched chrome to show
the experiment UI.
On 2012/11/30 21:52:02, robertshield wrote:
> nit: launched

This is not actually part of the diff, based off the wrong branch (which
explains the massive bot failures!).

Fixed.

[gab, 2012-11-30 23:49:08]

@thakis for OWNER approval.

Thanks,
Gab

[Nico, 2012-12-01 01:45:39]

I wouldn't move the methods in the cc files around. Makes the method history
harder to look at, and results in just minimal gains.

https://codereview.chromium.org/11415237/diff/8009/chrome/browser/process_sin...
File chrome/browser/process_singleton_linux_unittest.cc (right):

https://codereview.chromium.org/11415237/diff/8009/chrome/browser/process_sin...
chrome/browser/process_singleton_linux_unittest.cc:43: class
MockProcessSingleton : ProcessSingleton {
nit: This isn't what a mock is.

https://codereview.chromium.org/11415237/diff/8009/chrome/browser/process_sin...
chrome/browser/process_singleton_linux_unittest.cc:47: }
It looks like all these methods just make superclass methods public. I think you
can do the same like this instead:

  public:
    using ProcessSingleton::NotifyOtherProcess();
    using /* …the other methods */

I think you can even omit the "ProcessSingleton::" bit.

[gab, 2012-12-02 18:41:00]

Addressed comments.

Decided to keep ProcessSingleton::Create() public after all as it makes sense to
be able to Create() without risking a NotifyOtherProcess() -- in particular in
tests (e.g. chrome_frame/test/net/fake_external_tab.cc).

PTAL.

Thanks,
Gab

https://codereview.chromium.org/11415237/diff/8009/chrome/browser/process_sin...
File chrome/browser/process_singleton_linux_unittest.cc (right):

https://codereview.chromium.org/11415237/diff/8009/chrome/browser/process_sin...
chrome/browser/process_singleton_linux_unittest.cc:43: class
MockProcessSingleton : ProcessSingleton {
On 2012/12/01 01:45:39, Nico wrote:
> nit: This isn't what a mock is.

Right, what about TestableProcessSingleton?

https://codereview.chromium.org/11415237/diff/8009/chrome/browser/process_sin...
chrome/browser/process_singleton_linux_unittest.cc:47: }
On 2012/12/01 01:45:39, Nico wrote:
> It looks like all these methods just make superclass methods public. I think
you
> can do the same like this instead:
> 
>   public:
>     using ProcessSingleton::NotifyOtherProcess();
>     using /* …the other methods */
> 
> I think you can even omit the "ProcessSingleton::" bit.

Oh neat :).

[Nico, 2012-12-02 20:16:56]

lgtm!

[gab, 2012-12-03 00:31:57]

Thanks,

@robertshield: FYI, your original "committer" LGTM is now an "owner" LGTM for
chrome_frame/, PTAL, I'll CQ anyways since it's only a comment addition.

Gab

[commit-bot: I haz the power, 2012-12-03 00:32:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/11415237/9003

[commit-bot: I haz the power, 2012-12-03 03:58:32]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-03 04:55:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/11415237/9003

[commit-bot: I haz the power, 2012-12-03 08:13:11]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-03 13:55:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/11415237/9003

[commit-bot: I haz the power, 2012-12-03 14:23:36]

Retried try job too often on mac_rel for step(s) ash_unittests

[commit-bot: I haz the power, 2012-12-03 16:22:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/11415237/9003

[commit-bot: I haz the power, 2012-12-03 17:16:59]

Retried try job too often on linux_rel for step(s) ash_unittests

[commit-bot: I haz the power, 2012-12-03 18:11:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/11415237/9003

[commit-bot: I haz the power, 2012-12-03 18:51:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/11415237/9003