Refactor NetworkPortalDetector and NetworkPortalNotificationController.

Refactor NetworkPortalDetector and NetworkPortalNotificationController.

* NetworkPortalNotificationController directly observes network and portal changes.
* conditionally create NetworkPortalNotificationController
* move NetworkPortalDetectorStubImpl to stub files.
* make interface implementations private
* initialize data members in header

BUG=530485
TEST=unit tests and browser tests.

Committed: https://crrev.com/94c1a3dd78fc09ef765a14a200f4347a43c2e32f
Cr-Commit-Position: refs/heads/master@{#350202}

[achuithb, 2015-09-17 19:36:12]

The CQ bit was checked by achuith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-17 19:37:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1346843003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1346843003/1

[achuithb, 2015-09-17 19:41:14]

achuith@chromium.org changed reviewers:
+ stevenjb@chromium.org

[achuithb, 2015-09-17 19:41:14]

Steven, please take a look.

[commit-bot: I haz the power, 2015-09-17 20:06:02]

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

[commit-bot: I haz the power, 2015-09-17 20:06:03]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2015-09-17 20:45:53]

Nice cleanup, just some minor nits / suggestions.

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/network_portal_detector_impl.cc (right):

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_portal_detector_impl.cc:237: void
NetworkPortalDetector::Initialize(
I don't feel strongly about this, but I think this is a bit confusing. Better I
think to put the stub impl in it's own file, and move this code to
network_portal_detector.cc.

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_portal_detector_impl.cc:368: if
(notification_controller_.get())
nit: if (notification_controller_)

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

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_portal_detector_impl.h:75: typedef
base::hash_map<std::string, CaptivePortalState> CaptivePortalStateMap;
nit: WS

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc
(right):

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:131:
g_browser_process->system_request_context(), true);
Add comment to document |true|

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/network_portal_notification_controller.cc
(right):

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:242: if
(NetworkHandler::IsInitialized()) {
This should probably be a DCHECK, it would be bad if we create this before NSH.

[achuithb, 2015-09-17 22:02:35]

The CQ bit was checked by achuith@chromium.org to run a CQ dry run

[achuithb, 2015-09-17 22:03:33]

Thanks for the speedy review! PTAL!

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/network_portal_detector_impl.cc (right):

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_portal_detector_impl.cc:237: void
NetworkPortalDetector::Initialize(
On 2015/09/17 20:45:52, stevenjb wrote:
> I don't feel strongly about this, but I think this is a bit confusing. Better
I
> think to put the stub impl in it's own file, and move this code to
> network_portal_detector.cc.

I can move the stub impl to it's own file, but it's currently small and in the
anonymous namespace, which reduces linker burden somewhat.

I can't move this to network_portal_detector.cc because I would need to include
network_portal_detector_impl.h, which isn't possible because of DEPS
restrictions :(

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_portal_detector_impl.cc:368: if
(notification_controller_.get())
On 2015/09/17 20:45:52, stevenjb wrote:
> nit: if (notification_controller_)

Done.

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

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_portal_detector_impl.h:75: typedef
base::hash_map<std::string, CaptivePortalState> CaptivePortalStateMap;
On 2015/09/17 20:45:52, stevenjb wrote:
> nit: WS

What's WS? I imagine you mean using using? If so, done

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc
(right):

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:131:
g_browser_process->system_request_context(), true);
On 2015/09/17 20:45:52, stevenjb wrote:
> Add comment to document |true|

Done.

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/network_portal_notification_controller.cc
(right):

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:242: if
(NetworkHandler::IsInitialized()) {
On 2015/09/17 20:45:53, stevenjb wrote:
> This should probably be a DCHECK, it would be bad if we create this before
NSH.

It's NULL for unit tests. I've added a comment.

[commit-bot: I haz the power, 2015-09-17 22:03:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1346843003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1346843003/20001

[commit-bot: I haz the power, 2015-09-17 22:50:31]

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

[commit-bot: I haz the power, 2015-09-17 22:50:32]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2015-09-18 22:04:10]

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/network_portal_detector_impl.cc (right):

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_portal_detector_impl.cc:237: void
NetworkPortalDetector::Initialize(
On 2015/09/17 22:03:32, achuithb wrote:
> On 2015/09/17 20:45:52, stevenjb wrote:
> > I don't feel strongly about this, but I think this is a bit confusing.
Better
> I
> > think to put the stub impl in it's own file, and move this code to
> > network_portal_detector.cc.
> 
> I can move the stub impl to it's own file, but it's currently small and in the
> anonymous namespace, which reduces linker burden somewhat.
> 
> I can't move this to network_portal_detector.cc because I would need to
include
> network_portal_detector_impl.h, which isn't possible because of DEPS
> restrictions :(

We can't implement a NetworkPortalDetector method outside of the directory it is
defined in, that breaks all sorts of compontentization rules. Basically, changes
to this method should be reverted, it should continue to be a
NetworkPortalDetectorImpl method.

Also, I really think that the stub impl should be in its own file in
chromeos/network/portal_detector/.

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

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_portal_detector_impl.h:75: typedef
base::hash_map<std::string, CaptivePortalState> CaptivePortalStateMap;
On 2015/09/17 22:03:32, achuithb wrote:
> On 2015/09/17 20:45:52, stevenjb wrote:
> > nit: WS
> 
> What's WS? I imagine you mean using using? If so, done

"White Space" :) There should be a blank line here.

[achuithb, 2015-09-22 17:29:12]

Patchset #3 (id:40001) has been deleted

[achuithb, 2015-09-22 17:31:20]

The CQ bit was checked by achuith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-22 17:32:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1346843003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1346843003/60001

[achuithb, 2015-09-22 17:34:10]

Patchset #3 (id:60001) has been deleted

[achuithb, 2015-09-22 17:34:22]

The CQ bit was checked by achuith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-22 17:34:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1346843003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1346843003/80001

[achuithb, 2015-09-22 17:37:23]

PTAL Steven.

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/network_portal_detector_impl.cc (right):

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_portal_detector_impl.cc:237: void
NetworkPortalDetector::Initialize(
On 2015/09/18 22:04:09, stevenjb wrote:
> On 2015/09/17 22:03:32, achuithb wrote:
> > On 2015/09/17 20:45:52, stevenjb wrote:
> > > I don't feel strongly about this, but I think this is a bit confusing.
> Better
> > I
> > > think to put the stub impl in it's own file, and move this code to
> > > network_portal_detector.cc.
> > 
> > I can move the stub impl to it's own file, but it's currently small and in
the
> > anonymous namespace, which reduces linker burden somewhat.
> > 
> > I can't move this to network_portal_detector.cc because I would need to
> include
> > network_portal_detector_impl.h, which isn't possible because of DEPS
> > restrictions :(
> 
> We can't implement a NetworkPortalDetector method outside of the directory it
is
> defined in, that breaks all sorts of compontentization rules. Basically,
changes
> to this method should be reverted, it should continue to be a
> NetworkPortalDetectorImpl method.
> 
> Also, I really think that the stub impl should be in its own file in
> chromeos/network/portal_detector/.

ok, I've created the stub files and reverted these changes.

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

https://codereview.chromium.org/1346843003/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_portal_detector_impl.h:75: typedef
base::hash_map<std::string, CaptivePortalState> CaptivePortalStateMap;
On 2015/09/18 22:04:09, stevenjb wrote:
> On 2015/09/17 22:03:32, achuithb wrote:
> > On 2015/09/17 20:45:52, stevenjb wrote:
> > > nit: WS
> > 
> > What's WS? I imagine you mean using using? If so, done
> 
> "White Space" :) There should be a blank line here.

Done.

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

https://codereview.chromium.org/1346843003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_portal_detector_impl.h:66: static void
Initialize(net::URLRequestContextGetter* url_context);
The problem is I need to make this go away since it initializes the singleton.
If I create a standalone instance of this class, this method would make no sense
(and it does the wrong thing).

I'd like to move it out elsewhere. Obviously the base class was the wrong idea.
Any suggestions?

[stevenjb, 2015-09-22 17:41:25]

lgtm

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

https://codereview.chromium.org/1346843003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_portal_detector_impl.h:66: static void
Initialize(net::URLRequestContextGetter* url_context);
On 2015/09/22 17:37:23, achuithb wrote:
> The problem is I need to make this go away since it initializes the singleton.
> If I create a standalone instance of this class, this method would make no
sense
> (and it does the wrong thing).
> 
> I'd like to move it out elsewhere. Obviously the base class was the wrong
idea.
> Any suggestions?

I'm not sure I understand the problem. It doesn't need to change for this CL,
does it? Maybe in the context of the next CL it will make more sense to me?

[achuithb, 2015-09-22 17:43:15]

thanks for the review!

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

https://codereview.chromium.org/1346843003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_portal_detector_impl.h:66: static void
Initialize(net::URLRequestContextGetter* url_context);
On 2015/09/22 17:41:25, stevenjb wrote:
> On 2015/09/22 17:37:23, achuithb wrote:
> > The problem is I need to make this go away since it initializes the
singleton.
> > If I create a standalone instance of this class, this method would make no
> sense
> > (and it does the wrong thing).
> > 
> > I'd like to move it out elsewhere. Obviously the base class was the wrong
> idea.
> > Any suggestions?
> 
> I'm not sure I understand the problem. It doesn't need to change for this CL,
> does it? Maybe in the context of the next CL it will make more sense to me?

Acknowledged.

[commit-bot: I haz the power, 2015-09-22 18:06:27]

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

[commit-bot: I haz the power, 2015-09-22 18:06:28]

Dry run: This issue passed the CQ dry run.

[achuithb, 2015-09-22 18:14:56]

The CQ bit was checked by achuith@chromium.org

[commit-bot: I haz the power, 2015-09-22 18:16:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1346843003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1346843003/80001

[achuithb, 2015-09-22 18:32:38]

achuith@chromium.org changed reviewers:
+ kalman@chromium.org

[achuithb, 2015-09-22 18:32:39]

Ben, could I get an owner lgtm?

[achuithb, 2015-09-22 18:33:17]

On 2015/09/22 18:32:39, achuithb wrote:
> Ben, could I get an owner lgtm?

for networking_config_chromeos_apitest_chromeos.cc

[not at google - send to devlin, 2015-09-22 18:34:28]

lgtm but you should add a per-file OWNER for
chrome/browser/extensions/api/networking_config_chromeos_apitest_chromeos.cc, or
better, move it into some directory you're an owner for.

[commit-bot: I haz the power, 2015-09-22 18:34:53]

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

[commit-bot: I haz the power, 2015-09-22 18:34:54]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[achuithb, 2015-09-22 18:35:59]

The CQ bit was checked by achuith@chromium.org

[achuithb, 2015-09-22 18:36:27]

On 2015/09/22 18:34:28, kalman wrote:
> lgtm but you should add a per-file OWNER for
> chrome/browser/extensions/api/networking_config_chromeos_apitest_chromeos.cc,
or
> better, move it into some directory you're an owner for.

I'll send a CL your way

[commit-bot: I haz the power, 2015-09-22 18:36:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1346843003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1346843003/80001

[commit-bot: I haz the power, 2015-09-22 18:45:48]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-09-22 18:47:40]

Patchset 3 (id:??) landed as
https://crrev.com/94c1a3dd78fc09ef765a14a200f4347a43c2e32f
Cr-Commit-Position: refs/heads/master@{#350202}