Move CaptivePortalDetector to src/components/captive_portal

Move CaptivePortalDetector to src/chomponents/captive_portal

BUG=364894
For trivial header file renaming in c/b and c/test:

R=mmenke@chromium.org, ygorshenin@chromium.org
TBR=sky@chromium.org

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

[stevenjb, 2014-04-18 21:02:46]

Moved to a component.
PTAL

[willchan no longer on Chromium, 2014-04-18 22:09:20]

I defer the review to mmenke. That said, the CL description is wrong (the
message is correct though).


On Fri, Apr 18, 2014 at 2:02 PM, <stevenjb@chromium.org> wrote:

> Reviewers: mmenke, ygorshenin1, willchan,
>
> Message:
> Moved to a component.
> PTAL
>
>
> Description:
> Move CaptivePortalDetector to src/net
>
> BUG=364894
>
>
> Please review this at https://codereview.chromium.org/242483003/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+469, -949 lines):
>   A chrome/browser/captive_portal/DEPS
>   M chrome/browser/captive_portal/captive_portal_browsertest.cc
>   D chrome/browser/captive_portal/captive_portal_detector.h
>   D chrome/browser/captive_portal/captive_portal_detector.cc
>   D chrome/browser/captive_portal/captive_portal_detector_unittest.cc
>   M chrome/browser/captive_portal/captive_portal_login_detector.h
>   M chrome/browser/captive_portal/captive_portal_login_detector.cc
>   M chrome/browser/captive_portal/captive_portal_service.h
>   M chrome/browser/captive_portal/captive_portal_service.cc
>   M chrome/browser/captive_portal/captive_portal_service_unittest.cc
>   M chrome/browser/captive_portal/captive_portal_tab_helper.h
>   M chrome/browser/captive_portal/captive_portal_tab_helper.cc
>   M chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc
>   M chrome/browser/captive_portal/captive_portal_tab_reloader.h
>   M chrome/browser/captive_portal/captive_portal_tab_reloader.cc
>   D chrome/browser/captive_portal/testing_utils.h
>   D chrome/browser/captive_portal/testing_utils.cc
>   M chrome/browser/chromeos/customization_document_unittest.cc
>   M chrome/browser/chromeos/login/DEPS
>   M chrome/browser/chromeos/login/captive_portal_view.cc
>   M chrome/browser/chromeos/login/captive_portal_window_browsertest.cc
>   M chrome/browser/chromeos/login/enrollment/auto_enrollment_check_step.h
>   M chrome/browser/chromeos/login/enrollment/auto_enrollment_check_step.cc
>   M chrome/browser/chromeos/login/managed/locally_managed_user_
> creation_screen.cc
>   M chrome/browser/chromeos/login/managed/supervised_user_
> creation_browsertest.cc
>   M chrome/browser/chromeos/login/oobe_base_test.cc
>   M chrome/browser/chromeos/login/screens/update_screen.h
>   M chrome/browser/chromeos/login/screens/update_screen.cc
>   M chrome/browser/chromeos/login/screens/update_screen_browsertest.cc
>   A chrome/browser/chromeos/net/DEPS
>   M chrome/browser/chromeos/net/delay_network_call.cc
>   M chrome/browser/chromeos/net/network_portal_detector.h
>   M chrome/browser/chromeos/net/network_portal_detector.cc
>   M chrome/browser/chromeos/net/network_portal_detector_impl.h
>   M chrome/browser/chromeos/net/network_portal_detector_impl.cc
>   M chrome/browser/chromeos/net/network_portal_detector_impl_
> browsertest.cc
>   M chrome/browser/chromeos/net/network_portal_detector_impl_unittest.cc
>   M chrome/browser/chromeos/net/network_portal_notification_controller.cc
>   M chrome/browser/chromeos/net/network_portal_notification_
> controller_unittest.cc
>   M chrome/browser/extensions/api/networking_private/networking_
> private_api_chromeos.cc
>   M chrome/browser/extensions/api/networking_private/networking_
> private_apitest.cc
>   M chrome/browser/extensions/api/networking_private/networking_
> private_event_router_chromeos.cc
>   M chrome/browser/ssl/ssl_blocking_page.cc
>   M chrome/browser/ui/ash/system_tray_delegate_chromeos.cc
>   M chrome/browser/ui/webui/chromeos/login/network_state_informer.cc
>   M chrome/chrome_browser.gypi
>   M chrome/chrome_tests.gypi
>   M chrome/chrome_tests_unit.gypi
>   A components/captive_portal.gypi
>   A + components/captive_portal/DEPS
>   A + components/captive_portal/OWNERS
>   A + components/captive_portal/captive_portal_detector.h
>   A + components/captive_portal/captive_portal_detector.cc
>   A + components/captive_portal/captive_portal_detector_unittest.cc
>   A components/captive_portal/captive_portal_export.h
>   A + components/captive_portal/captive_portal_testing_utils.h
>   A + components/captive_portal/captive_portal_testing_utils.cc
>   A components/captive_portal/captive_portal_types.h
>   A components/captive_portal/captive_portal_types.cc
>   M components/components.gyp
>   M components/components_tests.gyp
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ygorshenin1, 2014-04-21 13:36:44]

lgtm

https://codereview.chromium.org/242483003/diff/40001/chrome/browser/captive_p...
File chrome/browser/captive_portal/captive_portal_service.cc (right):

https://codereview.chromium.org/242483003/diff/40001/chrome/browser/captive_p...
chrome/browser/captive_portal/captive_portal_service.cc:67:
captive_portal::CaptivePortalResultToString(result),
Why do you need to explicitly specify namespace here? RecordRepeatHistograms()
is already in the captive_portal namespace.

https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
File components/captive_portal/captive_portal_detector.cc (right):

https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
components/captive_portal/captive_portal_detector.cc:81: results->result =
captive_portal::RESULT_NO_RESPONSE;
Why did you explicitly specify namespace here?

[mmenke, 2014-04-21 14:32:42]

https://codereview.chromium.org/242483003/diff/40001/chrome/browser/captive_p...
File chrome/browser/captive_portal/captive_portal_browsertest.cc (right):

https://codereview.chromium.org/242483003/diff/40001/chrome/browser/captive_p...
chrome/browser/captive_portal/captive_portal_browsertest.cc:56: using
captive_portal::CaptivePortalDetector;
If we're going to share the captive_portal namespace, this isn't needed.

https://codereview.chromium.org/242483003/diff/40001/chrome/browser/captive_p...
File chrome/browser/captive_portal/captive_portal_login_detector.h (right):

https://codereview.chromium.org/242483003/diff/40001/chrome/browser/captive_p...
chrome/browser/captive_portal/captive_portal_login_detector.h:13: namespace
captive_portal {
Should we be using the namespace both in components/ and in chrome?  Not
familiar with what's usually done, just seems odd to me to have a namespace span
the two projects.

https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
File components/captive_portal/captive_portal_detector_unittest.cc (right):

https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
components/captive_portal/captive_portal_detector_unittest.cc:57:
scoped_refptr<net::URLRequestContextGetter> request_context_getter(
Should include ref_counted.h (And may as well add scoped_ptr.h while you're
doing so)

https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
File components/captive_portal/captive_portal_testing_utils.h (right):

https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
components/captive_portal/captive_portal_testing_utils.h:20: class URLFetcher;
There isn't a captive_portal::URLFetcher class...  In fact, we don't seem to
need this forward declaration at all.

https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
File components/captive_portal/captive_portal_types.h (right):

https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
components/captive_portal/captive_portal_types.h:26: enum CaptivePortalStatus {
These don't seem to be used anywhere in the component.  It's also unclear what
the difference between a status and a result is.

[stevenjb, 2014-04-21 16:42:26]

PTAL

https://codereview.chromium.org/242483003/diff/40001/chrome/browser/captive_p...
File chrome/browser/captive_portal/captive_portal_browsertest.cc (right):

https://codereview.chromium.org/242483003/diff/40001/chrome/browser/captive_p...
chrome/browser/captive_portal/captive_portal_browsertest.cc:56: using
captive_portal::CaptivePortalDetector;
On 2014/04/21 14:32:43, mmenke wrote:
> If we're going to share the captive_portal namespace, this isn't needed.

Done.

https://codereview.chromium.org/242483003/diff/40001/chrome/browser/captive_p...
File chrome/browser/captive_portal/captive_portal_login_detector.h (right):

https://codereview.chromium.org/242483003/diff/40001/chrome/browser/captive_p...
chrome/browser/captive_portal/captive_portal_login_detector.h:13: namespace
captive_portal {
On 2014/04/21 14:32:43, mmenke wrote:
> Should we be using the namespace both in components/ and in chrome?  Not
> familiar with what's usually done, just seems odd to me to have a namespace
span
> the two projects.

I've seen this done before (we definitely do it with chromoeos and ash, I feel
like I've seen it with other libraries). src/components contains the
non-chrome-specific library, and src/chrome contains the chrome specific parts.
I would be OK with removing the namespacing from src/chrome if there is a strong
preference for that.

https://codereview.chromium.org/242483003/diff/40001/chrome/browser/captive_p...
File chrome/browser/captive_portal/captive_portal_service.cc (right):

https://codereview.chromium.org/242483003/diff/40001/chrome/browser/captive_p...
chrome/browser/captive_portal/captive_portal_service.cc:67:
captive_portal::CaptivePortalResultToString(result),
On 2014/04/21 13:36:45, ygorshenin1 wrote:
> Why do you need to explicitly specify namespace here? RecordRepeatHistograms()
> is already in the captive_portal namespace.

Done.

https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
File components/captive_portal/captive_portal_detector.cc (right):

https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
components/captive_portal/captive_portal_detector.cc:81: results->result =
captive_portal::RESULT_NO_RESPONSE;
On 2014/04/21 13:36:45, ygorshenin1 wrote:
> Why did you explicitly specify namespace here?
In this case, I prefer explicitly namespaced enums, it makes it easier to tell
that the enum is not class or file local.

https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
File components/captive_portal/captive_portal_detector_unittest.cc (right):

https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
components/captive_portal/captive_portal_detector_unittest.cc:57:
scoped_refptr<net::URLRequestContextGetter> request_context_getter(
On 2014/04/21 14:32:43, mmenke wrote:
> Should include ref_counted.h (And may as well add scoped_ptr.h while you're
> doing so)

Done.

https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
File components/captive_portal/captive_portal_testing_utils.h (right):

https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
components/captive_portal/captive_portal_testing_utils.h:20: class URLFetcher;
On 2014/04/21 14:32:43, mmenke wrote:
> There isn't a captive_portal::URLFetcher class...  In fact, we don't seem to
> need this forward declaration at all.

Done.

https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
File components/captive_portal/captive_portal_types.h (right):

https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
components/captive_portal/captive_portal_types.h:26: enum CaptivePortalStatus {
On 2014/04/21 14:32:43, mmenke wrote:
> These don't seem to be used anywhere in the component.  It's also unclear what
> the difference between a status and a result is.

I agree with the second statement, but the intention is to use these elsewhere
in the future (that is in fact my primary motivation for this re-factoring).
This will allow us to store captive portal states in modules without chrome
dependencies (e.g. src/chromeos and src/extensions). 

Eventually I would like to move the CaptivePortalDetector base class here also,
but currently it depends directly on ErrorScreen::Observer and I wanted to limit
the amount of re-factoring in this CL (and ideally done by me since I am not as
familiar with the captive portal code).

[willchan no longer on Chromium, 2014-04-21 17:06:06]

https://codereview.chromium.org/242483003/diff/40001/chrome/browser/captive_p...
File chrome/browser/captive_portal/captive_portal_login_detector.h (right):

https://codereview.chromium.org/242483003/diff/40001/chrome/browser/captive_p...
chrome/browser/captive_portal/captive_portal_login_detector.h:13: namespace
captive_portal {
On 2014/04/21 16:42:27, stevenjb wrote:
> On 2014/04/21 14:32:43, mmenke wrote:
> > Should we be using the namespace both in components/ and in chrome?  Not
> > familiar with what's usually done, just seems odd to me to have a namespace
> span
> > the two projects.
> 
> I've seen this done before (we definitely do it with chromoeos and ash, I feel
> like I've seen it with other libraries). src/components contains the
> non-chrome-specific library, and src/chrome contains the chrome specific
parts.
> I would be OK with removing the namespacing from src/chrome if there is a
strong
> preference for that.

I believe when we discussed this previously on chromium-dev@, we decided not to
use namespaces for chrome/ code. So I think we should only use this namespace
within components.

[mmenke, 2014-04-21 17:46:49]

https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
File components/captive_portal/captive_portal_types.h (right):

https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
components/captive_portal/captive_portal_types.h:26: enum CaptivePortalStatus {
On 2014/04/21 16:42:27, stevenjb wrote:
> On 2014/04/21 14:32:43, mmenke wrote:
> > These don't seem to be used anywhere in the component.  It's also unclear
what
> > the difference between a status and a result is.
> 
> I agree with the second statement, but the intention is to use these elsewhere
> in the future (that is in fact my primary motivation for this re-factoring).
> This will allow us to store captive portal states in modules without chrome
> dependencies (e.g. src/chromeos and src/extensions). 
> 
> Eventually I would like to move the CaptivePortalDetector base class here
also,
> but currently it depends directly on ErrorScreen::Observer and I wanted to
limit
> the amount of re-factoring in this CL (and ideally done by me since I am not
as
> familiar with the captive portal code).

I'm not at all averse to moving them, but I don't they shouldn't be moved until
they're used by the captive portal component.

[stevenjb, 2014-04-21 18:32:59]

On 2014/04/21 17:06:06, willchan wrote:
>
https://codereview.chromium.org/242483003/diff/40001/chrome/browser/captive_p...
> File chrome/browser/captive_portal/captive_portal_login_detector.h (right):
> 
>
https://codereview.chromium.org/242483003/diff/40001/chrome/browser/captive_p...
> chrome/browser/captive_portal/captive_portal_login_detector.h:13: namespace
> captive_portal {
> On 2014/04/21 16:42:27, stevenjb wrote:
> > On 2014/04/21 14:32:43, mmenke wrote:
> > > Should we be using the namespace both in components/ and in chrome?  Not
> > > familiar with what's usually done, just seems odd to me to have a
namespace
> > span
> > > the two projects.
> > 
> > I've seen this done before (we definitely do it with chromoeos and ash, I
feel
> > like I've seen it with other libraries). src/components contains the
> > non-chrome-specific library, and src/chrome contains the chrome specific
> parts.
> > I would be OK with removing the namespacing from src/chrome if there is a
> strong
> > preference for that.
> 
> I believe when we discussed this previously on chromium-dev@, we decided not
to
> use namespaces for chrome/ code. So I think we should only use this namespace
> within components.

True. I eliminated the 'captive_portal' namespace from src/chrome.

[stevenjb, 2014-04-21 19:18:11]

On 2014/04/21 17:46:49, mmenke wrote:
>
https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
> File components/captive_portal/captive_portal_types.h (right):
> 
>
https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
> components/captive_portal/captive_portal_types.h:26: enum CaptivePortalStatus
{
> On 2014/04/21 16:42:27, stevenjb wrote:
> > On 2014/04/21 14:32:43, mmenke wrote:
> > > These don't seem to be used anywhere in the component.  It's also unclear
> what
> > > the difference between a status and a result is.
> > 
> > I agree with the second statement, but the intention is to use these
elsewhere
> > in the future (that is in fact my primary motivation for this re-factoring).
> > This will allow us to store captive portal states in modules without chrome
> > dependencies (e.g. src/chromeos and src/extensions). 
> > 
> > Eventually I would like to move the CaptivePortalDetector base class here
> also,
> > but currently it depends directly on ErrorScreen::Observer and I wanted to
> limit
> > the amount of re-factoring in this CL (and ideally done by me since I am not
> as
> > familiar with the captive portal code).
> 
> I'm not at all averse to moving them, but I don't they shouldn't be moved
until
> they're used by the captive portal component.

So, here's the problem I am looking to solve (which may turn out to be
orthogonal to this cleanup, but I do think we will want CaptivePortaDetector
regardless):
* We want to move networking_private_api*, which includes an API for captive
portal state, to src/extensions
* To do this we will need to either:
a) access NetworkPortalDetector from src/extensions or src/chromeos
b) add a method to ExtensionHostDelegate that returns the captive portal status

My original intention was to do a), which me still might be able to do, but it
turned out to be more re-factoring than I wanted to do, at least initially.

I think that b) is a reasonable compromise, but I wanted to avoid creating
yet-another-portal-state just for the extensions code. Maybe that's the more
correct solution from a layering perspective, but I have had bad experiences
with multiple similar states (e.g. related to login), and I was hoping to avoid
that, which is why I put CaptivePortalStatus in captive_portal_types.h.

So, I can move it back and plan to create a mirrored state in src/extensions,
but my preference would be to leave CaptivePortalStatus in
captive_portal_types.h so that it can be used outside of src/chrome, even if it
isn't currently used anywhere else in src/components/captive_portal.

[mmenke, 2014-04-22 14:56:15]

On 2014/04/21 19:18:11, stevenjb wrote:
> On 2014/04/21 17:46:49, mmenke wrote:
> >
>
https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
> > File components/captive_portal/captive_portal_types.h (right):
> > 
> >
>
https://codereview.chromium.org/242483003/diff/40001/components/captive_porta...
> > components/captive_portal/captive_portal_types.h:26: enum
CaptivePortalStatus
> {
> > On 2014/04/21 16:42:27, stevenjb wrote:
> > > On 2014/04/21 14:32:43, mmenke wrote:
> > > > These don't seem to be used anywhere in the component.  It's also
unclear
> > what
> > > > the difference between a status and a result is.
> > > 
> > > I agree with the second statement, but the intention is to use these
> elsewhere
> > > in the future (that is in fact my primary motivation for this
re-factoring).
> > > This will allow us to store captive portal states in modules without
chrome
> > > dependencies (e.g. src/chromeos and src/extensions). 
> > > 
> > > Eventually I would like to move the CaptivePortalDetector base class here
> > also,
> > > but currently it depends directly on ErrorScreen::Observer and I wanted to
> > limit
> > > the amount of re-factoring in this CL (and ideally done by me since I am
not
> > as
> > > familiar with the captive portal code).
> > 
> > I'm not at all averse to moving them, but I don't they shouldn't be moved
> until
> > they're used by the captive portal component.
> 
> So, here's the problem I am looking to solve (which may turn out to be
> orthogonal to this cleanup, but I do think we will want CaptivePortaDetector
> regardless):
> * We want to move networking_private_api*, which includes an API for captive
> portal state, to src/extensions
> * To do this we will need to either:
> a) access NetworkPortalDetector from src/extensions or src/chromeos
> b) add a method to ExtensionHostDelegate that returns the captive portal
status
> 
> My original intention was to do a), which me still might be able to do, but it
> turned out to be more re-factoring than I wanted to do, at least initially.
> 
> I think that b) is a reasonable compromise, but I wanted to avoid creating
> yet-another-portal-state just for the extensions code. Maybe that's the more
> correct solution from a layering perspective, but I have had bad experiences
> with multiple similar states (e.g. related to login), and I was hoping to
avoid
> that, which is why I put CaptivePortalStatus in captive_portal_types.h.
> 
> So, I can move it back and plan to create a mirrored state in src/extensions,
> but my preference would be to leave CaptivePortalStatus in
> captive_portal_types.h so that it can be used outside of src/chrome, even if
it
> isn't currently used anywhere else in src/components/captive_portal.

I really don't like the intermediate state of putting them here when the
component doesn't use them, or depend on them in any way.

There are three options I prefer:
1) Pass in an opaque string for now.  I assume this isn't a workable solution
for you.
2) Move them into browser/captive_portal until you move the code to generate
them, since they are used by two browser/ classes.
3) (Actually, I prefer this one):  Merge the two enums.  CaptivePortalStatus
actually has more information than CaptivePortalResult, going beyond just
captive portal information.  In fact, I think CaptivePortal is actually a
misnomer here.  If we're planning on moving it into the CaptivePortal code, We
should probably rename everything (including the component) from CaptivePortal
to something along the lines of NetworkState / NetworkStatus /
NetworkDiagnostics / Network[Status]Checker.

[stevenjb, 2014-04-22 15:47:43]

1) I'm not sure what you mean, unless you mean effectively a separate enum
(or set of strings) for the extension API, which is certainly doable, and
may be what I need to do for now.
2) I'm not sure where to put CaptivePortalStatus in the short term since we
want to remove the captive_portal namespace from chrome.
3) I'm not entirely comfortable with re-factoring a class that I only have
a peripheral understanding of.

I hate to ditch the re-factoring entirely since I know we will want it
eventually, so maybe I will do 2) by renaming CaptivePortalStatus to
NetworkportalStatus and embedding it in that class since that is where it
is used. Then (orthogonally) I will define a set of strings to use in the
extension API.


On Tue, Apr 22, 2014 at 7:56 AM, <mmenke@chromium.org> wrote:

> On 2014/04/21 19:18:11, stevenjb wrote:
>
>> On 2014/04/21 17:46:49, mmenke wrote:
>> >
>>
>
> https://codereview.chromium.org/242483003/diff/40001/
> components/captive_portal/captive_portal_types.h
>
>> > File components/captive_portal/captive_portal_types.h (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/242483003/diff/40001/
> components/captive_portal/captive_portal_types.h#newcode26
>
>> > components/captive_portal/captive_portal_types.h:26: enum
>>
> CaptivePortalStatus
>
>> {
>> > On 2014/04/21 16:42:27, stevenjb wrote:
>> > > On 2014/04/21 14:32:43, mmenke wrote:
>> > > > These don't seem to be used anywhere in the component.  It's also
>>
> unclear
>
>> > what
>> > > > the difference between a status and a result is.
>> > >
>> > > I agree with the second statement, but the intention is to use these
>> elsewhere
>> > > in the future (that is in fact my primary motivation for this
>>
> re-factoring).
>
>> > > This will allow us to store captive portal states in modules without
>>
> chrome
>
>> > > dependencies (e.g. src/chromeos and src/extensions).
>> > >
>> > > Eventually I would like to move the CaptivePortalDetector base class
>> here
>> > also,
>> > > but currently it depends directly on ErrorScreen::Observer and I
>> wanted to
>> > limit
>> > > the amount of re-factoring in this CL (and ideally done by me since I
>> am
>>
> not
>
>> > as
>> > > familiar with the captive portal code).
>> >
>> > I'm not at all averse to moving them, but I don't they shouldn't be
>> moved
>> until
>> > they're used by the captive portal component.
>>
>
>  So, here's the problem I am looking to solve (which may turn out to be
>> orthogonal to this cleanup, but I do think we will want
>> CaptivePortaDetector
>> regardless):
>> * We want to move networking_private_api*, which includes an API for
>> captive
>> portal state, to src/extensions
>> * To do this we will need to either:
>> a) access NetworkPortalDetector from src/extensions or src/chromeos
>> b) add a method to ExtensionHostDelegate that returns the captive portal
>>
> status
>
>  My original intention was to do a), which me still might be able to do,
>> but it
>> turned out to be more re-factoring than I wanted to do, at least
>> initially.
>>
>
>  I think that b) is a reasonable compromise, but I wanted to avoid creating
>> yet-another-portal-state just for the extensions code. Maybe that's the
>> more
>> correct solution from a layering perspective, but I have had bad
>> experiences
>> with multiple similar states (e.g. related to login), and I was hoping to
>>
> avoid
>
>> that, which is why I put CaptivePortalStatus in captive_portal_types.h.
>>
>
>  So, I can move it back and plan to create a mirrored state in
>> src/extensions,
>> but my preference would be to leave CaptivePortalStatus in
>> captive_portal_types.h so that it can be used outside of src/chrome, even
>> if
>>
> it
>
>> isn't currently used anywhere else in src/components/captive_portal.
>>
>
> I really don't like the intermediate state of putting them here when the
> component doesn't use them, or depend on them in any way.
>
> There are three options I prefer:
> 1) Pass in an opaque string for now.  I assume this isn't a workable
> solution
> for you.
> 2) Move them into browser/captive_portal until you move the code to
> generate
> them, since they are used by two browser/ classes.
> 3) (Actually, I prefer this one):  Merge the two enums.
>  CaptivePortalStatus
> actually has more information than CaptivePortalResult, going beyond just
> captive portal information.  In fact, I think CaptivePortal is actually a
> misnomer here.  If we're planning on moving it into the CaptivePortal
> code, We
> should probably rename everything (including the component) from
> CaptivePortal
> to something along the lines of NetworkState / NetworkStatus /
> NetworkDiagnostics / Network[Status]Checker.
>
> https://codereview.chromium.org/242483003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmenke, 2014-04-22 16:14:13]

On 2014/04/22 15:47:43, stevenjb wrote:
> 1) I'm not sure what you mean, unless you mean effectively a separate enum
> (or set of strings) for the extension API, which is certainly doable, and
> may be what I need to do for now.
> 2) I'm not sure where to put CaptivePortalStatus in the short term since we
> want to remove the captive_portal namespace from chrome.
> 3) I'm not entirely comfortable with re-factoring a class that I only have
> a peripheral understanding of.
> 
> I hate to ditch the re-factoring entirely since I know we will want it
> eventually, so maybe I will do 2) by renaming CaptivePortalStatus to
> NetworkportalStatus and embedding it in that class since that is where it
> is used. Then (orthogonally) I will define a set of strings to use in the
> extension API.

I'm fine with that.

I think moving captive portal logic into its own component is a good idea, and
definitely support the effort.

[stevenjb, 2014-04-22 21:34:49]

OK, I have reverted the CaptivePortalState changes. Alas, this CL will no longer
help my needs, but should still be good cleanup regardless. Maybe we can look
into moving NetworkPortalDetector to src/chromeos once
https://codereview.chromium.org/245113003/ lands.

PTAL

[mmenke, 2014-04-23 14:56:35]

LGTM

https://codereview.chromium.org/242483003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/network_portal_detector_impl.h (right):

https://codereview.chromium.org/242483003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_portal_detector_impl.h:24: #include
"components/captive_portal/captive_portal_detector.h"
optional:  While you're here, could just forward declare this, and move this
into the cc file, since it's only stored in a scoped_ptr.

https://codereview.chromium.org/242483003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_portal_detector_impl.h:24: #include
"components/captive_portal/captive_portal_detector.h"
Should probably include captive_portal_types.h

[stevenjb, 2014-04-23 19:48:33]

https://codereview.chromium.org/242483003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/network_portal_detector_impl.h (right):

https://codereview.chromium.org/242483003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_portal_detector_impl.h:24: #include
"components/captive_portal/captive_portal_detector.h"
On 2014/04/23 14:56:36, mmenke wrote:
> optional:  While you're here, could just forward declare this, and move this
> into the cc file, since it's only stored in a scoped_ptr.

Actually, we need this for CaptivePortalDetector::Results.

[stevenjb, 2014-04-23 19:50:25]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-04-23 19:51:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/242483003/120001

[commit-bot: I haz the power, 2014-04-23 20:48:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-23 20:48:26]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[stevenjb, 2014-04-25 19:49:56]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-04-25 21:40:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/242483003/140001

[commit-bot: I haz the power, 2014-04-25 23:57:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-25 23:57:36]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[stevenjb, 2014-04-28 21:45:14]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-04-28 21:46:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/242483003/160001

[commit-bot: I haz the power, 2014-04-28 23:39:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 23:39:04]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_x64_rel

[stevenjb, 2014-04-29 18:05:54]

Committed patchset #11 manually as r266920 (presubmit successful).