Enable captive portal detection by default.

Enable captive portal detection by default, but keep it
disabled for most browser tests.

Also remove the option from about:flags.  It can still be
disabled by deselecting "use a web service to resolve
navigation errors".

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

[mmenke, 2012-07-24 14:24:08]

http://codereview.chromium.org/10795038/diff/3020/chrome/test/base/test_launc...
File chrome/test/base/test_launcher_utils.cc (right):

http://codereview.chromium.org/10795038/diff/3020/chrome/test/base/test_launc...
chrome/test/base/test_launcher_utils.cc:47: if
(!command_line->HasSwitch(switches::kCaptivePortalDetection)) {
This is necessary because this is called after SetUpCommandLine.

[cbentzel, 2012-07-25 16:15:36]

Sorry for the delay.

Could you remove the command line flags by just adding a class static about
whether it is enabled and change the default SetUpOnMainThread?

[mmenke1, 2012-07-25 16:28:54]

On 2012/07/25 16:15:36, cbentzel wrote:
> Sorry for the delay.
> 
> Could you remove the command line flags by just adding a class static about
> whether it is enabled and change the default SetUpOnMainThread?

And disable it in InProcessBrowserTest::SetUp (Since it needs to be enabled in
chrome.exe)?  Sure.

[cbentzel, 2012-07-25 16:30:33]

On Wed, Jul 25, 2012 at 12:28 PM, <mmenke@google.com> wrote:

> On 2012/07/25 16:15:36, cbentzel wrote:
>
>> Sorry for the delay.
>>
>
>  Could you remove the command line flags by just adding a class static
>> about
>> whether it is enabled and change the default SetUpOnMainThread?
>>
>
> And disable it in InProcessBrowserTest::SetUp (Since it needs to be
> enabled in
> chrome.exe)?  Sure.
>

Yeah, that's the ticket.

>
>
http://codereview.chromium.**org/10795038/<http://codereview.chromium.org/107...
>

[mmenke, 2012-07-26 15:36:01]

On 2012/07/25 16:30:33, cbentzel wrote:
> On Wed, Jul 25, 2012 at 12:28 PM, <mailto:mmenke@google.com> wrote:
> 
> > On 2012/07/25 16:15:36, cbentzel wrote:
> >
> >> Sorry for the delay.
> >>
> >
> >  Could you remove the command line flags by just adding a class static
> >> about
> >> whether it is enabled and change the default SetUpOnMainThread?
> >>
> >
> > And disable it in InProcessBrowserTest::SetUp (Since it needs to be
> > enabled in
> > chrome.exe)?  Sure.
> >
> 
> Yeah, that's the ticket.

Done.

[cbentzel, 2012-07-26 17:50:05]

LGTM

[commit-bot: I haz the power, 2012-07-26 17:53:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/10795038/32002

[commit-bot: I haz the power, 2012-07-26 17:53:58]

Presubmit check for 10795038-32002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/test
    chrome/app

Presubmit checks took 4.4s to calculate.

[mmenke, 2012-07-26 18:12:41]

[+jam]:  Please review chrome/test/ and chrome/common/.
[+cpu]:  Please review chrome/app/generated_resources.

Thanks!

[mmenke, 2012-07-26 18:58:29]

Oops.  Forgot to add cpu to the list of reviewers.

[+cpu]:  Please review chrome/app/generated_resources.

[jam, 2012-07-26 21:47:53]

lgtm

for future reference, please send changes in chrome to other owners, since i'm
boggled down with reviews in content. unless it's code that i wrote, which
should be rare.

http://codereview.chromium.org/10795038/diff/32002/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_service.h (right):

http://codereview.chromium.org/10795038/diff/32002/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_service.h:81: static void
set_is_disabled_for_tests(bool is_disabled_for_tests) {
nit: name this set_is_disabled_for_testing. there is a presubmit check that
ensures that *for_testing and ForTesting methods are only called by tests

[mmenke, 2012-07-26 22:08:27]

Thanks for the feedback.

I'll send reviews to other content owners in the future, as requested.  I should
have checked how many reviews are in your queue.

http://codereview.chromium.org/10795038/diff/32002/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_service.h (right):

http://codereview.chromium.org/10795038/diff/32002/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_service.h:81: static void
set_is_disabled_for_tests(bool is_disabled_for_tests) {
On 2012/07/26 21:47:53, John Abd-El-Malek wrote:
> nit: name this set_is_disabled_for_testing. there is a presubmit check that
> ensures that *for_testing and ForTesting methods are only called by tests

Done.

[cpu_(ooo_6.6-7.5), 2012-07-27 03:13:34]

grd piece lgtm.

[commit-bot: I haz the power, 2012-07-27 14:27:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/10795038/32002

[commit-bot: I haz the power, 2012-07-27 14:40:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/10795038/31024

[commit-bot: I haz the power, 2012-07-27 17:15:18]

Change committed as 148764