- Invoke FinishDiscovery() if no network interfaces are detected.

- Invoke FinishDiscovery() if no network interfaces are detected.
- Send multiple requests per discovery cycle using a timer object.
- Refactor to make it possible to test code with timers.
- Add unit tests for multiple discovery.

BUG=166550, 170250
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181838

[mark a. foltz, 2013-02-07 02:49:09]

Hi Justin, PTAL.

[justinlin, 2013-02-07 09:03:24]

Cool, I think I understand what this does now. Looks good, minor comments, but
I'd like to look at this again since there is some callback/timer schedule
ordering stuff that I want to make sure makes sense. Also, could you please
rebase to HEAD as the side-by-side's are kind of weird right now. Might make it
easier to read. Thanks.

https://codereview.chromium.org/12150002/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/api/dial/dial_service.cc (right):

https://codereview.chromium.org/12150002/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/dial_service.cc:162: // TODO(mfoltz): Add a
net::NetworkChangeNotifier() to listen for connection
This TODO no longer needed.

https://codereview.chromium.org/12150002/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/dial_service.cc:197: if (!ReadSocket()) {
no braces

https://codereview.chromium.org/12150002/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/dial_service.cc:250: if (request_interval_ <=
TimeDelta::FromSeconds(0)) {
Is this check needed? Seems like scheduling a timer at 0 seconds would pretty
much do the same thing.

https://codereview.chromium.org/12150002/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/dial_service.cc:258:
DCHECK(request_timer_.IsRunning());
I'm not sure this DCHECK is valid. Since the DialService can be shutdown
arbitrarily due to network events, it can probably happen that we scheduled a
SocketRead, but we end up being shutdown when this is run.

https://codereview.chromium.org/12150002/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/dial_service.cc:425: list.clear();
This isn't needed as the vector is already initialized to empty, also please
sync to head and rebase as some of this has changed, I think.

[mark a. foltz, 2013-02-07 22:36:55]

Thanks for the review.  Let's chat about the timer stuff since I'm also a little
unclear on the best way to test it.

https://codereview.chromium.org/12150002/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/api/dial/dial_service.cc (right):

https://codereview.chromium.org/12150002/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/dial_service.cc:162: // TODO(mfoltz): Add a
net::NetworkChangeNotifier() to listen for connection
On 2013/02/07 09:03:24, justinlin wrote:
> This TODO no longer needed.

Done.

https://codereview.chromium.org/12150002/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/dial_service.cc:197: if (!ReadSocket()) {
On 2013/02/07 09:03:24, justinlin wrote:
> no braces

Done.

https://codereview.chromium.org/12150002/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/dial_service.cc:250: if (request_interval_ <=
TimeDelta::FromSeconds(0)) {
On 2013/02/07 09:03:24, justinlin wrote:
> Is this check needed? Seems like scheduling a timer at 0 seconds would pretty
> much do the same thing.

Scheduling a zero-delay timer didn't result in a transfer of control flow to the
posted task, so the unit test would exit before any of the posted tasks fired. 
It might be clearer to disable the timers by setting a flag like
DisableTimersForTest()?

https://codereview.chromium.org/12150002/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/dial_service.cc:258:
DCHECK(request_timer_.IsRunning());
On 2013/02/07 09:03:24, justinlin wrote:
> I'm not sure this DCHECK is valid. Since the DialService can be shutdown
> arbitrarily due to network events, it can probably happen that we scheduled a
> SocketRead, but we end up being shutdown when this is run.

Good point.  Removed.

https://codereview.chromium.org/12150002/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/dial_service.cc:425: list.clear();
On 2013/02/07 09:03:24, justinlin wrote:
> This isn't needed as the vector is already initialized to empty, also please
> sync to head and rebase as some of this has changed, I think.

Done.  Will rebase.

[justinlin, 2013-02-07 22:42:05]

https://codereview.chromium.org/12150002/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/api/dial/dial_service.cc (right):

https://codereview.chromium.org/12150002/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/dial_service.cc:250: if (request_interval_ <=
TimeDelta::FromSeconds(0)) {
On 2013/02/07 22:36:55, mark a. foltz wrote:
> On 2013/02/07 09:03:24, justinlin wrote:
> > Is this check needed? Seems like scheduling a timer at 0 seconds would
pretty
> > much do the same thing.
> 
> Scheduling a zero-delay timer didn't result in a transfer of control flow to
the
> posted task, so the unit test would exit before any of the posted tasks fired.

> It might be clearer to disable the timers by setting a flag like
> DisableTimersForTest()?
> 

Ah, if it's for testing, then I think it's possible to test without needing
this. You just need a MessageLoop, and use "RunLoop run_loop;
run_loop.RunUntilIdle();".
If you need to wait for an event, you can also use "run_loop.QuitClosure()" and
run_loop.Run()". I have an example here:
https://code.google.com/p/chromium/codesearch#chrome/src/content/browser/rend...

But, I think, depending on the test, run_loop.RunUntilIdle should be enough.

[mark a. foltz, 2013-02-08 00:04:50]

https://codereview.chromium.org/12150002/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/api/dial/dial_service.cc (right):

https://codereview.chromium.org/12150002/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/dial_service.cc:250: if (request_interval_ <=
TimeDelta::FromSeconds(0)) {
On 2013/02/07 22:42:05, justinlin wrote:
> On 2013/02/07 22:36:55, mark a. foltz wrote:
> > On 2013/02/07 09:03:24, justinlin wrote:
> > > Is this check needed? Seems like scheduling a timer at 0 seconds would
> pretty
> > > much do the same thing.
> > 
> > Scheduling a zero-delay timer didn't result in a transfer of control flow to
> the
> > posted task, so the unit test would exit before any of the posted tasks
fired.
> 
> > It might be clearer to disable the timers by setting a flag like
> > DisableTimersForTest()?
> > 
> 
> Ah, if it's for testing, then I think it's possible to test without needing
> this. You just need a MessageLoop, and use "RunLoop run_loop;
> run_loop.RunUntilIdle();".
> If you need to wait for an event, you can also use "run_loop.QuitClosure()"
and
> run_loop.Run()". I have an example here:
>
https://code.google.com/p/chromium/codesearch#chrome/src/content/browser/rend...
> 
> But, I think, depending on the test, run_loop.RunUntilIdle should be enough.

Thanks for the tip.  I wasn't able to use RunLoop since the socket libraries
expect a MessageLoop with type = TYPE_IO, but RunUntilIdle worked there too.

[mark a. foltz, 2013-02-08 01:04:40]

+Matt for OWNERS review.  
Matt, PTAL when you have a chance.  Thanks!

[justinlin, 2013-02-08 08:56:20]

lgtm

https://codereview.chromium.org/12150002/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/api/dial/dial_service.cc (right):

https://codereview.chromium.org/12150002/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/dial/dial_service.cc:111:
kDialRequestIntervalMillis) +
Align this with the "(" above.

https://codereview.chromium.org/12150002/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/dial/dial_service.cc:225: return;
Don't need this return

https://codereview.chromium.org/12150002/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/dial/dial_service.cc:422:
FOR_EACH_OBSERVER(Observer, observer_list_, OnDiscoveryFinished(this));
Don't need to do this in the CL, but I kinda feel that it's DialRegistry that
should have the observer list, and not DialService. DialRegistry should be the
sole listener to DialService, and clients should attach to DialService, but we
can do that later if you agree.

https://codereview.chromium.org/12150002/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/api/dial/dial_service.h (right):

https://codereview.chromium.org/12150002/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/dial/dial_service.h:167: // NOTE(mfoltz): It would
make this class cleaner to refactor most of the
Move this TODO to the top of this class or somewhere more relevant, maybe above
"StartDiscovery"?

[mark a. foltz, 2013-02-08 22:07:58]

Thanks.

https://codereview.chromium.org/12150002/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/api/dial/dial_service.cc (right):

https://codereview.chromium.org/12150002/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/dial/dial_service.cc:111:
kDialRequestIntervalMillis) +
On 2013/02/08 08:56:20, justinlin wrote:
> Align this with the "(" above.

Done.

https://codereview.chromium.org/12150002/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/dial/dial_service.cc:225: return;
On 2013/02/08 08:56:20, justinlin wrote:
> Don't need this return

Done.

https://codereview.chromium.org/12150002/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/dial/dial_service.cc:422:
FOR_EACH_OBSERVER(Observer, observer_list_, OnDiscoveryFinished(this));
On 2013/02/08 08:56:20, justinlin wrote:
> Don't need to do this in the CL, but I kinda feel that it's DialRegistry that
> should have the observer list, and not DialService. DialRegistry should be the
> sole listener to DialService, and clients should attach to DialService, but we
> can do that later if you agree.

I agree with you that DialService is exclusively for use by DialRegistry at the
moment but it doesn't seem to make a big difference in complexity whether there
is an observer list or a single observer for DialService.

https://codereview.chromium.org/12150002/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/api/dial/dial_service.h (right):

https://codereview.chromium.org/12150002/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/dial/dial_service.h:167: // NOTE(mfoltz): It would
make this class cleaner to refactor most of the
On 2013/02/08 08:56:20, justinlin wrote:
> Move this TODO to the top of this class or somewhere more relevant, maybe
above
> "StartDiscovery"?

Done, and elaborated it a bit.

[justinlin, 2013-02-09 21:55:24]

Hmm, I think you were right about DCHECK. Looks like your test failed on the
asan bots. Try changing the DCHECK in your cl to CHECK, that might fix it.

[justinlin, 2013-02-09 21:56:12]

Also, I'm getting the same mac_asan failures, so I think that's a different
problem, and win_rel is flaky.

[Matt Perry, 2013-02-11 21:43:59]

just skimmed, LGTM

[commit-bot: I haz the power, 2013-02-12 00:59:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mfoltz@chromium.org/12150002/13004

[commit-bot: I haz the power, 2013-02-12 03:46:30]

Change committed as 181838