Open a login tab on captive portal detection on SSL loads.

Captive portals intercept all HTTP requests until the user
has logged in, like at Starbucks and airports.  When behind
one, all SSL requests timeout after a potentially
substantial delay.

This CL Adds a CaptivePortalTabHelper which triggers captive
portal checks when an SSL load is taking too long.  If a
captive portal is found, opens a login tab.  Whenever the
new tab is navigated, we check again for a captive portal.
Once we discover the portal is gone, we reload the original
tab.

design doc:  https://docs.google.com/a/chromium.org/document/d/1k-gP2sswzYNvryu9NcgN7q5XrsMlUdlUdoW9WRaEmfM/edit

R=cbentzel@chromium.org,avi@chromium.org
BUG=87100, 115487
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142242

[mmenke, 2012-04-17 15:32:02]

Here's the other half of the CL, without browser tests yet (Though with a number
of unit tests).  Also doesn't have updated GYP files, since the other CL touches
them, and I use SVN.  PTAL when you get a chance, though prioritize the other
CL.

Thanks,
Matt

[cbentzel, 2012-04-17 21:28:08]

Will review by early tomorrow. Gave another round of comments on the
CapitvePortalService CL.

[cbentzel, 2012-04-18 14:36:31]

Why do you need both a CaptivePortalTabObserver and Helper?

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:271:
CaptivePortalServiceFactory::GetForProfile(profile_)->CheckForCaptivePortal();
Is this guaranteed to always exist? Probably safer to check for non-NULL in case
we decide not to do this for incognito profiles, for example.

[mmenke, 2012-04-18 14:38:52]

Mostly to keep the number of times when CaptivePortalService could broadcast
results down to a less insane number, for unit testing.

On 2012/04/18 14:36:31, cbentzel wrote:
> Why do you need both a CaptivePortalTabObserver and Helper?
> 
>
http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
> File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):
> 
>
http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
> chrome/browser/captive_portal/captive_portal_tab_helper.cc:271:
> CaptivePortalServiceFactory::GetForProfile(profile_)->CheckForCaptivePortal();
> Is this guaranteed to always exist? Probably safer to check for non-NULL in
case
> we decide not to do this for incognito profiles, for example.

[mmenke, 2012-04-18 14:40:41]

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:271:
CaptivePortalServiceFactory::GetForProfile(profile_)->CheckForCaptivePortal();
On 2012/04/18 14:36:31, cbentzel wrote:
> Is this guaranteed to always exist? Probably safer to check for non-NULL in
case
> we decide not to do this for incognito profiles, for example.

Yes, it's guaranteed to exist.  I'll go ahead and add a NULL check, though I do
currently have an incognito profile browser test.

[cbentzel, 2012-04-18 15:12:48]

This is a more complex state machine than I initially envisioned - and you
aren't even handling HTTP redirects at this point.

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:25: // The time we
wait for a slow loading SSL page before trigginer a captive
Nit: trigginer

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:107:
MessageLoop::current()->PostTask(
I'm a little concerned here about doing unnecessary reloads - for example, we
started the https load, logged in correctly elsewhere, then it lets bytes
through, and then we decide to reload even though it wasn't needed.

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:181:
SetState(STATE_NONE);
Should this politely close itself, or are there concerns about the user browsing
further in the same tab after the login page?

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:202:
slow_ssl_load_timer_.Stop();
Do we only need to do this if state_ == STATE_TIMER_RUNNING at this point?

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:203: 
Would it make sense to add DCHECKs here about valid state transitions?

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:239:
observer_.web_contents()->GetController().Reload(true);
Do you want to have the user manually reload if there is post data? What you
have will bring up the repost warning, but I don't know if that would be
considered annoying. Also, is the repost warning tab-constrained or
browser-constrained?

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:242: void
CaptivePortalTabHelper::MaybeOpenCaptivePortalLoginTab() {
I'm wondering if this should move to the service in case there are multiple
signals to trigger detection of captive portals. Or do you anticipate that this
will be the only one to open tabs?

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
File chrome/browser/captive_portal/captive_portal_tab_helper.h (right):

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.h:57: static
CaptivePortalTabHelper* CaptivePortalTabHelper::Create(
Curious - why do you need a factory function instead of making the constructor
public?

Also, this should return a scoped_ptr<CaptivePortalTabHelper> which can transfer
ownership.

[cbentzel, 2012-04-18 15:16:04]

On Wed, Apr 18, 2012 at 11:12 AM, <cbentzel@chromium.org> wrote:

> This is a more complex state machine than I initially envisioned - and you
> aren't even handling HTTP redirects at this point.


And to be clear - I think the state machine needs to be this complex to
handle user expectations for behavior.

>
>
>
> http://codereview.chromium.**org/10020051/diff/3005/chrome/**
>
browser/captive_portal/**captive_portal_tab_helper.cc<http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_portal/captive_portal_tab_helper.cc>
> File chrome/browser/captive_portal/**captive_portal_tab_helper.cc (right):
>
> http://codereview.chromium.**org/10020051/diff/3005/chrome/**
>
browser/captive_portal/**captive_portal_tab_helper.cc#**newcode25<http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_portal/captive_portal_tab_helper.cc#newcode25>
> chrome/browser/captive_portal/**captive_portal_tab_helper.cc:**25: // The
> time we wait for a slow loading SSL page before trigginer a captive
> Nit: trigginer
>
> http://codereview.chromium.**org/10020051/diff/3005/chrome/**
>
browser/captive_portal/**captive_portal_tab_helper.cc#**newcode107<http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_portal/captive_portal_tab_helper.cc#newcode107>
> chrome/browser/captive_portal/**captive_portal_tab_helper.cc:**107:
> MessageLoop::current()->**PostTask(
> I'm a little concerned here about doing unnecessary reloads - for
> example, we started the https load, logged in correctly elsewhere, then
> it lets bytes through, and then we decide to reload even though it
> wasn't needed.
>
> http://codereview.chromium.**org/10020051/diff/3005/chrome/**
>
browser/captive_portal/**captive_portal_tab_helper.cc#**newcode181<http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_portal/captive_portal_tab_helper.cc#newcode181>
> chrome/browser/captive_portal/**captive_portal_tab_helper.cc:**181:
> SetState(STATE_NONE);
> Should this politely close itself, or are there concerns about the user
> browsing further in the same tab after the login page?
>
> http://codereview.chromium.**org/10020051/diff/3005/chrome/**
>
browser/captive_portal/**captive_portal_tab_helper.cc#**newcode202<http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_portal/captive_portal_tab_helper.cc#newcode202>
> chrome/browser/captive_portal/**captive_portal_tab_helper.cc:**202:
> slow_ssl_load_timer_.Stop();
> Do we only need to do this if state_ == STATE_TIMER_RUNNING at this
> point?
>
> http://codereview.chromium.**org/10020051/diff/3005/chrome/**
>
browser/captive_portal/**captive_portal_tab_helper.cc#**newcode203<http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_portal/captive_portal_tab_helper.cc#newcode203>
> chrome/browser/captive_portal/**captive_portal_tab_helper.cc:**203:
> Would it make sense to add DCHECKs here about valid state transitions?
>
> http://codereview.chromium.**org/10020051/diff/3005/chrome/**
>
browser/captive_portal/**captive_portal_tab_helper.cc#**newcode239<http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_portal/captive_portal_tab_helper.cc#newcode239>
> chrome/browser/captive_portal/**captive_portal_tab_helper.cc:**239:
> observer_.web_contents()->**GetController().Reload(true);
> Do you want to have the user manually reload if there is post data? What
> you have will bring up the repost warning, but I don't know if that
> would be considered annoying. Also, is the repost warning
> tab-constrained or browser-constrained?
>
> http://codereview.chromium.**org/10020051/diff/3005/chrome/**
>
browser/captive_portal/**captive_portal_tab_helper.cc#**newcode242<http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_portal/captive_portal_tab_helper.cc#newcode242>
> chrome/browser/captive_portal/**captive_portal_tab_helper.cc:**242: void
> CaptivePortalTabHelper::**MaybeOpenCaptivePortalLoginTab**() {
> I'm wondering if this should move to the service in case there are
> multiple signals to trigger detection of captive portals. Or do you
> anticipate that this will be the only one to open tabs?
>
> http://codereview.chromium.**org/10020051/diff/3005/chrome/**
>
browser/captive_portal/**captive_portal_tab_helper.h<http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_portal/captive_portal_tab_helper.h>
> File chrome/browser/captive_portal/**captive_portal_tab_helper.h (right):
>
> http://codereview.chromium.**org/10020051/diff/3005/chrome/**
>
browser/captive_portal/**captive_portal_tab_helper.h#**newcode57<http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_portal/captive_portal_tab_helper.h#newcode57>
> chrome/browser/captive_portal/**captive_portal_tab_helper.h:**57: static
> CaptivePortalTabHelper* CaptivePortalTabHelper::**Create(
> Curious - why do you need a factory function instead of making the
> constructor public?
>
> Also, this should return a scoped_ptr<**CaptivePortalTabHelper> which can
> transfer ownership.
>
>
http://codereview.chromium.**org/10020051/<http://codereview.chromium.org/100...
>

[cbentzel, 2012-04-18 15:57:23]

I like the tests. Looks like you are getting pretty good coverage.

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:27: static int
kDefaultSlowSSLTime = 30;
Nit: kDefaultSlowSSLTimeSeconds since this does not have explicit units.

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:38:
tab_contents->profile(),
Nit: is this correct indent? Should just be 4 from the "return". Alternately,
looks like you might be able to fit on one line.

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:265:
content::PAGE_TRANSITION_TYPED);
Should we add a new page transition type here? Do we want this to show up in
browsing history?

http://codereview.chromium.org/10020051/diff/14002/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc
(right):

http://codereview.chromium.org/10020051/diff/14002/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:31: void
SetState(State state) {
Can you add a comment about why you need to alias these methods?

http://codereview.chromium.org/10020051/diff/14002/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:63:
MOCK_METHOD0(CheckForCaptivePortal, void());
I've been hesitant about using gmock, but this is a nice way to test this
without having to create a fake CaptivePortalTabService and injecting it in.

http://codereview.chromium.org/10020051/diff/14002/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:99: Profile*
fake_profile() { return reinterpret_cast<Profile*>(17); }
That is a bit of an unusual pattern. Hope the memory bots don't worry about it.
Why not just instantiate a TestingProfile or similar?

http://codereview.chromium.org/10020051/diff/14002/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:116:
MessageLoop::current()->RunAllPending();
Is there any potential for flakiness here, or is the TimeDelta() for the SSL
guaranteed to always be handled during this loop?

http://codereview.chromium.org/10020051/diff/14002/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:327: //
Simulate the user logging in while the timer is still running.  May happen
It doesn't look like there is a test for when the captive portal is detected
while the Timer is running. You could imagine this happen if the user opened one
tab and navigated to https://foo.com which times out and triggers the
CheckForCaptivePortal. Before we get a response, a second tab is opened to
https://bar.com.

[mmenke, 2012-04-18 19:15:46]

Suggest you hold off on re-reviewing this half of the captive portal stuff until
I've uploaded the browser tests.

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:25: // The time we
wait for a slow loading SSL page before trigginer a captive
On 2012/04/18 15:12:48, cbentzel wrote:
> Nit: trigginer

Fixed.

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:27: static int
kDefaultSlowSSLTime = 30;
On 2012/04/18 15:57:23, cbentzel wrote:
> Nit: kDefaultSlowSSLTimeSeconds since this does not have explicit units.

Done.

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:38:
tab_contents->profile(),
On 2012/04/18 15:57:23, cbentzel wrote:
> Nit: is this correct indent? Should just be 4 from the "return". Alternately,
> looks like you might be able to fit on one line.

Removed the function.

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:107:
MessageLoop::current()->PostTask(
On 2012/04/18 15:12:48, cbentzel wrote:
> I'm a little concerned here about doing unnecessary reloads - for example, we
> started the https load, logged in correctly elsewhere, then it lets bytes
> through, and then we decide to reload even though it wasn't needed. 

We only get here when we commit with a connection timed out error page (See line
87 in this CL/80 in the most recent CL), so I don't think there's an issue.

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:181:
SetState(STATE_NONE);
On 2012/04/18 15:12:48, cbentzel wrote:
> Should this politely close itself, or are there concerns about the user
browsing
> further in the same tab after the login page?

The user could have logged in from another tab, so I'm a bit paranoid about
automatically closing tabs.  Definitely worth considering in the future, but I'd
like to keep it as-is, and make sure things are working, before adding behaviors
that increase the likelihood of causing problems.

There's also the multi-profile case, which I'm a bit concerned about, since if
we somehow end up openning login tabs for multiple profiles, we only end up
closing the login tabs for one of them here, and the user could be trying to
navigate to a bookmark or some other site or something with the login tab.  How
likely this is, I have no idea.

Clearing the LOGIN_PAGE on browser-process initiated navigations may circumvent
these issues, though if the user navigated to an SSL page again that then times
out, we'd open and focus another tab, which could be a little annoying.

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:202:
slow_ssl_load_timer_.Stop();
On 2012/04/18 15:12:48, cbentzel wrote:
> Do we only need to do this if state_ == STATE_TIMER_RUNNING at this point?

Done.

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:203: 
On 2012/04/18 15:12:48, cbentzel wrote:
> Would it make sense to add DCHECKs here about valid state transitions?

Done.  Valid transitions are:

From anything to NONE (This is also the only self-to-self transition that can
occur)
From NONE to TIMER_RUNNING or LOGIN_PAGE.

For the states:
TIMER_RUNNING,
MAYBE_BROKEN,
BROKEN,
NEEDS_RELOAD,

Each state can transition to any of the later states, except TIMER_RUNNING can't
transition to BROKEN.  Adding a transition there could actually save us an extra
captive portal check in some cases, but I kept it out since we may not even have
a slow tab).

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:239:
observer_.web_contents()->GetController().Reload(true);
On 2012/04/18 15:12:48, cbentzel wrote:
> Do you want to have the user manually reload if there is post data? What you
> have will bring up the repost warning, but I don't know if that would be
> considered annoying. Also, is the repost warning tab-constrained or
> browser-constrained?

Since we failed to commit, we'll always reload the page with a GET.  Hmm...At
least if it's a new navigation.  Need to double-check what happens if the
original page was in the history.  I believe the same thing will be true.

That having been said, I've added a check so we don't reload posts.

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:242: void
CaptivePortalTabHelper::MaybeOpenCaptivePortalLoginTab() {
On 2012/04/18 15:12:48, cbentzel wrote:
> I'm wondering if this should move to the service in case there are multiple
> signals to trigger detection of captive portals. Or do you anticipate that
this
> will be the only one to open tabs? 

I've gone back and forth on this.  Currently, the captive portal service does
not depend on the TabHelpers, or even the browser process.  As a result, the
CaptivePortalService should work on Android, if we chose to compile it there,
but this function would not.  It's here primarily because of that.

That having been said, we aren't even compiling the service on Android, so I
think it's reasonable to either put this in the service, or give the service a
function to call this directly (Since we need to call SetState).

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:265:
content::PAGE_TRANSITION_TYPED);
On 2012/04/18 15:57:23, cbentzel wrote:
> Should we add a new page transition type here? Do we want this to show up in
> browsing history?

It would be great if the page didn't show up in the history.  I want to get a
better understanding of just how these things work first, however.

I suggest saving that for another CL, since it will involve touching some other
parts of the code.

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
File chrome/browser/captive_portal/captive_portal_tab_helper.h (right):

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.h:57: static
CaptivePortalTabHelper* CaptivePortalTabHelper::Create(
On 2012/04/18 15:12:48, cbentzel wrote:
> Curious - why do you need a factory function instead of making the constructor
> public?
> 
> Also, this should return a scoped_ptr<CaptivePortalTabHelper> which can
transfer
> ownership.

I used to take in a CaptivePortalService as well, and didn't want to expose that
to the TabContentsWrapper, but wanted to use NULLs for unit tests.  Now that I
don't, it's completely unnecessary.  Removed.

http://codereview.chromium.org/10020051/diff/14002/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc
(right):

http://codereview.chromium.org/10020051/diff/14002/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:31: void
SetState(State state) {
On 2012/04/18 15:57:23, cbentzel wrote:
> Can you add a comment about why you need to alias these methods?

Done.

http://codereview.chromium.org/10020051/diff/14002/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:63:
MOCK_METHOD0(CheckForCaptivePortal, void());
On 2012/04/18 15:57:23, cbentzel wrote:
> I've been hesitant about using gmock, but this is a nice way to test this
> without having to create a fake CaptivePortalTabService and injecting it in.

I haven't used it in the past, but simonjam's used it quite heavily in the
HttpPipelining tests, which has gotten me familiar with some cases in which they
can simplify life.

http://codereview.chromium.org/10020051/diff/14002/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:99: Profile*
fake_profile() { return reinterpret_cast<Profile*>(17); }
On 2012/04/18 15:57:23, cbentzel wrote:
> That is a bit of an unusual pattern. Hope the memory bots don't worry about
it.
> Why not just instantiate a TestingProfile or similar?

Because the profile shouldn't be dereferenced.  If we end up deferencing it
unexpectedly, we're suddenly depending on something we shouldn't be.  It also
increases how long the test takes by a factor of 60, though admittedly, the
tests are still fast.

I believe I've seen this done before, but can't remember in what test.  The
downside of this approach is we crash if it fails, so obviously not something to
do in a test that's likely to become flaky.  These tests never enter a message
wait, or depend on out of order events, so I think we're fairly safe.

Since it's never dereferenced, but only used as an index in a map, I don't think
the memory bots will care, unless they tract the C++ type of all variables in
the code as it runs, which I don't believe they do.

That having been said, if you think this is a bad idea, or would just prefer me
to use a TestingProfile, I'm happy to do so.

http://codereview.chromium.org/10020051/diff/14002/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:116:
MessageLoop::current()->RunAllPending();
On 2012/04/18 15:57:23, cbentzel wrote:
> Is there any potential for flakiness here, or is the TimeDelta() for the SSL
> guaranteed to always be handled during this loop?

No.  PostDelayedTask and PostTask are both internally mapped to the same code,
PostTask just uses a delay of 0, while PostDelayedTask uses whatever value is
passed it, in milliseconds rounded up.

So, assuming it's safe to do with PostTask, it's safe to do with PostDelayedTask
with a delay of 0 as well.

http://codereview.chromium.org/10020051/diff/14002/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:327: //
Simulate the user logging in while the timer is still running.  May happen
On 2012/04/18 15:57:23, cbentzel wrote:
> It doesn't look like there is a test for when the captive portal is detected
> while the Timer is running. You could imagine this happen if the user opened
one
> tab and navigated to https://foo.com which times out and triggers the
> CheckForCaptivePortal. Before we get a response, a second tab is opened to
> https://bar.com.

Test added for that case.

[cbentzel, 2012-04-18 21:12:37]

I'll hold off a final review until browser_tests are in. But I don't see any
problems with the way this is architected or the behavior it has.

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:242: void
CaptivePortalTabHelper::MaybeOpenCaptivePortalLoginTab() {
On 2012/04/18 19:15:46, Matt Menke wrote:
> On 2012/04/18 15:12:48, cbentzel wrote:
> > I'm wondering if this should move to the service in case there are multiple
> > signals to trigger detection of captive portals. Or do you anticipate that
> this
> > will be the only one to open tabs? 
> 
> I've gone back and forth on this.  Currently, the captive portal service does
> not depend on the TabHelpers, or even the browser process.  As a result, the
> CaptivePortalService should work on Android, if we chose to compile it there,
> but this function would not.  It's here primarily because of that.
> 
> That having been said, we aren't even compiling the service on Android, so I
> think it's reasonable to either put this in the service, or give the service a
> function to call this directly (Since we need to call SetState).

That's a good argument for why this is here.

http://codereview.chromium.org/10020051/diff/3005/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:265:
content::PAGE_TRANSITION_TYPED);
On 2012/04/18 19:15:46, Matt Menke wrote:
> On 2012/04/18 15:57:23, cbentzel wrote:
> > Should we add a new page transition type here? Do we want this to show up in
> > browsing history?
> 
> It would be great if the page didn't show up in the history.  I want to get a
> better understanding of just how these things work first, however.
> 
> I suggest saving that for another CL, since it will involve touching some
other
> parts of the code.

OK.

http://codereview.chromium.org/10020051/diff/14002/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc
(right):

http://codereview.chromium.org/10020051/diff/14002/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:99: Profile*
fake_profile() { return reinterpret_cast<Profile*>(17); }
On 2012/04/18 19:15:46, Matt Menke wrote:
> On 2012/04/18 15:57:23, cbentzel wrote:
> > That is a bit of an unusual pattern. Hope the memory bots don't worry about
> it.
> > Why not just instantiate a TestingProfile or similar?
> 
> Because the profile shouldn't be dereferenced.  If we end up deferencing it
> unexpectedly, we're suddenly depending on something we shouldn't be.  It also
> increases how long the test takes by a factor of 60, though admittedly, the
> tests are still fast.
> 
> I believe I've seen this done before, but can't remember in what test.  The
> downside of this approach is we crash if it fails, so obviously not something
to
> do in a test that's likely to become flaky.  These tests never enter a message
> wait, or depend on out of order events, so I think we're fairly safe.
> 
> Since it's never dereferenced, but only used as an index in a map, I don't
think
> the memory bots will care, unless they tract the C++ type of all variables in
> the code as it runs, which I don't believe they do.
> 
> That having been said, if you think this is a bad idea, or would just prefer
me
> to use a TestingProfile, I'm happy to do so.

You may want to ask Qin about this. I agree that it shouldn't be dereferenced,
and this will help catch that.

[mmenke, 2012-04-19 15:26:25]

http://codereview.chromium.org/10020051/diff/14002/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc
(right):

http://codereview.chromium.org/10020051/diff/14002/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:99: Profile*
fake_profile() { return reinterpret_cast<Profile*>(17); }
On 2012/04/18 21:12:37, cbentzel wrote:
> On 2012/04/18 19:15:46, Matt Menke wrote:
> > On 2012/04/18 15:57:23, cbentzel wrote:
> > > That is a bit of an unusual pattern. Hope the memory bots don't worry
about
> > it.
> > > Why not just instantiate a TestingProfile or similar?
> > 
> > Because the profile shouldn't be dereferenced.  If we end up deferencing it
> > unexpectedly, we're suddenly depending on something we shouldn't be.  It
also
> > increases how long the test takes by a factor of 60, though admittedly, the
> > tests are still fast.
> > 
> > I believe I've seen this done before, but can't remember in what test.  The
> > downside of this approach is we crash if it fails, so obviously not
something
> to
> > do in a test that's likely to become flaky.  These tests never enter a
message
> > wait, or depend on out of order events, so I think we're fairly safe.
> > 
> > Since it's never dereferenced, but only used as an index in a map, I don't
> think
> > the memory bots will care, unless they tract the C++ type of all variables
in
> > the code as it runs, which I don't believe they do.
> > 
> > That having been said, if you think this is a bad idea, or would just prefer
> me
> > to use a TestingProfile, I'm happy to do so.
> 
> You may want to ask Qin about this. I agree that it shouldn't be dereferenced,
> and this will help catch that.

Qin's response:

1. for the good behavior, i.e. if the junk pointer is never de-referenced I
don't see why it would be a problem. 
2. for abnormal behavior, i.e. if it's deliberately de-referenced and the fault
is masked then the tools will report errors that will have to be suppressed (or
annotated away), which might have some performance hit, depend how frequently it
happens.
   One thing to be careful is that if the junk value is chosen poorly, maybe it
could anchor a leak and cause a false negative (or primary to possible
downgrade) but that seems unlikely.


So I think we should be fine.

[cbentzel, 2012-05-03 19:38:26]

Is this ready for more of a review now that the CaptivePortalService is pretty
close to landing? Or is it something which will be changed quite a bit?

[mmenke, 2012-05-03 21:55:47]

On 2012/05/03 19:38:26, cbentzel wrote:
> Is this ready for more of a review now that the CaptivePortalService is pretty
> close to landing? Or is it something which will be changed quite a bit?

Don't plan on any significant changes prior to landing, other than the browser
tests.  Still haven't gotten around to figuring out just what I want to add to
them - currently only two of them have the captive portal page load before the
login tab.  Want to at least get the one with three tabs doing the same thing. 
Beyond that, not sure.

I've gone ahead and uploaded the browser_tests file (And its dependencies).

[mmenke, 2012-05-03 22:23:07]

On 2012/05/03 21:55:47, Matt Menke wrote:
> On 2012/05/03 19:38:26, cbentzel wrote:
> > Is this ready for more of a review now that the CaptivePortalService is
pretty
> > close to landing? Or is it something which will be changed quite a bit?
> 
> Don't plan on any significant changes prior to landing, other than the browser
> tests.  Still haven't gotten around to figuring out just what I want to add to
> them - currently only two of them have the captive portal page load before the
> login tab.  Want to at least get the one with three tabs doing the same thing.

> Beyond that, not sure.
> 
> I've gone ahead and uploaded the browser_tests file (And its dependencies).

May want to hold off on a thorough review the browser tests, just so I can go
over them myself for style issues myself, tomorrow.

[mmenke, 2012-05-03 22:24:57]

On 2012/05/03 22:23:07, Matt Menke wrote:
> On 2012/05/03 21:55:47, Matt Menke wrote:
> > On 2012/05/03 19:38:26, cbentzel wrote:
> > > Is this ready for more of a review now that the CaptivePortalService is
> pretty
> > > close to landing? Or is it something which will be changed quite a bit?
> > 
> > Don't plan on any significant changes prior to landing, other than the
browser
> > tests.  Still haven't gotten around to figuring out just what I want to add
to
> > them - currently only two of them have the captive portal page load before
the
> > login tab.  Want to at least get the one with three tabs doing the same
thing.
> 
> > Beyond that, not sure.
> > 
> > I've gone ahead and uploaded the browser_tests file (And its dependencies).
> 
> May want to hold off on a thorough review the browser tests, just so I can go
> over them myself for style issues myself, tomorrow.

There's also no rush on review.  I'll have to get darin's approval and land the
other half first, anyways, and don't want to land that until after the branch,
anyways, and maybe let it stew a few days on Canary, too.

[cbentzel, 2012-05-15 01:16:56]

On 2012/05/03 22:24:57, Matt Menke wrote:
> On 2012/05/03 22:23:07, Matt Menke wrote:
> > On 2012/05/03 21:55:47, Matt Menke wrote:
> > > On 2012/05/03 19:38:26, cbentzel wrote:
> > > > Is this ready for more of a review now that the CaptivePortalService is
> > pretty
> > > > close to landing? Or is it something which will be changed quite a bit?
> > > 
> > > Don't plan on any significant changes prior to landing, other than the
> browser
> > > tests.  Still haven't gotten around to figuring out just what I want to
add
> to
> > > them - currently only two of them have the captive portal page load before
> the
> > > login tab.  Want to at least get the one with three tabs doing the same
> thing.
> > 
> > > Beyond that, not sure.
> > > 
> > > I've gone ahead and uploaded the browser_tests file (And its
dependencies).
> > 
> > May want to hold off on a thorough review the browser tests, just so I can
go
> > over them myself for style issues myself, tomorrow.
> 
> There's also no rush on review.  I'll have to get darin's approval and land
the
> other half first, anyways, and don't want to land that until after the branch,
> anyways, and maybe let it stew a few days on Canary, too.

Let me know when you want a review. When we chatted earlier today it sounded
like you still needed to work out some kinks with the tests.

[mmenke, 2012-05-17 17:01:48]

PTAL, when you get a chance.

[cbentzel, 2012-05-17 20:39:25]

Need to go through this some more. 

Does this handle HSTS style redirects? In that case, the http://site.com
redirect to https://site.com may stall, and since the original unsecure request
did not go over the wire it should not be a problem.

Also, I don't think this would catch secure upgrades due to the
Alternate-Protocol upgrade. Perhaps we should have that do HSTS style redirects?

http://codereview.chromium.org/10020051/diff/49028/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper.h (right):

http://codereview.chromium.org/10020051/diff/49028/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:25: // Class that
keeps track of whether a tab is a login page or has encountered
Nit: "Keeps track" instead of "Class that keeps track"

http://codereview.chromium.org/10020051/diff/49028/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:33: STATE_NONE,
It doesn't look like there is documentation here about why this restricts to SSL
provisional loads only. May help to provide justification.

http://codereview.chromium.org/10020051/diff/49028/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:45: // response a
RESULT_BEHIND_CAPTIVE_PORTAL.  The tab will remain in this
Nit: response to

[mmenke, 2012-05-17 20:52:23]

This does not handle redirects.  There is a way to watch for redirects on a
per-frame basis that should not be too hard to implement in the TabObserver, but
decided not to add it yet since this CL is already massive, with all the tests.

Definitely something I want to add soon after landing this, for the HSTs case. 
Think that's the only case where it really matters.

http://codereview.chromium.org/10020051/diff/49028/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper.h (right):

http://codereview.chromium.org/10020051/diff/49028/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:25: // Class that
keeps track of whether a tab is a login page or has encountered
On 2012/05/17 20:39:25, cbentzel wrote:
> Nit: "Keeps track" instead of "Class that keeps track"

Done.

http://codereview.chromium.org/10020051/diff/49028/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:33: STATE_NONE,
On 2012/05/17 20:39:25, cbentzel wrote:
> It doesn't look like there is documentation here about why this restricts to
SSL
> provisional loads only. May help to provide justification.

Done.  Also added another link to the design doc (The Service has one, too, but
think it's actually more relevant here).

http://codereview.chromium.org/10020051/diff/49028/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:45: // response a
RESULT_BEHIND_CAPTIVE_PORTAL.  The tab will remain in this
On 2012/05/17 20:39:25, cbentzel wrote:
> Nit: response to

Done.

[cbentzel, 2012-05-18 03:23:16]

Too tired to finish this review tonight, sorry.

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper.h (right):

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:30: // Only supports
SSL main frames which time out in response to captive portals,
Ah - I think what I was getting at here is that non-SSL main frames should get
the captive portal interception page. SSL main frames have the potential to be
blackholed and time out. Hence, we don't need to track non-SSL main frames but
do need to track SSL main frames.

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:39: enum State {
Why does this need to be public?

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_observer.cc (right):

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_observer.cc:18:
pending_error_code_(net::OK) {
DHCECK that tab_helper_ is non-NULL.

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_observer.cc:24: void
CaptivePortalTabObserver::DidStartProvisionalLoadForFrame(
One thought I had - let's say the main page is for an https site, but the
safebrowsing interstitial comes up. Will we run into incorrect detection then?
I'm also concerned about any of the webRequest extensions delaying the main
frame load and how that gets signaled.

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_observer.cc:70: // Ignore
subframes.
So, I don't know enough about how the error page sequence works. Is this
correct?

- We get a DidStartProvisionalLoadForFrame for the intended page.

- That fails with a net error, and we get DidFailProvisionalLoad, with the error
code (is this a net error code, or something different)?

- We then do a DidStartProvisionalLoadForFrame for the error page.

- This then does a DidCommitProvisionalLoadForFrame for the error page.

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_observer.cc:75: // reset the
state. now, to prevent any captive portal checks from triggering.
Nit: unneeded period.

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_observer.h (right):

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_observer.h:23: //              
at the network layer.  Also should reduce the number of
Why does this reduce the number of unnecessary checks?

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_observer.h:54: virtual void
DidStopLoading() OVERRIDE;
I'm unsure if this should use DidStopLoading or DidFinishLoad as the signal.

[mmenke, 2012-05-18 06:12:52]

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper.h (right):

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:30: // Only supports
SSL main frames which time out in response to captive portals,
On 2012/05/18 03:23:16, cbentzel wrote:
> Ah - I think what I was getting at here is that non-SSL main frames should get
> the captive portal interception page. SSL main frames have the potential to be
> blackholed and time out. Hence, we don't need to track non-SSL main frames but
> do need to track SSL main frames.

Comment updated.

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:39: enum State {
On 2012/05/18 03:23:16, cbentzel wrote:
> Why does this need to be public?

So individual unit tests can use them, without friending every single unit test.

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_observer.cc (right):

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_observer.cc:18:
pending_error_code_(net::OK) {
On 2012/05/18 03:23:16, cbentzel wrote:
> DHCECK that tab_helper_ is non-NULL.

Done.

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_observer.cc:24: void
CaptivePortalTabObserver::DidStartProvisionalLoadForFrame(
On 2012/05/18 03:23:16, cbentzel wrote:
> One thought I had - let's say the main page is for an https site, but the
> safebrowsing interstitial comes up. Will we run into incorrect detection then?
> I'm also concerned about any of the webRequest extensions delaying the main
> frame load and how that gets signaled. 

On safe browsing pages I'm pretty sure we will indeed trigger, if the
interstitial is shown for over 30 seconds.

webRequest extensions that delay loading for 30 seconds will also trigger
checks.

We also can't monitor when requests go from the queue to actually being started,
so it's possible that in the case of proxies, especially, or on startup, we
could trigger checks erroneously.

We could handle these cases, but we'd have to plug in to a lot more.  Simplest
option would probably be to monitor the NetLog (Which doesn't get us
safebrowsing, but does get us webRequests and tell us when requests start,
though we'd need to be able to map frame_ids to net log ids, which may be a
little tricky, but we do elsewhere in the code already).

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_observer.cc:70: // Ignore
subframes.
On 2012/05/18 03:23:16, cbentzel wrote:
> So, I don't know enough about how the error page sequence works. Is this
> correct?
> 
> - We get a DidStartProvisionalLoadForFrame for the intended page.
> 
> - That fails with a net error, and we get DidFailProvisionalLoad, with the
error
> code (is this a net error code, or something different)?
> 
> - We then do a DidStartProvisionalLoadForFrame for the error page.
> 
> - This then does a DidCommitProvisionalLoadForFrame for the error page.

Yes.  And it is indeed a net error page.  The same one we're going to display
the error page for.  The DidStartProvisionalLoadForFrame has an is_error value
of true.

For link doctor pages, we get a bonus DidStart/DidCommit for the link doctor
page itself as well.

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_observer.cc:75: // reset the
state. now, to prevent any captive portal checks from triggering.
On 2012/05/18 03:23:16, cbentzel wrote:
> Nit: unneeded period.

Removed

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_observer.h (right):

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_observer.h:23: //              
at the network layer.  Also should reduce the number of
On 2012/05/18 03:23:16, cbentzel wrote:
> Why does this reduce the number of unnecessary checks?

Thinking false positives are more likely in the case redirect chains, and we can
reset the timer on SSL to SSL transitions.  30 second timer should be long
enough to minimize that, anyways, however.  Of course, we may do more than one
captive portal check on really, really slow redirects.

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_observer.h:54: virtual void
DidStopLoading() OVERRIDE;
On 2012/05/18 03:23:16, cbentzel wrote:
> I'm unsure if this should use DidStopLoading or DidFinishLoad as the signal. 

Went with DidStopLoading because I can imagine a captive portal using frames to
log in.  Of course, one can imagine captive portals that do all sorts of
things....  Use XHR requests (Which I don't believe will trigger another
start/stop loading event), having hanging subresources that end up loading until
they themselves timeout, etc.

[mmenke, 2012-05-18 08:08:09]

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_observer.h (right):

http://codereview.chromium.org/10020051/diff/65007/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_observer.h:54: virtual void
DidStopLoading() OVERRIDE;
On 2012/05/18 06:12:53, Matt Menke wrote:
> On 2012/05/18 03:23:16, cbentzel wrote:
> > I'm unsure if this should use DidStopLoading or DidFinishLoad as the signal.

> 
> Went with DidStopLoading because I can imagine a captive portal using frames
to
> log in.  Of course, one can imagine captive portals that do all sorts of
> things....  Use XHR requests (Which I don't believe will trigger another
> start/stop loading event), having hanging subresources that end up loading
until
> they themselves timeout, etc.

Oh, and also, if logging it takes you to a redirect to the 304 page, then that
load is aborted, so will not result in a DidFinishLoad, but rather a
DidFailProvisionalLoad.  Then there's also the DidFailLoads to consider...

[cbentzel, 2012-05-22 17:54:27]

http://codereview.chromium.org/10020051/diff/66013/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper.h (right):

http://codereview.chromium.org/10020051/diff/66013/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:38: class
CaptivePortalTabHelper : public content::NotificationObserver,
Would it make sense to have different TabHelper's for the login page versus
every other page?

http://codereview.chromium.org/10020051/diff/66013/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:98: virtual void
OnLoadCommitted(int net_error);
Could this use net::Error instead of int?

[mmenke, 2012-05-22 18:22:35]

http://codereview.chromium.org/10020051/diff/66013/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper.h (right):

http://codereview.chromium.org/10020051/diff/66013/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:38: class
CaptivePortalTabHelper : public content::NotificationObserver,
On 2012/05/22 17:54:27, cbentzel wrote:
> Would it make sense to have different TabHelper's for the login page versus
> every other page?

I've gone back and forth on this more than once.

We need to be able to tell if a tab is a captive portal login page to avoid
opening multiple login tabs, so there needs to be some publicly accessible
entity that tracks this.  Two obvious ways to do this are to make them part of
the TabContentsWrapper interface, or have the CaptivePortalService track them
(In the latter case, we could only create them as needed).  I didn't go with the
second option because of the Browser-not-supported-on-Android issue means the
CaptivePortalService can't open the login tab, so it seems weird to have it
track them, though we could just make that one function non-android, if so
desired.

Similarly, I prefer that BrowserTests be able to get the CaptivePortalTabHelper
state of a tab, though it's not strictly necessary, and to do that, they need to
be part of the TabContentsWrapper interface, unless something else tracks them.

I decided to go with just one class to avoid interface bloat, but having two
always-existent observers is certainly doable.  Since they share pretty much no
behavior with each other, it would simplify some things a little.

Another option is to replace the STATE_CAPTIVE_PORTAL_LOGIN_PAGE with a boolean
flag, tracked independently of everything else.  Cleared when a captive portal
goes away, and just use it to check state again when a load stops.  This would
remove a couple of the "if STATE_CAPTIVE_PORTAL_LOGIN_PAGE, do nothing" checks,
and isolate the two types of state, while also introducing only a single new
TabHelper.

I think all 3 approaches are reasonable, though have a weak preference for the
last option (Get rid of state, use a bool).  Didn't go with it because it
occurred to me late, and wanted to see if you had any opinion on the three
options.  If you have a preference for two classes, I'll go ahead and go that
route.

http://codereview.chromium.org/10020051/diff/66013/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:98: virtual void
OnLoadCommitted(int net_error);
On 2012/05/22 17:54:27, cbentzel wrote:
> Could this use net::Error instead of int? 

The WebContentsObserver only declares it as an int, and the network stack almost
always passes net::Errors around as ints as well, so I think using an int is
more in keeping with existing style.  I admit to also not being a big fan of
casts unless necessary.  In this case, it makes it through a couple levels of
WebKit in the renderer process before we finally get it.

[cbentzel, 2012-05-24 12:41:37]

http://codereview.chromium.org/10020051/diff/66013/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper.h (right):

http://codereview.chromium.org/10020051/diff/66013/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:38: class
CaptivePortalTabHelper : public content::NotificationObserver,
On 2012/05/22 18:22:36, Matt Menke wrote:
> On 2012/05/22 17:54:27, cbentzel wrote:
> > Would it make sense to have different TabHelper's for the login page versus
> > every other page?
> 
> I've gone back and forth on this more than once.
> 
> We need to be able to tell if a tab is a captive portal login page to avoid
> opening multiple login tabs, so there needs to be some publicly accessible
> entity that tracks this.  Two obvious ways to do this are to make them part of
> the TabContentsWrapper interface, or have the CaptivePortalService track them
> (In the latter case, we could only create them as needed).  I didn't go with
the
> second option because of the Browser-not-supported-on-Android issue means the
> CaptivePortalService can't open the login tab, so it seems weird to have it
> track them, though we could just make that one function non-android, if so
> desired.
> 
> Similarly, I prefer that BrowserTests be able to get the
CaptivePortalTabHelper
> state of a tab, though it's not strictly necessary, and to do that, they need
to
> be part of the TabContentsWrapper interface, unless something else tracks
them.
> 
> I decided to go with just one class to avoid interface bloat, but having two
> always-existent observers is certainly doable.  Since they share pretty much
no
> behavior with each other, it would simplify some things a little.
> 
> Another option is to replace the STATE_CAPTIVE_PORTAL_LOGIN_PAGE with a
boolean
> flag, tracked independently of everything else.  Cleared when a captive portal
> goes away, and just use it to check state again when a load stops.  This would
> remove a couple of the "if STATE_CAPTIVE_PORTAL_LOGIN_PAGE, do nothing"
checks,
> and isolate the two types of state, while also introducing only a single new
> TabHelper.
> 
> I think all 3 approaches are reasonable, though have a weak preference for the
> last option (Get rid of state, use a bool).  Didn't go with it because it
> occurred to me late, and wanted to see if you had any opinion on the three
> options.  If you have a preference for two classes, I'll go ahead and go that
> route.

I would not have two classes which are directly referenced by TabContentsWrapper
since that adds complexity for users of this class. You could have
CaptivePortalTabHelper hold scoped_ptr's to objects of the two different
classes, and just forward requests along to them. I do think that the
implementation may be simpler in that case, since the state machines are pretty
much completely separate for the login tab page versus every other tab.

[mmenke, 2012-05-24 14:19:04]

http://codereview.chromium.org/10020051/diff/66013/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper.h (right):

http://codereview.chromium.org/10020051/diff/66013/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:38: class
CaptivePortalTabHelper : public content::NotificationObserver,
On 2012/05/24 12:41:37, cbentzel wrote:
> On 2012/05/22 18:22:36, Matt Menke wrote:
> > On 2012/05/22 17:54:27, cbentzel wrote:
> > > Would it make sense to have different TabHelper's for the login page
versus
> > > every other page?
> > 
> > I've gone back and forth on this more than once.
> > 
> > We need to be able to tell if a tab is a captive portal login page to avoid
> > opening multiple login tabs, so there needs to be some publicly accessible
> > entity that tracks this.  Two obvious ways to do this are to make them part
of
> > the TabContentsWrapper interface, or have the CaptivePortalService track
them
> > (In the latter case, we could only create them as needed).  I didn't go with
> the
> > second option because of the Browser-not-supported-on-Android issue means
the
> > CaptivePortalService can't open the login tab, so it seems weird to have it
> > track them, though we could just make that one function non-android, if so
> > desired.
> > 
> > Similarly, I prefer that BrowserTests be able to get the
> CaptivePortalTabHelper
> > state of a tab, though it's not strictly necessary, and to do that, they
need
> to
> > be part of the TabContentsWrapper interface, unless something else tracks
> them.
> > 
> > I decided to go with just one class to avoid interface bloat, but having two
> > always-existent observers is certainly doable.  Since they share pretty much
> no
> > behavior with each other, it would simplify some things a little.
> > 
> > Another option is to replace the STATE_CAPTIVE_PORTAL_LOGIN_PAGE with a
> boolean
> > flag, tracked independently of everything else.  Cleared when a captive
portal
> > goes away, and just use it to check state again when a load stops.  This
would
> > remove a couple of the "if STATE_CAPTIVE_PORTAL_LOGIN_PAGE, do nothing"
> checks,
> > and isolate the two types of state, while also introducing only a single new
> > TabHelper.
> > 
> > I think all 3 approaches are reasonable, though have a weak preference for
the
> > last option (Get rid of state, use a bool).  Didn't go with it because it
> > occurred to me late, and wanted to see if you had any opinion on the three
> > options.  If you have a preference for two classes, I'll go ahead and go
that
> > route.
> 
> I would not have two classes which are directly referenced by
TabContentsWrapper
> since that adds complexity for users of this class. You could have
> CaptivePortalTabHelper hold scoped_ptr's to objects of the two different
> classes, and just forward requests along to them. I do think that the
> implementation may be simpler in that case, since the state machines are
pretty
> much completely separate for the login tab page versus every other tab.

SGTM, will do.

[mmenke, 2012-05-30 15:00:42]

PTAL.  Simplest to compare patch set 20 to the latest patch, to just view
changes.

I basically converted the CaptivePortalObserver to the CaptivePortalTabHelper,
and renamed CaptivePortalTabHelper to CaptivePortalReloader (Which is pretty
much all that patch set 20 does).

I could split the old CaptivePortalObserver apart again pretty easily, but it
seemed redundant.

Another option would be to have the CaptivePortalReloader and
CaptivePortalLoginDetector do their own observing of both notifications and the
TabContents.  This would simplify the tab helper a bit, at the cost of adding
observers (And I'd need to bring the CaptivePortalObserver back as well).

[cbentzel, 2012-05-30 16:26:42]

I'll need to do a fine-grained pass, but this approach generally L G T M. I
think the separation of the login state made the CaptivePortalTabReloader easier
to understand.

Also, please confirm: if the user does not opt-in to the captive portal
detection, we'll still be running the CaptivePortalTabHelper but the actual
captive portal checks in the CaptivePortalService will be disabled.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_login_detector.h (right):

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_login_detector.h:10: #include
"base/compiler_specific.h"
Where is compiler_specific userd?

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_login_detector.h:29: virtual
~CaptivePortalLoginDetector();
Why does this need to be virtual?

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_reloader.cc (right):

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_reloader.cc:223: void
CaptivePortalTabReloader::MaybeOpenCaptivePortalLoginTab() {
Should this be done by passing a callback in, rather than having a direct
dependency on CaptivePortalTabHelper? I wonder if it would make testing easier.

[mmenke, 2012-05-30 17:46:24]

On 2012/05/30 16:26:42, cbentzel wrote:
> I'll need to do a fine-grained pass, but this approach generally L G T M. I
> think the separation of the login state made the CaptivePortalTabReloader
easier
> to understand.
> 
> Also, please confirm: if the user does not opt-in to the captive portal
> detection, we'll still be running the CaptivePortalTabHelper but the actual
> captive portal checks in the CaptivePortalService will be disabled.

That is correct.  We'll run the timer on SSL loads, cancelling it when they
complete.  If the timer triggers, we'll run a fake captive portal check that
will just post a message back to us saying there's no captive portal.

This isn't a lot of work, and I prefer to avoid having too many different checks
based on whether the service is enabled or disabled, though we could opt to just
not start the timer when the service is disabled.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_login_detector.h (right):

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_login_detector.h:10: #include
"base/compiler_specific.h"
On 2012/05/30 16:26:42, cbentzel wrote:
> Where is compiler_specific userd?

Removed.  OVERRIDE and DISALLOW_COPY_AND_ASSIGN being in different, randomly
named headers always confuses me.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_login_detector.h:29: virtual
~CaptivePortalLoginDetector();
On 2012/05/30 16:26:42, cbentzel wrote:
> Why does this need to be virtual? 

Removed.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_reloader.cc (right):

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_reloader.cc:223: void
CaptivePortalTabReloader::MaybeOpenCaptivePortalLoginTab() {
On 2012/05/30 16:26:42, cbentzel wrote:
> Should this be done by passing a callback in, rather than having a direct
> dependency on CaptivePortalTabHelper? I wonder if it would make testing
easier.

Done.  I like getting rid of the circular dependency, and making SetIsLoginTab
private (Left IsLoginTab public mostly to simplify testing, though could make it
private, too).

However, since I'm still mocking out ReloadTab and ReloadTabIfNeeded, I don't
see how I can use it to make the tests any simpler.

[cbentzel, 2012-05-30 18:48:02]

The only thing I am concerned about is whether we want to do the captive portal
detection in incognito mode. The static JobList also needs to be addressed.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_browsertest.cc (right):

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:80: const char*
const kMockHttpsUrl = "https://mock.captive.portal/";
Nit: perhaps https://mock.captive.portal/long_timeout/ instead?

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:124: static JobList
waiting_jobs_list_;
Static class member variables must be POD only.

You could probably change this to be a static URLRequestTimeoutOnDemandJob*, and
have an intrusive linked list approach to maintain a list of all instances.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:185: //
SetBehindCaptivePortal(true) is called, it emulates behavior when not behind
Is this meant to be SetBehindCaptivePortal(false)?

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:221:
DISALLOW_COPY_AND_ASSIGN(URLRequestMockCaptivePortalJobFactory);
You could use DISALLOW_IMPLICIT_CONSTRUCTORS() here instead.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:276: if
(request->url().spec() == kMockHttpsQuickTimeoutUrl)
Is this doing a std::string comparison or just a char*? May be clearer to make a
GURL(kMockHttpsQuickTimeoutUrl) instead?

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:315: if
(tab_contents_it->web_contents()->IsLoading())
Does this go across all browser windows?

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:333: // Waits for
exactly least |num_navigations_to_wait_for| LOAD_STOP
Nit: Should be "exactly" without the "least".

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:406: if
(num_navigations_to_wait_for_ == num_navigations_)
Should this only quit the loop if WaitForNavigations has been called?

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:878: void
CaptivePortalBrowserTest::FastTimeoutBehindCaptivePortal(
Is there a test for when the captive portal test routinely times out/503s? Or is
that covered by CaptivePortalService tests?

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:1128:
NavigateToPageExpectNoTest(browser(), url, 2);
Should there be a test for the case that the user has LinkDoctor disabled?

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:1231: // non-NULL
captive portal service, and we open the tab in the correct
Let's make sure that this is desired behavior for the incognito case. I could
imagine that not allowing captive portal detection would be allowed.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc
(right):

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:144:
TEST_F(CaptivePortalTabHelperTest, HttpTimeoutLinkDoctor) {
Do most of these belong here, or in the CaptivePortalTabReloader tests?

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/ui/tab_cont...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/ui/tab_cont...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:94: #if
!defined(OS_ANDROID)
Should you add a #define such as ENABLE_CAPTIVE_PORTAL_DETECTION instead?

[cbentzel, 2012-05-31 17:29:00]

On 2012/05/30 18:48:02, cbentzel wrote:
> The only thing I am concerned about is whether we want to do the captive
portal
> detection in incognito mode.

I consider the incognito issue resolved. So all that remains are the existing
code nits I raised in the previous round.

[mmenke, 2012-05-31 20:31:45]

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_browsertest.cc (right):

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:80: const char*
const kMockHttpsUrl = "https://mock.captive.portal/";
On 2012/05/30 18:48:02, cbentzel wrote:
> Nit: perhaps https://mock.captive.portal/long_timeout/ instead?

Done.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:124: static JobList
waiting_jobs_list_;
On 2012/05/30 18:48:02, cbentzel wrote:
> Static class member variables must be POD only.
> 
> You could probably change this to be a static URLRequestTimeoutOnDemandJob*,
and
> have an intrusive linked list approach to maintain a list of all instances.

Done.  For some reason, had just assumed something that's an empty list at quit,
and only used in tests would be fine.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:185: //
SetBehindCaptivePortal(true) is called, it emulates behavior when not behind
On 2012/05/30 18:48:02, cbentzel wrote:
> Is this meant to be SetBehindCaptivePortal(false)?

Indeed.  Done.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:221:
DISALLOW_COPY_AND_ASSIGN(URLRequestMockCaptivePortalJobFactory);
On 2012/05/30 18:48:02, cbentzel wrote:
> You could use DISALLOW_IMPLICIT_CONSTRUCTORS() here instead.

Done.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:276: if
(request->url().spec() == kMockHttpsQuickTimeoutUrl)
On 2012/05/30 18:48:02, cbentzel wrote:
> Is this doing a std::string comparison or just a char*? May be clearer to make
a
> GURL(kMockHttpsQuickTimeoutUrl) instead?

std::string.  Switched, as suggested.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:315: if
(tab_contents_it->web_contents()->IsLoading())
On 2012/05/30 18:48:02, cbentzel wrote:
> Does this go across all browser windows?

Yes.  And all profiles, too.  Updated comment.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:333: // Waits for
exactly least |num_navigations_to_wait_for| LOAD_STOP
On 2012/05/30 18:48:02, cbentzel wrote:
> Nit: Should be "exactly" without the "least".

Done.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:406: if
(num_navigations_to_wait_for_ == num_navigations_)
On 2012/05/30 18:48:02, cbentzel wrote:
> Should this only quit the loop if WaitForNavigations has been called?

|num_navigations_to_wait_for_| is only non-zero if WaitForNavigations has been
called, and |num_navigations_| is always at least one, at this point, so not
needed.

However, I'll go ahead and add a bool to the two classes that do this in this
CL, to make things clearer.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:878: void
CaptivePortalBrowserTest::FastTimeoutBehindCaptivePortal(
On 2012/05/30 18:48:02, cbentzel wrote:
> Is there a test for when the captive portal test routinely times out/503s? Or
is
> that covered by CaptivePortalService tests?

The CaptivePortalServiceTests do that, since they can pretend to advance time
without too much extra work.  Don't think there's much benefit to testing the
timers here as well.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:1128:
NavigateToPageExpectNoTest(browser(), url, 2);
On 2012/05/30 18:48:02, cbentzel wrote:
> Should there be a test for the case that the user has LinkDoctor disabled? 

In that case, captive portal testing would be disabled too.  Added a test for
that case.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:1231: // non-NULL
captive portal service, and we open the tab in the correct
On 2012/05/30 18:48:02, cbentzel wrote:
> Let's make sure that this is desired behavior for the incognito case. I could
> imagine that not allowing captive portal detection would be allowed.

Done.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc
(right):

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:144:
TEST_F(CaptivePortalTabHelperTest, HttpTimeoutLinkDoctor) {
On 2012/05/30 18:48:02, cbentzel wrote:
> Do most of these belong here, or in the CaptivePortalTabReloader tests? 

Here, we make sure TabReloader always either the correct pattern of one or more
of:

OnLoadStart -> OnLoadCommitted(error)
OnLoadStart -> OnLoadCommitted(OK)
OnLoadStart -> OnAbort()

Given input from the TabContents.

The tests for the Reloader make sure that those pairs of steps intermixed with
the results captive portal results work.

If merged, every single Reloader test would have two more steps (DidFail,
DidStartLoading for the error page), and to adequately protect against
regressions, we'd also need to test when captive portal results are received
between those extra steps, despite the fact the CaptivePortalReloader currently
doesn't care about them.

Given the current complexity of the Reloader tests without the extra steps, I
think it's simpler to test the two classes separately, rather than merge them
into more tests that are more complicated.

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/ui/tab_cont...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/10020051/diff/81008/chrome/browser/ui/tab_cont...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:94: #if
!defined(OS_ANDROID)
On 2012/05/30 18:48:02, cbentzel wrote:
> Should you add a #define such as ENABLE_CAPTIVE_PORTAL_DETECTION instead?

Done and tested, both with it enabled and not.  I rather like this approach.

[cbentzel, 2012-05-31 20:50:43]

LGTM

http://codereview.chromium.org/10020051/diff/79008/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_browsertest.cc (right):

http://codereview.chromium.org/10020051/diff/79008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:173:
URLRequestTimeoutOnDemandJob** job = &job_list_;
Ah, you know the ** trick for iterating and removing an element in a singly
linked list.

[mmenke, 2012-05-31 21:12:57]

Thanks for the comments.

http://codereview.chromium.org/10020051/diff/79008/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_browsertest.cc (right):

http://codereview.chromium.org/10020051/diff/79008/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:173:
URLRequestTimeoutOnDemandJob** job = &job_list_;
On 2012/05/31 20:50:43, cbentzel wrote:
> Ah, you know the ** trick for iterating and removing an element in a singly
> linked list. 

Does that put me among some sort of elite hacker cadre?  Is there a secret
handshake?  Or does that just mean I'm such a slow enough typist that I use
tricks to save a couple lines of code.

[mmenke, 2012-06-06 17:21:15]

[+avi]  Could you please review this?  Thanks!  Might want to skip/skim the
tests, as they're kinda huge.

[mmenke, 2012-06-12 14:38:31]

[avi]  Ping!  Just want to make sure this is on your radar.

[Avi (use Gerrit), 2012-06-14 15:24:28]

lgtm

http://codereview.chromium.org/10020051/diff/99002/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper.h (right):

http://codereview.chromium.org/10020051/diff/99002/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:30: //
CaptivePortalTabReloader.  It filters out non-main-frame resource loads, and
acts as a WCO for its X and X. Are those two Xs supposed to be different?

[mmenke, 2012-06-14 15:37:25]

http://codereview.chromium.org/10020051/diff/99002/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper.h (right):

http://codereview.chromium.org/10020051/diff/99002/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:30: //
CaptivePortalTabReloader.  It filters out non-main-frame resource loads, and
On 2012/06/14 15:24:28, Avi wrote:
> acts as a WCO for its X and X. Are those two Xs supposed to be different?

Indeed.  Fixed (One should be CaptivePortalLoginDetector)

[commit-bot: I haz the power, 2012-06-14 17:41:41]

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

[commit-bot: I haz the power, 2012-06-14 17:41:50]

Presubmit check for 10020051-106004 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/browser/profiles

Presubmit checks took 2.1s to calculate.

[mmenke, 2012-06-14 17:47:52]

[+erg]:  Please review profiles, thanks.

[Elliot Glaysher, 2012-06-14 17:49:19]

lgtm

[mmenke, 2012-06-14 18:04:18]

Thanks

[commit-bot: I haz the power, 2012-06-14 18:04:45]

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

[commit-bot: I haz the power, 2012-06-14 19:46:29]

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

[commit-bot: I haz the power, 2012-06-14 21:28:48]

Change committed as 142242