Don't pass the connector check policy flag along.

Don't pass the connector check policy flag along.

If a browser is running, and the service process is running the connector, the
connector will occasionally start up a chromium process to check the connector
enablement policy. The flag to do so would get passed to a running browser, if
present, and there is a path through the policy check code that could cause the
browser to quit.

This patch prevents the policy check flag from being passed on; the started
process will be very short lived and does a silent launch.

BUG=134252
TEST=None


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

[Scott Byer, 2012-06-22 23:28:28]

This closes a very small timing window that could cause the browser to quit. The
bulk of the patch is about getting a good, solid unit test for that portion of
the start up code working.

[Paweł Hajdan Jr., 2012-06-23 07:23:37]

http://codereview.chromium.org/10666010/diff/7001/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/10666010/diff/7001/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:1518: bool pass_command_line =
This #ifdef-mixed assignment is ugly. Please find a prettier way, e.g.

bool pass_command_line = true;

#if !defined(OS_MACOSX)
pass_command_line = !HasImportSwitch(...);
#endif

pass_command_line = pass_command_line && ...

http://codereview.chromium.org/10666010/diff/7001/chrome/browser/printing/clo...
File chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc
(right):

http://codereview.chromium.org/10666010/diff/7001/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:511:
TEST_F(CloudPrintProxyPolicyTest, CheckCommandLineCrossTalk) {
Aren't you mocking too much here? Would it make sense to write a browser test
instead? Alternatively, extract the logic under test so that you don't require
so many mocks. This test looks very brittle.

http://codereview.chromium.org/10666010/diff/7001/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:530: {
nit: When you use braces, indent the following lines.

http://codereview.chromium.org/10666010/diff/7001/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:594:
g_browser_process = original_browser_process;
We have AutoReset or something for that.

[Miranda Callahan, 2012-06-23 15:04:50]

OWNER LGTM for profiles/*.

On 2012/06/23 07:23:37, Paweł Hajdan Jr. wrote:
>
http://codereview.chromium.org/10666010/diff/7001/chrome/browser/chrome_brows...
> File chrome/browser/chrome_browser_main.cc (right):
> 
>
http://codereview.chromium.org/10666010/diff/7001/chrome/browser/chrome_brows...
> chrome/browser/chrome_browser_main.cc:1518: bool pass_command_line =
> This #ifdef-mixed assignment is ugly. Please find a prettier way, e.g.
> 
> bool pass_command_line = true;
> 
> #if !defined(OS_MACOSX)
> pass_command_line = !HasImportSwitch(...);
> #endif
> 
> pass_command_line = pass_command_line && ...
> 
>
http://codereview.chromium.org/10666010/diff/7001/chrome/browser/printing/clo...
> File chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc
> (right):
> 
>
http://codereview.chromium.org/10666010/diff/7001/chrome/browser/printing/clo...
> chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:511:
> TEST_F(CloudPrintProxyPolicyTest, CheckCommandLineCrossTalk) {
> Aren't you mocking too much here? Would it make sense to write a browser test
> instead? Alternatively, extract the logic under test so that you don't require
> so many mocks. This test looks very brittle.
> 
>
http://codereview.chromium.org/10666010/diff/7001/chrome/browser/printing/clo...
> chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:530:
{
> nit: When you use braces, indent the following lines.
> 
>
http://codereview.chromium.org/10666010/diff/7001/chrome/browser/printing/clo...
> chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:594:
> g_browser_process = original_browser_process;
> We have AutoReset or something for that.

[Albert Bodenhamer, 2012-06-27 16:35:33]

I just realized I hadn't sent this.

http://codereview.chromium.org/10666010/diff/7001/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/10666010/diff/7001/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:1519: #if !defined(OS_MACOSX)
This is a bit hard to parse.  perhaps something like:
bool pass_command_line = !parsed_command_line().HasSwitch();
#if <blah>
pass_command_line = pass_command_line && !HasImportSwitch()
#endif

[Scott Byer, 2012-06-27 19:29:06]

http://codereview.chromium.org/10666010/diff/7001/chrome/browser/printing/clo...
File chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc
(right):

http://codereview.chromium.org/10666010/diff/7001/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:511:
TEST_F(CloudPrintProxyPolicyTest, CheckCommandLineCrossTalk) {
On 2012/06/23 07:23:37, Paweł Hajdan Jr. wrote:
> Aren't you mocking too much here? Would it make sense to write a browser test
> instead? Alternatively, extract the logic under test so that you don't require
> so many mocks. This test looks very brittle.

Yes, yes I am. I started in and ended up "pot committed" on a mock-based unit
test. I think I know how to do a simpler browser test...

[Scott Byer, 2012-07-02 19:04:17]

PTAL; I like the browser test route, though it did require adding a new exit
code to the browser.

thestig@, needs owner approval.

[Albert Bodenhamer, 2012-07-09 04:09:39]

lgtm

http://codereview.chromium.org/10666010/diff/25001/chrome/browser/printing/cl...
File chrome/browser/printing/cloud_print/test/cloud_print_policy_browsertest.cc
(right):

http://codereview.chromium.org/10666010/diff/25001/chrome/browser/printing/cl...
chrome/browser/printing/cloud_print/test/cloud_print_policy_browsertest.cc:49:
TestTimeouts::action_timeout_ms());
Do we need action_timeout_ms or action_timeout here?

[Lei Zhang, 2012-07-09 22:40:04]

http://codereview.chromium.org/10666010/diff/25001/chrome/browser/printing/cl...
File chrome/browser/printing/cloud_print/test/cloud_print_policy_browsertest.cc
(right):

http://codereview.chromium.org/10666010/diff/25001/chrome/browser/printing/cl...
chrome/browser/printing/cloud_print/test/cloud_print_policy_browsertest.cc:20:
#if !defined(OS_MACOSX)
Can we just exclude this file on Mac in the .gypi file? Then you can change this
to:

#if defined(OS_MACOSX)
#error blah
#endif

http://codereview.chromium.org/10666010/diff/25001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/10666010/diff/25001/chrome/chrome_tests.gypi#n...
chrome/chrome_tests.gypi:2874:
'browser/printing/cloud_print/test/cloud_print_policy_browsertest.cc',
nit: policy before proxy.

[Scott Byer, 2012-07-10 16:23:11]

http://codereview.chromium.org/10666010/diff/25001/chrome/browser/printing/cl...
File chrome/browser/printing/cloud_print/test/cloud_print_policy_browsertest.cc
(right):

http://codereview.chromium.org/10666010/diff/25001/chrome/browser/printing/cl...
chrome/browser/printing/cloud_print/test/cloud_print_policy_browsertest.cc:20:
#if !defined(OS_MACOSX)
On 2012/07/09 22:40:04, Lei Zhang wrote:
> Can we just exclude this file on Mac in the .gypi file? Then you can change
this
> to:
> 
> #if defined(OS_MACOSX)
> #error blah
> #endif

Done.

http://codereview.chromium.org/10666010/diff/25001/chrome/browser/printing/cl...
chrome/browser/printing/cloud_print/test/cloud_print_policy_browsertest.cc:49:
TestTimeouts::action_timeout_ms());
On 2012/07/09 04:09:40, Albert Bodenhamer wrote:
> Do we need action_timeout_ms or action_timeout here?

action_timeout_ms. I don't like integer (sec/tick/ms) time APIs either. Wish it
were all double seconds.

http://codereview.chromium.org/10666010/diff/25001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/10666010/diff/25001/chrome/chrome_tests.gypi#n...
chrome/chrome_tests.gypi:2874:
'browser/printing/cloud_print/test/cloud_print_policy_browsertest.cc',
On 2012/07/09 22:40:04, Lei Zhang wrote:
> nit: policy before proxy.

Done. Could've sworn I did M-x sort-lines before uploading...

[Lei Zhang, 2012-07-10 18:24:17]

LGTM

minor nit: isn't EXPECT_EQ written as EXPECT_EQ(expected, actual) ?

[commit-bot: I haz the power, 2012-07-10 18:34:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottbyer@chromium.org/10666010/26004

[commit-bot: I haz the power, 2012-07-10 19:28:44]

Try job failure for 10666010-26004 (retry) (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-07-10 19:32:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottbyer@chromium.org/10666010/26004

[commit-bot: I haz the power, 2012-07-10 21:11:28]

Try job failure for 10666010-26004 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-07-10 21:13:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottbyer@chromium.org/10666010/26004

[commit-bot: I haz the power, 2012-07-10 23:02:11]

Try job failure for 10666010-26004 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-07-11 16:50:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottbyer@chromium.org/10666010/28005

[commit-bot: I haz the power, 2012-07-11 18:20:44]

Change committed as 146149