[Media Router] Notify MRUI of a media status update when it starts observing

[Media Router] Notify MRUI of a media status update when it starts observing

When the browser-side MediaRouteController already existed for a route, new
observers weren't getting notified of media status updates until there were
further updates. This change makes MRUI get notified when it starts observing.

BUG=684638
Review-Url: https://codereview.chromium.org/2874283002
Cr-Commit-Position: refs/heads/master@{#471917}
Committed: https://chromium.googlesource.com/chromium/src/+/f5128acd5e3d64e615280d77f43d685641a21ee0

[takumif, 2017-05-11 17:42:55]

Description was changed from

==========
[Media Router] Notify MRUI with a media status update when it starts observing

BUG=684638
==========

to

==========
[Media Router] Notify MRUI of a media status update when it starts observing

When the browser-side MediaRouteController already existed for a route, new
observers weren't getting notified of media status updates until there were
further updates. This change makes MRUI get notified when it starts observing.

BUG=684638
==========

[takumif, 2017-05-11 17:44:48]

Description was changed from

==========
[Media Router] Notify MRUI of a media status update when it starts observing

When the browser-side MediaRouteController already existed for a route, new
observers weren't getting notified of media status updates until there were
further updates. This change makes MRUI get notified when it starts observing.

BUG=684638
==========

to

==========
[Media Router] Notify MRUI of a media status update when it starts observing

When the browser-side MediaRouteController already existed for a route, new
observers weren't getting notified of media status updates until there were
further updates. This change makes MRUI get notified when it starts observing.

BUG=684638
==========

[takumif, 2017-05-11 17:45:09]

takumif@chromium.org changed reviewers:
+ mfoltz@google.com

[takumif, 2017-05-11 17:45:09]

Please take a look, thanks!

[takumif, 2017-05-11 17:46:25]

takumif@chromium.org changed reviewers:
+ mfoltz@chromium.org
- mfoltz@google.com

[takumif, 2017-05-11 17:46:25]

(Changing to mfoltz@chromium instead of mfoltz@google)

[mark a. foltz, 2017-05-12 21:00:58]

https://codereview.chromium.org/2874283002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/mojo/media_route_controller.h (right):

https://codereview.chromium.org/2874283002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/mojo/media_route_controller.h:49: void
CheckForMediaStatusUpdates();
Is this necessary?

https://codereview.chromium.org/2874283002/diff/1/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2874283002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_ui.cc:161:
CheckForMediaStatusUpdates();
Instead you could call OnMediaStatusUpdated(controller->current_media_status()).

[takumif, 2017-05-12 22:03:52]

Patchset #2 (id:20001) has been deleted

[takumif, 2017-05-12 22:09:49]

Patchset #2 (id:40001) has been deleted

[takumif, 2017-05-12 22:20:58]

https://codereview.chromium.org/2874283002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/mojo/media_route_controller.h (right):

https://codereview.chromium.org/2874283002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/mojo/media_route_controller.h:49: void
CheckForMediaStatusUpdates();
On 2017/05/12 21:00:58, mark a. foltz wrote:
> Is this necessary?

Removed.

https://codereview.chromium.org/2874283002/diff/1/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2874283002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_ui.cc:161:
CheckForMediaStatusUpdates();
On 2017/05/12 21:00:58, mark a. foltz wrote:
> Instead you could call
OnMediaStatusUpdated(controller->current_media_status()).

That would require current_media_status() to be public (and that's why I didn't
do so initially), but it does make things simpler. Done.

[mark a. foltz, 2017-05-12 23:06:19]

lgtm % comments

https://codereview.chromium.org/2874283002/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_route_controller.cc (right):

https://codereview.chromium.org/2874283002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_route_controller.cc:72:
current_media_status_ = base::make_optional<MediaStatus>(MediaStatus(status));
I don't think make_optional adds much here.
It looks like 
current_media_status_ = MediaStatus(status) should suffice (or std::move(status)
if it is move-only).

https://codereview.chromium.org/2874283002/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_route_controller.h (right):

https://codereview.chromium.org/2874283002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_route_controller.h:100: const
base::Optional<MediaStatus>& current_media_status() const {
Document this method

https://codereview.chromium.org/2874283002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2874283002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:161: if
(controller_->current_media_status().has_value())
has_value() is optional here

[takumif, 2017-05-15 17:56:07]

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

[commit-bot: I haz the power, 2017-05-15 17:56:47]

Dry run: CQ is trying da patch.

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

[takumif, 2017-05-15 17:57:29]

Thanks for reviewing!

https://codereview.chromium.org/2874283002/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_route_controller.cc (right):

https://codereview.chromium.org/2874283002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_route_controller.cc:72:
current_media_status_ = base::make_optional<MediaStatus>(MediaStatus(status));
On 2017/05/12 23:06:18, mark a. foltz wrote:
> I don't think make_optional adds much here.
> It looks like 
> current_media_status_ = MediaStatus(status) should suffice (or
std::move(status)
> if it is move-only).
> 

Done.

https://codereview.chromium.org/2874283002/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_route_controller.h (right):

https://codereview.chromium.org/2874283002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_route_controller.h:100: const
base::Optional<MediaStatus>& current_media_status() const {
On 2017/05/12 23:06:18, mark a. foltz wrote:
> Document this method

Done.

https://codereview.chromium.org/2874283002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2874283002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:161: if
(controller_->current_media_status().has_value())
On 2017/05/12 23:06:19, mark a. foltz wrote:
> has_value() is optional here

Removed.

[commit-bot: I haz the power, 2017-05-15 20:40:12]

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

[commit-bot: I haz the power, 2017-05-15 20:40:13]

Dry run: This issue passed the CQ dry run.

[takumif, 2017-05-15 21:52:00]

The CQ bit was checked by takumif@chromium.org

[takumif, 2017-05-15 21:52:00]

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/2874283002/#ps80001
(title: "Address Mark's comments")

[commit-bot: I haz the power, 2017-05-15 21:53:19]

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-15 22:07:14]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494885120049610,
"parent_rev": "8e82a234b7eb4903ec4a0af7f4be01ab4e17999d", "commit_rev":
"f5128acd5e3d64e615280d77f43d685641a21ee0"}

[commit-bot: I haz the power, 2017-05-15 22:07:26]

Description was changed from

==========
[Media Router] Notify MRUI of a media status update when it starts observing

When the browser-side MediaRouteController already existed for a route, new
observers weren't getting notified of media status updates until there were
further updates. This change makes MRUI get notified when it starts observing.

BUG=684638
==========

to

==========
[Media Router] Notify MRUI of a media status update when it starts observing

When the browser-side MediaRouteController already existed for a route, new
observers weren't getting notified of media status updates until there were
further updates. This change makes MRUI get notified when it starts observing.

BUG=684638

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

[commit-bot: I haz the power, 2017-05-15 22:07:27]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/f5128acd5e3d64e615280d77f43d...