Cleanup TrayCast with media-router only support in mind.

Cleanup TrayCast with media-router only support in mind.

This allows the TrayCast instance to immediately connect to the CastConfigDelegate, which allows TrayCast to fetch cast data before the tray pops up the first time.

This also allows TrayCast to properly support DIAL casts. A DIAL cast is when the Chromecast / sink stream directly from another source, ie, YouTube.

BUG=653313
Committed: https://crrev.com/e8ea94d6917ade3afc5f7c3164369ef15c3e76c9
Cr-Commit-Position: refs/heads/master@{#432603}

[jdufault, 2016-10-31 20:44:01]

Description was changed from

==========
Cleanup TrayCast with media-router only support in mind.

BUG=653313
==========

to

==========
Cleanup TrayCast with media-router only support in mind.

This allows the TrayCast instance to immediately connect to the
CastConfigDelegate, which allows TrayCast to fetch cast data before the tray
pops up the first time.

This also allows TrayCast to properly support DIAL casts. A DIAL cast is when
the Chromecast / sink stream directly from another source, ie, YouTube.

BUG=653313
==========

[jdufault, 2016-10-31 21:13:01]

jdufault@chromium.org changed reviewers:
+ achuith@chromium.org

[jdufault, 2016-10-31 21:13:14]

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

[jdufault, 2016-10-31 21:13:33]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-10-31 21:13:38]

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-10-31 22:24:37]

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

[commit-bot: I haz the power, 2016-10-31 22:24:38]

Dry run: This issue passed the CQ dry run.

[jdufault, 2016-11-01 19:06:07]

achuith@, PTAL. Thanks!

[achuithb, 2016-11-01 19:44:59]

lgtm.

We don't have any tests for this code, do we?

[jdufault, 2016-11-01 19:46:11]

On 2016/11/01 19:44:59, achuithb wrote:
> lgtm.
> 
> We don't have any tests for this code, do we?

There are some at
https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/system_tray_tray_c...

[achuithb, 2016-11-01 19:48:35]

On 2016/11/01 19:46:11, jdufault wrote:
> On 2016/11/01 19:44:59, achuithb wrote:
> > lgtm.
> > 
> > We don't have any tests for this code, do we?
> 
> There are some at
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/system_tray_tray_c...

Nothing relevant in this CL that could be tested cleanly?

[jdufault, 2016-11-01 19:50:17]

On 2016/11/01 19:48:35, achuithb wrote:
> On 2016/11/01 19:46:11, jdufault wrote:
> > On 2016/11/01 19:44:59, achuithb wrote:
> > > lgtm.
> > > 
> > > We don't have any tests for this code, do we?
> > 
> > There are some at
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/system_tray_tray_c...
> 
> Nothing relevant in this CL that could be tested cleanly?

I don't think so. I'm not sure of a good way to test the profile changes, and
the DIAL support is all abstracted cleanly behind the media router API.

[achuithb, 2016-11-01 19:57:44]

On 2016/11/01 19:50:17, jdufault wrote:
> On 2016/11/01 19:48:35, achuithb wrote:
> > On 2016/11/01 19:46:11, jdufault wrote:
> > > On 2016/11/01 19:44:59, achuithb wrote:
> > > > lgtm.
> > > > 
> > > > We don't have any tests for this code, do we?
> > > 
> > > There are some at
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/system_tray_tray_c...
> > 
> > Nothing relevant in this CL that could be tested cleanly?
> 
> I don't think so. I'm not sure of a good way to test the profile changes, and
> the DIAL support is all abstracted cleanly behind the media router API.

ok

[jdufault, 2016-11-02 19:40:21]

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

[commit-bot: I haz the power, 2016-11-02 19:40:53]

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-11-02 19:45:11]

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

[commit-bot: I haz the power, 2016-11-02 19:45:12]

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...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jdufault, 2016-11-02 21:10:08]

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

[commit-bot: I haz the power, 2016-11-02 21:10:48]

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-11-02 22:50:01]

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

[commit-bot: I haz the power, 2016-11-02 22:50:02]

Dry run: This issue passed the CQ dry run.

[jdufault, 2016-11-10 00:30:02]

Description was changed from

==========
Cleanup TrayCast with media-router only support in mind.

This allows the TrayCast instance to immediately connect to the
CastConfigDelegate, which allows TrayCast to fetch cast data before the tray
pops up the first time.

This also allows TrayCast to properly support DIAL casts. A DIAL cast is when
the Chromecast / sink stream directly from another source, ie, YouTube.

BUG=653313
==========

to

==========
Cleanup TrayCast with media-router only support in mind.

This allows the TrayCast instance to immediately connect to the
CastConfigDelegate, which allows TrayCast to fetch cast data before the tray
pops up the first time.

This also allows TrayCast to properly support DIAL casts. A DIAL cast is when
the Chromecast / sink stream directly from another source, ie, YouTube.

TBR=oshima@chromium.org
BUG=653313
==========

[jdufault, 2016-11-10 00:31:40]

Description was changed from

==========
Cleanup TrayCast with media-router only support in mind.

This allows the TrayCast instance to immediately connect to the
CastConfigDelegate, which allows TrayCast to fetch cast data before the tray
pops up the first time.

This also allows TrayCast to properly support DIAL casts. A DIAL cast is when
the Chromecast / sink stream directly from another source, ie, YouTube.

TBR=oshima@chromium.org
BUG=653313
==========

to

==========
Cleanup TrayCast with media-router only support in mind.

This allows the TrayCast instance to immediately connect to the
CastConfigDelegate, which allows TrayCast to fetch cast data before the tray
pops up the first time.

This also allows TrayCast to properly support DIAL casts. A DIAL cast is when
the Chromecast / sink stream directly from another source, ie, YouTube.

BUG=653313
==========

[jdufault, 2016-11-10 00:31:40]

jdufault@chromium.org changed reviewers:
+ oshima@chromium.org

[jdufault, 2016-11-10 00:31:56]

oshima@, PTAL for owner approval. Thanks!

[oshima, 2016-11-10 18:10:18]

https://codereview.chromium.org/2466043002/diff/60001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/cast_config_delegate_media_router.cc (right):

https://codereview.chromium.org/2466043002/diff/60001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/cast_config_delegate_media_router.cc:228: case
chrome::NOTIFICATION_SESSION_STARTED:
notification is deprecated and it's discouraged to add new usage.
(crbug.com/268984) Can you use LoginState::Observer?

[jdufault, 2016-11-15 00:47:02]

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

[commit-bot: I haz the power, 2016-11-15 00:48:24]

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-11-15 03:24:07]

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

[commit-bot: I haz the power, 2016-11-15 03:24:08]

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

[jdufault, 2016-11-15 19:18:35]

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

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

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-11-15 20:30:42]

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

[commit-bot: I haz the power, 2016-11-15 20:30:43]

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

[jdufault, 2016-11-15 23:19:19]

On 2016/11/10 18:10:18, oshima wrote:
>
https://codereview.chromium.org/2466043002/diff/60001/chrome/browser/ui/ash/c...
> File chrome/browser/ui/ash/cast_config_delegate_media_router.cc (right):
> 
>
https://codereview.chromium.org/2466043002/diff/60001/chrome/browser/ui/ash/c...
> chrome/browser/ui/ash/cast_config_delegate_media_router.cc:228: case
> chrome::NOTIFICATION_SESSION_STARTED:
> notification is deprecated and it's discouraged to add new usage.
> (crbug.com/268984) Can you use LoginState::Observer?

I played around with doing this (see patch set 5), but it looks like the actual
notification dependency is on NOTIFICATION_LOGIN_USER_PROFILE_PREPARED. I don't
think there is an equivalent callback yet. I also looked into adding such a
callback, but user manager / session manager code is currently getting
refactored so I think using the notification for the time being is reasonable;
once the relevant callback is added it should be pretty trivial to switch this
use over.

[jdufault, 2016-11-15 23:19:29]

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

[commit-bot: I haz the power, 2016-11-15 23:19:49]

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-11-15 23:36:19]

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

[commit-bot: I haz the power, 2016-11-15 23:36:19]

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

[oshima, 2016-11-15 23:38:05]

lgtm

https://codereview.chromium.org/2466043002/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/cast_config_delegate_media_router.cc (right):

https://codereview.chromium.org/2466043002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/cast_config_delegate_media_router.cc:227: switch (type) {
can you add TODO + bug to change to use observer/lisntener?

[jdufault, 2016-11-16 19:19:20]

https://codereview.chromium.org/2466043002/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/cast_config_delegate_media_router.cc (right):

https://codereview.chromium.org/2466043002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/cast_config_delegate_media_router.cc:227: switch (type) {
On 2016/11/15 23:38:05, oshima wrote:
> can you add TODO + bug to change to use observer/lisntener?

Done.

[jdufault, 2016-11-16 19:19:25]

The CQ bit was checked by jdufault@chromium.org

[jdufault, 2016-11-16 19:19:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
achuith@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2466043002/#ps140001
(title: "Add TODO")

[commit-bot: I haz the power, 2016-11-16 19:20:36]

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-11-16 19:57:56]

Description was changed from

==========
Cleanup TrayCast with media-router only support in mind.

This allows the TrayCast instance to immediately connect to the
CastConfigDelegate, which allows TrayCast to fetch cast data before the tray
pops up the first time.

This also allows TrayCast to properly support DIAL casts. A DIAL cast is when
the Chromecast / sink stream directly from another source, ie, YouTube.

BUG=653313
==========

to

==========
Cleanup TrayCast with media-router only support in mind.

This allows the TrayCast instance to immediately connect to the
CastConfigDelegate, which allows TrayCast to fetch cast data before the tray
pops up the first time.

This also allows TrayCast to properly support DIAL casts. A DIAL cast is when
the Chromecast / sink stream directly from another source, ie, YouTube.

BUG=653313
==========

[commit-bot: I haz the power, 2016-11-16 19:57:57]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-11-16 20:24:34]

Description was changed from

==========
Cleanup TrayCast with media-router only support in mind.

This allows the TrayCast instance to immediately connect to the
CastConfigDelegate, which allows TrayCast to fetch cast data before the tray
pops up the first time.

This also allows TrayCast to properly support DIAL casts. A DIAL cast is when
the Chromecast / sink stream directly from another source, ie, YouTube.

BUG=653313
==========

to

==========
Cleanup TrayCast with media-router only support in mind.

This allows the TrayCast instance to immediately connect to the
CastConfigDelegate, which allows TrayCast to fetch cast data before the tray
pops up the first time.

This also allows TrayCast to properly support DIAL casts. A DIAL cast is when
the Chromecast / sink stream directly from another source, ie, YouTube.

BUG=653313

Committed: https://crrev.com/e8ea94d6917ade3afc5f7c3164369ef15c3e76c9
Cr-Commit-Position: refs/heads/master@{#432603}
==========

[commit-bot: I haz the power, 2016-11-16 20:24:35]

Patchset 7 (id:??) landed as
https://crrev.com/e8ea94d6917ade3afc5f7c3164369ef15c3e76c9
Cr-Commit-Position: refs/heads/master@{#432603}