Remove Chromecast extension support from cast system tray menu.

Remove Chromecast extension support from cast system tray menu.

This CL does not contain any behavior changes.

BUG=521127
Committed: https://crrev.com/db5e5f49f81273ba1af09b6a9f7277797f4726c7
Cr-Commit-Position: refs/heads/master@{#430747}

[jdufault, 2016-10-17 20:41:20]

Description was changed from

==========
WIP: Remove Chromecast extension support

BUG=
==========

to

==========
WIP: Remove Chromecast extension support from cast system tray menu

BUG=
==========

[jdufault, 2016-10-26 00:19:55]

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

[commit-bot: I haz the power, 2016-10-26 00:20:36]

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-26 01:08:43]

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

[commit-bot: I haz the power, 2016-10-26 01:08:44]

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-10-26 21:18:26]

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

[commit-bot: I haz the power, 2016-10-26 21:19:47]

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

[jdufault, 2016-10-26 21:20:45]

Description was changed from

==========
WIP: Remove Chromecast extension support from cast system tray menu

BUG=
==========

to

==========
Remove Chromecast extension support from cast system tray menu.

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.

BUG=653313,521127
==========

[jdufault, 2016-10-26 21:22:09]

Description was changed from

==========
Remove Chromecast extension support from cast system tray menu.

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.

BUG=653313,521127
==========

to

==========
Remove Chromecast extension support from cast system tray menu.

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,521127
==========

[jdufault, 2016-10-26 21:45:06]

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

[commit-bot: I haz the power, 2016-10-26 21:45:25]

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-26 22:38:53]

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

[commit-bot: I haz the power, 2016-10-26 22:38:54]

Dry run: This issue passed the CQ dry run.

[jdufault, 2016-10-26 22:45:55]

Patchset #5 (id:80001) has been deleted

[jdufault, 2016-10-26 22:46:03]

Patchset #3 (id:40001) has been deleted

[jdufault, 2016-10-26 22:46:13]

Patchset #2 (id:20001) has been deleted

[jdufault, 2016-10-26 22:46:21]

Patchset #1 (id:1) has been deleted

[jdufault, 2016-10-26 22:51:25]

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

[jdufault, 2016-10-26 22:51:41]

achuith@, PTAL when you get a chance. Thanks!

[achuithb, 2016-10-27 19:39:00]

This CL has some renaming, some deletion of the now-unused extension code, and
some behavior change. How easy would it be to move the behavior change parts to
a separate CL? 

I can see a future bug being traced to this CL, and the headache of having to
comb through it to figure out what's causing the regression.

https://codereview.chromium.org/2424183002/diff/60001/ash/common/cast_config_...
File ash/common/cast_config_delegate.h (right):

https://codereview.chromium.org/2424183002/diff/60001/ash/common/cast_config_...
ash/common/cast_config_delegate.h:78: virtual void CastToReceiver(const Sink&
sink) = 0;
Should this be CastToSink, or is this name still ok?

https://codereview.chromium.org/2424183002/diff/60001/ash/common/system/cast/...
File ash/common/system/cast/tray_cast.cc (right):

https://codereview.chromium.org/2424183002/diff/60001/ash/common/system/cast/...
ash/common/system/cast/tray_cast.cc:49: if (!delegate)
Seems like a good place for the ternary operator:
return delegate ? delegate->GetCastConfigDelegate() : nullptr;

https://codereview.chromium.org/2424183002/diff/60001/ash/common/system/cast/...
ash/common/system/cast/tray_cast.cc:242: break;
I believe 
default:
  NOTREACHED() << "unhandled source";
is customary?

https://codereview.chromium.org/2424183002/diff/60001/ash/common/system/cast/...
ash/common/system/cast/tray_cast.cc:524: if (cast_config_delegate) {
When is this not true?

https://codereview.chromium.org/2424183002/diff/60001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (left):

https://codereview.chromium.org/2424183002/diff/60001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
RIP :'(

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

https://codereview.chromium.org/2424183002/diff/60001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/cast_config_delegate_media_router.cc:35:
ProfileManager::GetPrimaryUserProfile());
Is there still a problem with this and multi-profile? The extension code had a
TODO about this.

https://codereview.chromium.org/2424183002/diff/60001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/cast_config_delegate_media_router.cc:112: // b/28691645.
Not sure about this internal bug link. Wonder if it's worthwhile to create a
crbug with some details, and then refer to the internal bug anyway.

https://codereview.chromium.org/2424183002/diff/60001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/cast_config_delegate_media_router.cc:227: switch (type) {
Is this a change in behavior?

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

https://codereview.chromium.org/2424183002/diff/60001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/cast_config_delegate_media_router.h:21: class
CastConfigDelegateMediaRouter : public ash::CastConfigDelegate,
We can now fold this delegate into CastConfigDelegate, right? Or do we still
need this for dependency injection?

[jdufault, 2016-10-31 20:39:03]

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

[commit-bot: I haz the power, 2016-10-31 20:39:35]

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 20:43:22]

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

[commit-bot: I haz the power, 2016-10-31 20:43:23]

Dry run: Try jobs failed on following builders:
  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-10-31 20:43:34]

Description was changed from

==========
Remove Chromecast extension support from cast system tray menu.

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,521127
==========

to

==========
Remove Chromecast extension support from cast system tray menu.

BUG=521127
==========

[jdufault, 2016-10-31 21:06:55]

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

[commit-bot: I haz the power, 2016-10-31 21:07:25]

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

[jdufault, 2016-10-31 21:12:08]

I've split this up into three CLs.

- This one removes the extension cast support code.
- One for renaming types.
- One for behavior changes.

https://codereview.chromium.org/2424183002/diff/60001/ash/common/cast_config_...
File ash/common/cast_config_delegate.h (right):

https://codereview.chromium.org/2424183002/diff/60001/ash/common/cast_config_...
ash/common/cast_config_delegate.h:78: virtual void CastToReceiver(const Sink&
sink) = 0;
On 2016/10/27 19:38:59, achuithb wrote:
> Should this be CastToSink, or is this name still ok?

Done.

https://codereview.chromium.org/2424183002/diff/60001/ash/common/system/cast/...
File ash/common/system/cast/tray_cast.cc (right):

https://codereview.chromium.org/2424183002/diff/60001/ash/common/system/cast/...
ash/common/system/cast/tray_cast.cc:49: if (!delegate)
On 2016/10/27 19:38:59, achuithb wrote:
> Seems like a good place for the ternary operator:
> return delegate ? delegate->GetCastConfigDelegate() : nullptr;

Done.

https://codereview.chromium.org/2424183002/diff/60001/ash/common/system/cast/...
ash/common/system/cast/tray_cast.cc:242: break;
On 2016/10/27 19:38:59, achuithb wrote:
> I believe 
> default:
>   NOTREACHED() << "unhandled source";
> is customary?

Not having default means the compiler will emit a warning if an enum entry is
added to ContentSource but not handled here.

It looks like this is allowed by the style guide.

https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements

https://codereview.chromium.org/2424183002/diff/60001/ash/common/system/cast/...
ash/common/system/cast/tray_cast.cc:524: if (cast_config_delegate) {
On 2016/10/27 19:38:59, achuithb wrote:
> When is this not true?

Some unrelated tests do not fully initialize everything.

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

https://codereview.chromium.org/2424183002/diff/60001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/cast_config_delegate_media_router.cc:35:
ProfileManager::GetPrimaryUserProfile());
On 2016/10/27 19:38:59, achuithb wrote:
> Is there still a problem with this and multi-profile? The extension code had a
> TODO about this.

Nope, media router doesn't work very well with multi-profile though.

https://codereview.chromium.org/2424183002/diff/60001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/cast_config_delegate_media_router.cc:112: // b/28691645.
On 2016/10/27 19:38:59, achuithb wrote:
> Not sure about this internal bug link. Wonder if it's worthwhile to create a
> crbug with some details, and then refer to the internal bug anyway.

Done.

https://codereview.chromium.org/2424183002/diff/60001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/cast_config_delegate_media_router.cc:227: switch (type) {
On 2016/10/27 19:38:59, achuithb wrote:
> Is this a change in behavior?

Yea, this behavior works a bit better though and fixes some issues (cast data is
available without opening tray).

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

https://codereview.chromium.org/2424183002/diff/60001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/cast_config_delegate_media_router.h:21: class
CastConfigDelegateMediaRouter : public ash::CastConfigDelegate,
On 2016/10/27 19:38:59, achuithb wrote:
> We can now fold this delegate into CastConfigDelegate, right? Or do we still
> need this for dependency injection?

Right, this is still used for dependency injection.

[jdufault, 2016-10-31 21:12:27]

Description was changed from

==========
Remove Chromecast extension support from cast system tray menu.

BUG=521127
==========

to

==========
Remove Chromecast extension support from cast system tray menu.

This CL does not contain any behavior changes.

BUG=521127
==========

[commit-bot: I haz the power, 2016-10-31 22:21:35]

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

[commit-bot: I haz the power, 2016-10-31 22:21:36]

Dry run: This issue passed the CQ dry run.

[jdufault, 2016-11-02 18:27:26]

Ping

[achuithb, 2016-11-02 19:22:02]

lgtm.

Sorry, missed this one

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

https://codereview.chromium.org/2424183002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:150: :
user_profile_(NULL),
If you could move the initialization in the .h, either in this CL or a follow-up
CL

[jdufault, 2016-11-02 19:37:46]

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

https://codereview.chromium.org/2424183002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:150: :
user_profile_(NULL),
On 2016/11/02 19:22:02, achuithb wrote:
> If you could move the initialization in the .h, either in this CL or a
follow-up
> CL

Done.

[jdufault, 2016-11-02 19:37:53]

The CQ bit was checked by jdufault@chromium.org

[jdufault, 2016-11-02 19:37:54]

The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2424183002/#ps140001
(title: "Address comments")

[commit-bot: I haz the power, 2016-11-02 19:38: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-02 19:41:37]

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

[commit-bot: I haz the power, 2016-11-02 19:41:39]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jdufault, 2016-11-02 20:58:07]

The CQ bit was checked by jdufault@chromium.org

[jdufault, 2016-11-02 20:58:07]

The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2424183002/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2016-11-02 20:58:56]

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 21:06:51]

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

[commit-bot: I haz the power, 2016-11-02 21:06:52]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jdufault, 2016-11-02 21:49:23]

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

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

[jdufault, 2016-11-02 22:10:54]

jdufault@chromium.org changed reviewers:
+ oshima@chromium.org, rdevlin.cronin@chromium.org

[jdufault, 2016-11-02 22:13:36]

rdcronin@ PTAL at extension API removal / if I need to remove anything else.

oshima@ for FYI on ash/ changes.

There are no behavior changes in this CL.

Thanks!

[oshima, 2016-11-02 22:39:41]

ash lgtm

[commit-bot: I haz the power, 2016-11-02 23:34:24]

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

[commit-bot: I haz the power, 2016-11-02 23:34:25]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-11-05 06:31:34]

extensions lgtm assuming the others are still needed.

Thanks for the cleanup!

https://codereview.chromium.org/2424183002/diff/180001/chrome/common/extensio...
File chrome/common/extensions/api/_api_features.json (left):

https://codereview.chromium.org/2424183002/diff/180001/chrome/common/extensio...
chrome/common/extensions/api/_api_features.json:184: "cast.channel": {
To confirm, we still need all the rest of these?

[jdufault, 2016-11-08 20:15:59]

https://codereview.chromium.org/2424183002/diff/180001/chrome/common/extensio...
File chrome/common/extensions/api/_api_features.json (left):

https://codereview.chromium.org/2424183002/diff/180001/chrome/common/extensio...
chrome/common/extensions/api/_api_features.json:184: "cast.channel": {
On 2016/11/05 06:31:34, Devlin (slow) wrote:
> To confirm, we still need all the rest of these?

I'll reach out to the owners and see if we can remove it in a separate CL.

[jdufault, 2016-11-08 20:16:06]

The CQ bit was checked by jdufault@chromium.org

[jdufault, 2016-11-08 20:16:06]

The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2424183002/#ps180001
(title: "Additional removals")

[commit-bot: I haz the power, 2016-11-08 20:16:49]

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-08 21:57:28]

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

[commit-bot: I haz the power, 2016-11-08 21:57:30]

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...)

[jdufault, 2016-11-08 22:07:59]

The CQ bit was checked by jdufault@chromium.org

[commit-bot: I haz the power, 2016-11-08 22:08:30]

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-08 23:12:30]

Description was changed from

==========
Remove Chromecast extension support from cast system tray menu.

This CL does not contain any behavior changes.

BUG=521127
==========

to

==========
Remove Chromecast extension support from cast system tray menu.

This CL does not contain any behavior changes.

BUG=521127
==========

[commit-bot: I haz the power, 2016-11-08 23:12:32]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2016-11-08 23:21:39]

Description was changed from

==========
Remove Chromecast extension support from cast system tray menu.

This CL does not contain any behavior changes.

BUG=521127
==========

to

==========
Remove Chromecast extension support from cast system tray menu.

This CL does not contain any behavior changes.

BUG=521127

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

[commit-bot: I haz the power, 2016-11-08 23:21:41]

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