[Media Router] Sync state when MR extension is (re)connected to MR.

[Media Router] Sync state when MR extension is (re)connected to MR.

The extension might have become out of sync with MediaRouter due to one
of few reasons:
(1) The extension crashed and lost unpersisted changes.
(2) The extension was updated; temporary data is cleared.
(3) The extension has an unforseen bug which causes temporary data to be
    persisted incorrectly on suspension.

The number of calls made for the state sync should be relatively small
(< 10), and quite inexpensive since the extension is already up. Note
that the calls must be (and are) idempotent; the extension will no-op
if the query / observer being registered already exists.

Note that the existing mDNS activation call is also considered part of
state sync.

BUG=717325
Review-Url: https://codereview.chromium.org/2855473004
Cr-Commit-Position: refs/heads/master@{#469067}
Committed: https://chromium.googlesource.com/chromium/src/+/adb75ff0ecd96123cfeab63e9ab2bfc368e29f05

[imcheng, 2017-05-02 00:29:04]

imcheng@chromium.org changed reviewers:
+ mfoltz@chromium.org

[imcheng, 2017-05-02 00:29:04]

Mark: PTAL thanks!

[imcheng, 2017-05-02 22:10:55]

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

[commit-bot: I haz the power, 2017-05-02 22:11:32]

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-05-02 23:56:27]

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

[commit-bot: I haz the power, 2017-05-02 23:56:28]

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

[imcheng, 2017-05-03 00:17:24]

Patchset #1 (id:1) has been deleted

[imcheng, 2017-05-03 00:17:34]

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

[commit-bot: I haz the power, 2017-05-03 00:18:17]

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-05-03 01:09:36]

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

[commit-bot: I haz the power, 2017-05-03 01:09:37]

Dry run: This issue passed the CQ dry run.

[mark a. foltz, 2017-05-03 17:37:09]

LGTM

Nice change, a couple of bikeshed comments on naming.

https://codereview.chromium.org/2855473004/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/2855473004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:996:
DoEnsureMdnsDiscoveryEnabled();
Would prefer DoEnableMdnsDiscovery(), but not necessary to change in this patch.

https://codereview.chromium.org/2855473004/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right):

https://codereview.chromium.org/2855473004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.h:313: //
|media_route_provider_| is a valid handle.
Logically, this should be kicked off in RegisterMediaRouteProvider().  Won't the
handle be guaranteed to be valid then?

https://codereview.chromium.org/2855473004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.h:320: void
EnsureMediaRouteProviderStateSynced();
Can this be a verb, i.e. SyncToMediaRouteProvider() ?

https://codereview.chromium.org/2855473004/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc
(right):

https://codereview.chromium.org/2855473004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1066: class
NoopMessageObserver : public RouteMessageObserver {
s/Noop/Fake/ (or maybe s/Noop/Null/) to follow the usual terminology.

http://stackoverflow.com/questions/346372/whats-the-difference-between-faking...

[imcheng, 2017-05-03 18:22:42]

https://codereview.chromium.org/2855473004/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/2855473004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:996:
DoEnsureMdnsDiscoveryEnabled();
On 2017/05/03 17:37:08, mark a. foltz wrote:
> Would prefer DoEnableMdnsDiscovery(), but not necessary to change in this
patch.

Acknowledged.

https://codereview.chromium.org/2855473004/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right):

https://codereview.chromium.org/2855473004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.h:313: //
|media_route_provider_| is a valid handle.
On 2017/05/03 17:37:08, mark a. foltz wrote:
> Logically, this should be kicked off in RegisterMediaRouteProvider().  Won't
the
> handle be guaranteed to be valid then?

Correct. In other words, having a valid MRP handle is a precondition of this
method.

https://codereview.chromium.org/2855473004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.h:320: void
EnsureMediaRouteProviderStateSynced();
On 2017/05/03 17:37:08, mark a. foltz wrote:
> Can this be a verb, i.e. SyncToMediaRouteProvider() ?

Renamed to SyncStateToMediaRouteProvider.

https://codereview.chromium.org/2855473004/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc
(right):

https://codereview.chromium.org/2855473004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1066: class
NoopMessageObserver : public RouteMessageObserver {
On 2017/05/03 17:37:08, mark a. foltz wrote:
> s/Noop/Fake/ (or maybe s/Noop/Null/) to follow the usual terminology.
> 
>
http://stackoverflow.com/questions/346372/whats-the-difference-between-faking...

Done.

[imcheng, 2017-05-03 18:23:29]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2017-05-03 18:23:29]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org
Link to the patchset: https://codereview.chromium.org/2855473004/#ps40001
(title: "Addressed Mark's comments")

[commit-bot: I haz the power, 2017-05-03 18:24:28]

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-05-03 19:20:33]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493835809473660,
"parent_rev": "f5398c73a39f83748f31caf1c9e7747540e94b8b", "commit_rev":
"adb75ff0ecd96123cfeab63e9ab2bfc368e29f05"}

[commit-bot: I haz the power, 2017-05-03 19:21:32]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493835809473660,
"parent_rev": "f5398c73a39f83748f31caf1c9e7747540e94b8b", "commit_rev":
"adb75ff0ecd96123cfeab63e9ab2bfc368e29f05"}

[commit-bot: I haz the power, 2017-05-03 19:21:48]

Description was changed from

==========
[Media Router] Sync state when MR extension is (re)connected to MR.

The extension might have become out of sync with MediaRouter due to one
of few reasons:
(1) The extension crashed and lost unpersisted changes.
(2) The extension was updated; temporary data is cleared.
(3) The extension has an unforseen bug which causes temporary data to be
    persisted incorrectly on suspension.

The number of calls made for the state sync should be relatively small
(< 10), and quite inexpensive since the extension is already up. Note
that the calls must be (and are) idempotent; the extension will no-op
if the query / observer being registered already exists.

Note that the existing mDNS activation call is also considered part of
state sync.

BUG=717325
==========

to

==========
[Media Router] Sync state when MR extension is (re)connected to MR.

The extension might have become out of sync with MediaRouter due to one
of few reasons:
(1) The extension crashed and lost unpersisted changes.
(2) The extension was updated; temporary data is cleared.
(3) The extension has an unforseen bug which causes temporary data to be
    persisted incorrectly on suspension.

The number of calls made for the state sync should be relatively small
(< 10), and quite inexpensive since the extension is already up. Note
that the calls must be (and are) idempotent; the extension will no-op
if the query / observer being registered already exists.

Note that the existing mDNS activation call is also considered part of
state sync.

BUG=717325

Review-Url: https://codereview.chromium.org/2855473004
Cr-Commit-Position: refs/heads/master@{#469067}
Committed:
https://chromium.googlesource.com/chromium/src/+/adb75ff0ecd96123cfeab63e9ab2...
==========

[commit-bot: I haz the power, 2017-05-03 19:21:50]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/adb75ff0ecd96123cfeab63e9ab2...