Remove the (broken and unneeded) 'force_update' option from ServiceWatcher::DiscoverNewDevices.

Remove the (broken and unneeded) 'force_update' option from ServiceWatcher::DiscoverNewDevices.
    
This removes the race condition detailed in https://bugs.chromium.org/p/chromium/issues/detail?id=737224.


BUG=737224
Review-Url: https://codereview.chromium.org/2963613002
Cr-Commit-Position: refs/heads/master@{#483468}
Committed: https://chromium.googlesource.com/chromium/src/+/2479e9a225a5620ab403734eec1f206dc50fb599

[Carlson, 2017-06-27 21:50:59]

justincarlson@chromium.org changed reviewers:
+ gene@chromium.org

[Carlson, 2017-06-27 21:50:59]

After looking at usage and testing I'm reasonably sure this is a sane fix, but I
don't pretend that I understand this code completely, and the test coverage is
minimal, so I'm not super confident this is the right thing.

[Carlson, 2017-06-27 21:51:48]

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

[commit-bot: I haz the power, 2017-06-27 21:52:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Vitaly Buka (NO REVIEWS), 2017-06-27 22:02:55]

On 2017/06/27 21:50:59, Carlson wrote:
> After looking at usage and testing I'm reasonably sure this is a sane fix, but
I
> don't pretend that I understand this code completely, and the test coverage is
> minimal, so I'm not super confident this is the right thing.

This is not going to work as it will return devices which are already offline
and can't reply to request.

[commit-bot: I haz the power, 2017-06-27 22:07:55]

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

[commit-bot: I haz the power, 2017-06-27 22:07:56]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Vitaly Buka (NO REVIEWS), 2017-06-27 22:26:48]

On 2017/06/27 22:02:55, Vitaly Buka (NO REVIEWS) wrote:
> On 2017/06/27 21:50:59, Carlson wrote:
> > After looking at usage and testing I'm reasonably sure this is a sane fix,
but
> I
> > don't pretend that I understand this code completely, and the test coverage
is
> > minimal, so I'm not super confident this is the right thing.
> 
> This is not going to work as it will return devices which are already offline
> and can't reply to request.

Did you try to call DiscoverNewServices(false)? Looks like this is the only
place this flag is consumed.

I believe tests coverage there is quite reasonable, unfortunately test for
DiscoverNewServices(true) is missing.
I don't see why this flag was needed. It was added as-is
https://chromiumcodereview.appspot.com/16272006 and we didn't use it any where.
Later use was added here, not sure why false did not work for them. 
https://cs.chromium.org/chromium/src/chrome/browser/devtools/device/cast_devi...

[Vitaly Buka (NO REVIEWS), 2017-06-27 22:32:41]

Description was changed from

==========
Don't clear existing services_ when forcing a ServiceWatcher update.

This fixes the race condition detailed in
https://bugs.chromium.org/p/chromium/issues/detail?id=737224.

BUG=737224
==========

to

==========
Don't clear existing services_ when forcing a ServiceWatcher update.

This fixes the race condition detailed in
https://bugs.chromium.org/p/chromium/issues/detail?id=737224.

BUG=737224
==========

[Vitaly Buka (NO REVIEWS), 2017-06-27 22:32:41]

vitalybuka@chromium.org changed reviewers:
+ vitalybuka@chromium.org
- gene@chromium.org

[Vitaly Buka (NO REVIEWS), 2017-06-27 22:35:00]

+seantopping
Sean, do you remember why cast uses device_lister_->DiscoverNewDevices(true) ?
Maybe we should removed it just to avoid confusion.

[seantopping, 2017-06-27 22:55:51]

On 2017/06/27 22:35:00, Vitaly Buka (NO REVIEWS) wrote:
> +seantopping
> Sean, do you remember why cast uses device_lister_->DiscoverNewDevices(true) ?
> Maybe we should removed it just to avoid confusion.

We can safely use DiscoverNewDevices(false), I don't remember why I implemented
it that way at the time. It doesn't actually matter since no services exist
before we try to re-discover anyway.

[Carlson, 2017-06-27 23:01:04]

Description was changed from

==========
Don't clear existing services_ when forcing a ServiceWatcher update.

This fixes the race condition detailed in
https://bugs.chromium.org/p/chromium/issues/detail?id=737224.

BUG=737224
==========

to

==========
Remove the (broken and unneeded) 'force_update' option from
ServiceWatcher::DiscoverNewDevices.
    
This removes the race condition detailed in
https://bugs.chromium.org/p/chromium/issues/detail?id=737224.


BUG=737224
==========

[Carlson, 2017-06-27 23:01:24]

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

[commit-bot: I haz the power, 2017-06-27 23:01:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Carlson, 2017-06-27 23:09:43]

On 2017/06/27 22:55:51, seantopping wrote:
> On 2017/06/27 22:35:00, Vitaly Buka (NO REVIEWS) wrote:
> > +seantopping
> > Sean, do you remember why cast uses device_lister_->DiscoverNewDevices(true)
?
> > Maybe we should removed it just to avoid confusion.
> 
> We can safely use DiscoverNewDevices(false), I don't remember why I
implemented
> it that way at the time. It doesn't actually matter since no services exist
> before we try to re-discover anyway.

Roger that, nuking the flag instead.

[commit-bot: I haz the power, 2017-06-27 23:21:33]

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

[commit-bot: I haz the power, 2017-06-27 23:21:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Carlson, 2017-06-27 23:30:02]

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

[commit-bot: I haz the power, 2017-06-27 23:30:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Vitaly Buka (NO REVIEWS), 2017-06-27 23:32:18]

LGTM
Thank you.

BTW. Does this work for your case?

[Vitaly Buka (NO REVIEWS), 2017-06-27 23:33:51]

You have not updated *.mm files. OSX bots should detect that.

[Carlson, 2017-06-27 23:44:33]

On 2017/06/27 23:33:51, Vitaly Buka (NO REVIEWS) wrote:
> You have not updated *.mm files. OSX bots should detect that.

Indeed, fixed now.

[Vitaly Buka (NO REVIEWS), 2017-06-27 23:45:11]

lgtm

[Carlson, 2017-06-27 23:47:08]

justincarlson@chromium.org changed reviewers:
+ apacible@chromium.org, pfeldman@chromium.org

[Carlson, 2017-06-27 23:47:08]

pfeldman@ can you do OWNERS signoff for the fixups in //c/b/devtools?

apacible@ can you do OWNERS signoff for the fixups in //c/b/media/router?

[Carlson, 2017-06-27 23:47:15]

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

[commit-bot: I haz the power, 2017-06-27 23:47:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 00:19:06]

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

[commit-bot: I haz the power, 2017-06-28 00:19:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Carlson, 2017-06-28 00:34:18]

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

[commit-bot: I haz the power, 2017-06-28 00:34:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 01:35:25]

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

[commit-bot: I haz the power, 2017-06-28 01:35:25]

Dry run: This issue passed the CQ dry run.

[apacible, 2017-06-28 07:46:50]

c/b/media/router lgtm

[Carlson, 2017-06-29 18:27:54]

justincarlson@chromium.org changed reviewers:
+ dgozman@chromium.org
- pfeldman@chromium.org

[Carlson, 2017-06-29 18:27:55]

Apparently pfeldman is OOO, dgozman, can you sign off on the callsite fixups in
//c/b/devtools?

[dgozman, 2017-06-29 18:58:51]

devtools lgtm

[Carlson, 2017-06-29 19:02:37]

The CQ bit was checked by justincarlson@chromium.org

[Carlson, 2017-06-29 19:02:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
vitalybuka@chromium.org
Link to the patchset: https://codereview.chromium.org/2963613002/#ps80001
(title: "Fixup yet another mac site revealed by CQ")

[commit-bot: I haz the power, 2017-06-29 19:03:10]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 20:21:50]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498762957872360,
"parent_rev": "fad0f2a4cb07296a0293ccadb9be5e980a517e90", "commit_rev":
"2479e9a225a5620ab403734eec1f206dc50fb599"}

[commit-bot: I haz the power, 2017-06-29 20:22:05]

Description was changed from

==========
Remove the (broken and unneeded) 'force_update' option from
ServiceWatcher::DiscoverNewDevices.
    
This removes the race condition detailed in
https://bugs.chromium.org/p/chromium/issues/detail?id=737224.


BUG=737224
==========

to

==========
Remove the (broken and unneeded) 'force_update' option from
ServiceWatcher::DiscoverNewDevices.
    
This removes the race condition detailed in
https://bugs.chromium.org/p/chromium/issues/detail?id=737224.


BUG=737224

Review-Url: https://codereview.chromium.org/2963613002
Cr-Commit-Position: refs/heads/master@{#483468}
Committed:
https://chromium.googlesource.com/chromium/src/+/2479e9a225a5620ab403734eec1f...
==========

[commit-bot: I haz the power, 2017-06-29 20:22:06]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2479e9a225a5620ab403734eec1f...