mash: Change the MediaDelegate to a Media{Controller,Client}.

mash: Mojoify the MediaDelegate interface.

Instead of a MediaDelegate returned by the ShellDelegate, we now have a
MediaController on the ash side, which receives a MediaClient from the
chrome side at startup.

This also makes minor changes to the interface to send information with
a notification instead of having ash try to read the information back
after notification.

BUG=647409
Committed: https://crrev.com/6a2d7f260d8725344218710b439396c36b8a0f03
Cr-Commit-Position: refs/heads/master@{#438046}

[Elliot Glaysher, 2016-12-08 21:30:27]

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

[commit-bot: I haz the power, 2016-12-08 21:31:37]

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, 2016-12-08 22:54:47]

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

[commit-bot: I haz the power, 2016-12-08 22:54:48]

Dry run: This issue passed the CQ dry run.

[Elliot Glaysher, 2016-12-08 23:27:18]

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

[commit-bot: I haz the power, 2016-12-08 23:27:54]

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, 2016-12-08 23:38:20]

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

[commit-bot: I haz the power, 2016-12-08 23:38:21]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Elliot Glaysher, 2016-12-09 00:56:07]

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

[commit-bot: I haz the power, 2016-12-09 00:56: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, 2016-12-09 01:59:54]

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

[commit-bot: I haz the power, 2016-12-09 01:59:57]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Elliot Glaysher, 2016-12-09 18:45:45]

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

[commit-bot: I haz the power, 2016-12-09 18:46:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2016-12-09 20:14:12]

erg@chromium.org changed reviewers:
+ jamescook@chromium.org

[commit-bot: I haz the power, 2016-12-09 20:41:10]

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

[commit-bot: I haz the power, 2016-12-09 20:41:11]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-12-09 23:42:14]

Can you also expand the CL description a little bit?

https://codereview.chromium.org/2563643003/diff/40001/ash/common/system/user/...
File ash/common/system/user/user_card_view.cc (left):

https://codereview.chromium.org/2563643003/diff/40001/ash/common/system/user/...
ash/common/system/user/user_card_view.cc:51: #if defined(OS_CHROMEOS)
Thanks for cleaning up this stuff.

https://codereview.chromium.org/2563643003/diff/40001/ash/common/system/user/...
File ash/common/system/user/user_card_view.cc (right):

https://codereview.chromium.org/2563643003/diff/40001/ash/common/system/user/...
ash/common/system/user/user_card_view.cc:17: #include
"ash/common/system/tray/system_tray_notifier.h"
not needed?

https://codereview.chromium.org/2563643003/diff/40001/ash/common/system/user/...
ash/common/system/user/user_card_view.cc:108: SetVisible(false);
Is this necessary? I don't see the old code doing it.

https://codereview.chromium.org/2563643003/diff/40001/ash/common/system/user/...
ash/common/system/user/user_card_view.cc:120: mojom::MediaCaptureState state =
capture_states[index_];
DCHECK and/or explicitly exit if the index_ is out of range.

This probably works because Chrome OS cannot sign out individual users -- you
can only sign out all users at once -- so the size of capture_states only goes
up.

https://codereview.chromium.org/2563643003/diff/40001/ash/common/system/user/...
ash/common/system/user/user_card_view.cc:426: mojom::MediaCaptureState state =
capture_states[user_index_];
We need a comment somewhere that this might be racy with switching the current
user. I doubt that will ever happen in practice, and the old code was posting
tasks when media capture started, but it's possible we made it a little worse.

https://codereview.chromium.org/2563643003/diff/40001/ash/public/interfaces/m...
File ash/public/interfaces/media.mojom (right):

https://codereview.chromium.org/2563643003/diff/40001/ash/public/interfaces/m...
ash/public/interfaces/media.mojom:23: // by index of the shell content.
Can you explain the order/index a little more in the comment?

https://codereview.chromium.org/2563643003/diff/40001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/media_delegate_chromeos.cc (right):

https://codereview.chromium.org/2563643003/diff/40001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.cc:41: *media_state_out =
ash::mojom::MediaCaptureState::AUDIO_VIDEO;
This needs to change to accumulate one tab doing audio with one tab doing video
to conclude audio-video.

https://codereview.chromium.org/2563643003/diff/40001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.cc:176:
media_controller_->NotifyCaptureState(std::move(state));
This might be a good place to add a note about the potential race with switching
users.

https://codereview.chromium.org/2563643003/diff/40001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.cc:185:
base::ThreadTaskRunnerHandle::Get()->PostTask(
nit while you're here: Can you add something like this comment (from discussion
on the original CL https://codereview.chromium.org/253183003/): "This is
necessary because the state of
MediaStreamCaptureIndicator gets updated using after this."

https://codereview.chromium.org/2563643003/diff/40001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/media_delegate_chromeos.h (right):

https://codereview.chromium.org/2563643003/diff/40001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.h:14: class MediaDelegateChromeOS
: public ash::mojom::MediaClient,
I think this should be renamed to MediaClient. It could also use a class
comment.

https://codereview.chromium.org/2563643003/diff/40001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.h:33:
ash::mojom::MediaCaptureState GetMediaCaptureStateByIndex(int index);
|user_index| and/or document parameter

[Elliot Glaysher, 2016-12-10 00:11:40]

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

[commit-bot: I haz the power, 2016-12-10 00:12:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2016-12-10 00:20:48]

Description was changed from

==========
mash: Change the MediaDelegate to a Media{Controller,Client}.

BUG=647409
==========

to

==========
mash: Mojoify the MediaDelegate interface.

Instead of a MediaDelegate returned by the ShellDelegate, we now have a
MediaController on the ash side, which receives a MediaClient from the chrome
side at startup.

This also makes minor changes to the interface to send information with
a notification instead of having ash try to read the information back after
notification.

BUG=647409
==========

[Elliot Glaysher, 2016-12-10 00:20:57]

Description was changed from

==========
mash: Mojoify the MediaDelegate interface.

Instead of a MediaDelegate returned by the ShellDelegate, we now have a
MediaController on the ash side, which receives a MediaClient from the chrome
side at startup.

This also makes minor changes to the interface to send information with
a notification instead of having ash try to read the information back after
notification.

BUG=647409
==========

to

==========
mash: Mojoify the MediaDelegate interface.

Instead of a MediaDelegate returned by the ShellDelegate, we now have a
MediaController on the ash side, which receives a MediaClient from the
chrome side at startup.

This also makes minor changes to the interface to send information with
a notification instead of having ash try to read the information back
after notification.

BUG=647409
==========

[Elliot Glaysher, 2016-12-10 00:45:11]

Also updated the description.

https://codereview.chromium.org/2563643003/diff/40001/ash/common/system/user/...
File ash/common/system/user/user_card_view.cc (left):

https://codereview.chromium.org/2563643003/diff/40001/ash/common/system/user/...
ash/common/system/user/user_card_view.cc:51: #if defined(OS_CHROMEOS)
On 2016/12/09 23:42:13, James Cook wrote:
> Thanks for cleaning up this stuff.

Acknowledged.

https://codereview.chromium.org/2563643003/diff/40001/ash/common/system/user/...
File ash/common/system/user/user_card_view.cc (right):

https://codereview.chromium.org/2563643003/diff/40001/ash/common/system/user/...
ash/common/system/user/user_card_view.cc:17: #include
"ash/common/system/tray/system_tray_notifier.h"
On 2016/12/09 23:42:13, James Cook wrote:
> not needed?

Done.

https://codereview.chromium.org/2563643003/diff/40001/ash/common/system/user/...
ash/common/system/user/user_card_view.cc:108: SetVisible(false);
On 2016/12/09 23:42:13, James Cook wrote:
> Is this necessary? I don't see the old code doing it.

Yes. It defaults to visible here, but because RequestCaptureState was
synchronous, the processing of OnMediaCaptureChanged() would previously
immediately set it to false if there was no state.

https://codereview.chromium.org/2563643003/diff/40001/ash/common/system/user/...
ash/common/system/user/user_card_view.cc:120: mojom::MediaCaptureState state =
capture_states[index_];
On 2016/12/09 23:42:13, James Cook wrote:
> DCHECK and/or explicitly exit if the index_ is out of range.
> 
> This probably works because Chrome OS cannot sign out individual users -- you
> can only sign out all users at once -- so the size of capture_states only goes
> up.

Added explicit exit.

https://codereview.chromium.org/2563643003/diff/40001/ash/common/system/user/...
ash/common/system/user/user_card_view.cc:426: mojom::MediaCaptureState state =
capture_states[user_index_];
On 2016/12/09 23:42:13, James Cook wrote:
> We need a comment somewhere that this might be racy with switching the current
> user. I doubt that will ever happen in practice, and the old code was posting
> tasks when media capture started, but it's possible we made it a little worse.

Placed comment in MediaDelegateChromeOS::RequestCaptureState() which is the
source of all the calls to the various OnMediaCaptureChanged().

https://codereview.chromium.org/2563643003/diff/40001/ash/public/interfaces/m...
File ash/public/interfaces/media.mojom (right):

https://codereview.chromium.org/2563643003/diff/40001/ash/public/interfaces/m...
ash/public/interfaces/media.mojom:23: // by index of the shell content.
On 2016/12/09 23:42:13, James Cook wrote:
> Can you explain the order/index a little more in the comment?

Added mini-comment here which redirects to the longer one.

https://codereview.chromium.org/2563643003/diff/40001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/media_delegate_chromeos.cc (right):

https://codereview.chromium.org/2563643003/diff/40001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.cc:41: *media_state_out =
ash::mojom::MediaCaptureState::AUDIO_VIDEO;
On 2016/12/09 23:42:13, James Cook wrote:
> This needs to change to accumulate one tab doing audio with one tab doing
video
> to conclude audio-video.

Changed back to how it was previously, and added an operator|= for
MediaCaptureState.

https://codereview.chromium.org/2563643003/diff/40001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.cc:176:
media_controller_->NotifyCaptureState(std::move(state));
On 2016/12/09 23:42:13, James Cook wrote:
> This might be a good place to add a note about the potential race with
switching
> users.

(This was the place where the big block on user state lives.)

https://codereview.chromium.org/2563643003/diff/40001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.cc:185:
base::ThreadTaskRunnerHandle::Get()->PostTask(
On 2016/12/09 23:42:13, James Cook wrote:
> nit while you're here: Can you add something like this comment (from
discussion
> on the original CL https://codereview.chromium.org/253183003/): "This is
> necessary because the state of
> MediaStreamCaptureIndicator gets updated using after this."

Done.

https://codereview.chromium.org/2563643003/diff/40001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/media_delegate_chromeos.h (right):

https://codereview.chromium.org/2563643003/diff/40001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.h:14: class MediaDelegateChromeOS
: public ash::mojom::MediaClient,
On 2016/12/09 23:42:13, James Cook wrote:
> I think this should be renamed to MediaClient. It could also use a class
> comment.

Good catch. Done.

https://codereview.chromium.org/2563643003/diff/40001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.h:33:
ash::mojom::MediaCaptureState GetMediaCaptureStateByIndex(int index);
On 2016/12/09 23:42:14, James Cook wrote:
> |user_index| and/or document parameter

Done.

[commit-bot: I haz the power, 2016-12-10 02:13:54]

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

[commit-bot: I haz the power, 2016-12-10 02:13:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[James Cook, 2016-12-12 16:54:46]

LGTM!

https://codereview.chromium.org/2563643003/diff/60001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/media_client.cc (right):

https://codereview.chromium.org/2563643003/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_client.cc:168: // TODO(erg): Ash don't have stable
user indexes. Given the asynchronous
super nit: don't -> doesn't

https://codereview.chromium.org/2563643003/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_client.cc:191: // gets updated using after this.
super nit: -using

[Elliot Glaysher, 2016-12-12 19:17:50]

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

[Elliot Glaysher, 2016-12-12 19:19:00]

erg@chromium.org changed reviewers:
+ sky@chromium.org, tsepez@chromium.org

[Elliot Glaysher, 2016-12-12 19:19:01]

sky: owners stamp
tsepez: mojom

[commit-bot: I haz the power, 2016-12-12 19:19:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2016-12-12 19:40:23]

mojom LGTM, the flow is ash -> client.

[sky, 2016-12-12 21:05:02]

LGTM

[Elliot Glaysher, 2016-12-12 21:56:01]

The CQ bit was unchecked by erg@chromium.org

[Elliot Glaysher, 2016-12-12 21:56:07]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2016-12-12 21:56:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2563643003/#ps80001
(title: "Comment fixes")

[commit-bot: I haz the power, 2016-12-12 21:56:35]

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, 2016-12-13 01:54:44]

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

[commit-bot: I haz the power, 2016-12-13 01:54:46]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Elliot Glaysher, 2016-12-13 03:12:01]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2016-12-13 03:12:42]

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, 2016-12-13 03:54:34]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481598721757600,
"parent_rev": "fabfe51366d0ed75649495135316a2437888ec74", "commit_rev":
"19a809de7f145282baa4594c480bdadb4d6dfbdc"}

[commit-bot: I haz the power, 2016-12-13 03:55:10]

Description was changed from

==========
mash: Mojoify the MediaDelegate interface.

Instead of a MediaDelegate returned by the ShellDelegate, we now have a
MediaController on the ash side, which receives a MediaClient from the
chrome side at startup.

This also makes minor changes to the interface to send information with
a notification instead of having ash try to read the information back
after notification.

BUG=647409
==========

to

==========
mash: Mojoify the MediaDelegate interface.

Instead of a MediaDelegate returned by the ShellDelegate, we now have a
MediaController on the ash side, which receives a MediaClient from the
chrome side at startup.

This also makes minor changes to the interface to send information with
a notification instead of having ash try to read the information back
after notification.

BUG=647409

Review-Url: https://codereview.chromium.org/2563643003
==========

[commit-bot: I haz the power, 2016-12-13 03:55:12]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-13 03:57:39]

Description was changed from

==========
mash: Mojoify the MediaDelegate interface.

Instead of a MediaDelegate returned by the ShellDelegate, we now have a
MediaController on the ash side, which receives a MediaClient from the
chrome side at startup.

This also makes minor changes to the interface to send information with
a notification instead of having ash try to read the information back
after notification.

BUG=647409

Review-Url: https://codereview.chromium.org/2563643003
==========

to

==========
mash: Mojoify the MediaDelegate interface.

Instead of a MediaDelegate returned by the ShellDelegate, we now have a
MediaController on the ash side, which receives a MediaClient from the
chrome side at startup.

This also makes minor changes to the interface to send information with
a notification instead of having ash try to read the information back
after notification.

BUG=647409

Committed: https://crrev.com/6a2d7f260d8725344218710b439396c36b8a0f03
Cr-Commit-Position: refs/heads/master@{#438046}
==========

[commit-bot: I haz the power, 2016-12-13 03:57:41]

Patchset 5 (id:??) landed as
https://crrev.com/6a2d7f260d8725344218710b439396c36b8a0f03
Cr-Commit-Position: refs/heads/master@{#438046}