mash: Change CastConfigDelegate to a mojoified CastConfigClient.

mash: Change CastConfigDelegate to a mojoified CastConfigClient.

BUG=665074
Committed: https://crrev.com/4ff45be6d3af36828f1042a839cbb5a6a60f6563
Cr-Commit-Position: refs/heads/master@{#436107}

[Elliot Glaysher, 2016-11-22 23:38:59]

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

[commit-bot: I haz the power, 2016-11-22 23:39: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, 2016-11-22 23:44:08]

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

[commit-bot: I haz the power, 2016-11-22 23:44:09]

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

[Elliot Glaysher, 2016-11-22 23:50:30]

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

[commit-bot: I haz the power, 2016-11-22 23:51:11]

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-23 00:37:05]

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

[commit-bot: I haz the power, 2016-11-23 00:37:06]

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

[Elliot Glaysher, 2016-11-23 00:40:22]

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

[commit-bot: I haz the power, 2016-11-23 00:41:10]

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-23 00:46:04]

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

[commit-bot: I haz the power, 2016-11-23 00:46:05]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  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_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Elliot Glaysher, 2016-11-23 00:53:43]

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

[commit-bot: I haz the power, 2016-11-23 00:54: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-11-23 02:02:47]

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

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

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

[Elliot Glaysher, 2016-11-28 18:52:33]

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

[commit-bot: I haz the power, 2016-11-28 18:53:26]

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-28 19:43:44]

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

[commit-bot: I haz the power, 2016-11-28 19:43:44]

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

[Elliot Glaysher, 2016-11-28 22:12:48]

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

[commit-bot: I haz the power, 2016-11-28 22:13:56]

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-11-29 00:22:12]

Description was changed from

==========
mash: Change CastConfigDelegate to a mojoified CastConfigClient.

[xxx: not yet ready for review.]

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

to

==========
mash: Change CastConfigDelegate to a mojoified CastConfigClient.

BUG=665074
==========

[Elliot Glaysher, 2016-11-29 00:22:12]

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

[Elliot Glaysher, 2016-11-29 00:58:35]

Hold up on the review, the test failures on linux_chromium_chromeos_rel_ng look
real:

[23226:23226:1128/161211:FATAL:file_system_apitest_chromeos.cc(155)] Check
failed: fake_drive_service_ == NULL. 
#0 0x0000032c969e base::debug::StackTrace::StackTrace()
#1 0x0000032e0a7a logging::LogMessage::~LogMessage()
#2 0x0000009226bc
extensions::FileSystemApiTestForDrive::CreateDriveIntegrationService()
#3 0x0000017ebaf6
drive::DriveIntegrationServiceFactory::BuildServiceInstanceFor()
#4 0x000005677420 BrowserContextKeyedServiceFactory::BuildServiceInstanceFor()
#5 0x00000506a4a1 KeyedServiceFactory::GetServiceForContext()
#6 0x000001827b71 file_manager::VolumeManagerFactory::BuildServiceInstanceFor()
#7 0x000005677420 BrowserContextKeyedServiceFactory::BuildServiceInstanceFor()
#8 0x00000506a4a1 KeyedServiceFactory::GetServiceForContext()
#9 0x00000506b70b DependencyManager::CreateContextServices()
#10 0x000005676ff3
BrowserContextDependencyManager::DoCreateBrowserContextServices()
#11 0x0000034b3428 ProfileImpl::OnLocaleReady()
#12 0x0000034b0f38 ProfileImpl::OnPrefsLoaded()
#13 0x0000034b0d5f ProfileImpl::ProfileImpl()
#14 0x0000034af3f7 Profile::CreateProfile()
#15 0x0000034cd939 ProfileManager::CreateProfileHelper()
#16 0x0000034c76ce ProfileManager::CreateAndInitializeProfile()
#17 0x0000034c7411 ProfileManager::GetProfile()
#18 0x0000034c7181 ProfileManager::GetActiveUserOrOffTheRecordProfileFromPath()
#19 0x0000034c731c ProfileManager::GetActiveUserProfile()
#20 0x0000019a061e chromeos::ProfileHelper::GetProfileByUserUnsafe()
#21 0x000005ca8a87 CastConfigClientMediaRouter::devices()
#22 0x000005ca8b59 CastConfigClientMediaRouter::RequestDeviceRefresh()
#23 0x000002fa9b41 ash::mojom::CastConfigClientStubDispatch::Accept()
#24 0x0000043db86d mojo::InterfaceEndpointClient::HandleValidatedMessage()
#25 0x0000043db5d6 mojo::FilterChain::Accept()
#26 0x0000043dc74b mojo::InterfaceEndpointClient::HandleIncomingMessage()
#27 0x0000043e2031 mojo::internal::MultiplexRouter::ProcessIncomingMessage()
#28 0x0000043e088c mojo::internal::MultiplexRouter::ProcessTasks()
#29 0x0000043e2b3e mojo::internal::MultiplexRouter::LockAndCallProcessTasks()
#30 0x00000336119e base::debug::TaskAnnotator::RunTask()
#31 0x0000032e863d base::MessageLoop::RunTask()
#32 0x0000032e8cb5 base::MessageLoop::DoWork()
#33 0x0000032ea9d9 base::MessagePumpGlib::Run()
#34 0x0000032e83b9 base::MessageLoop::RunHandler()
#35 0x00000330c7e0 base::RunLoop::Run()
#36 0x00000396fe0c content::RunThisRunLoop()
#37 0x00000396ffc1 content::RunAllPendingInMessageLoop()
#38 0x00000092389e extensions::FileSystemApiTestForDrive::AddTestFile()
#39 0x000000922864
extensions::FileSystemApiTestForDrive::SetUpTestFileHierarchy()
#40 0x000000922713
extensions::FileSystemApiTestForDrive::CreateDriveIntegrationService()
#41 0x0000017ebaf6
drive::DriveIntegrationServiceFactory::BuildServiceInstanceFor()
#42 0x000005677420 BrowserContextKeyedServiceFactory::BuildServiceInstanceFor()
#43 0x00000506a4a1 KeyedServiceFactory::GetServiceForContext()
#44 0x000001827b71 file_manager::VolumeManagerFactory::BuildServiceInstanceFor()
#45 0x000005677420 BrowserContextKeyedServiceFactory::BuildServiceInstanceFor()
#46 0x00000506a4a1 KeyedServiceFactory::GetServiceForContext()
#47 0x00000506b70b DependencyManager::CreateContextServices()
#48 0x000005676ff3
BrowserContextDependencyManager::DoCreateBrowserContextServices()
#49 0x0000034b3428 ProfileImpl::OnLocaleReady()
#50 0x0000034b0f38 ProfileImpl::OnPrefsLoaded()
#51 0x0000034b0d5f ProfileImpl::ProfileImpl()
#52 0x0000034af3f7 Profile::CreateProfile()
#53 0x0000034cd939 ProfileManager::CreateProfileHelper()
#54 0x0000034c76ce ProfileManager::CreateAndInitializeProfile()
#55 0x0000034c7411 ProfileManager::GetProfile()
#56 0x0000034c7181 ProfileManager::GetActiveUserOrOffTheRecordProfileFromPath()
#57 0x0000034c731c ProfileManager::GetActiveUserProfile()
#58 0x0000033c539e ChromeBrowserMainParts::PreMainMessageLoopRunImpl()
#59 0x0000033c49f1 ChromeBrowserMainParts::PreMainMessageLoopRun()
#60 0x0000017cd0d0
chromeos::ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun()
#61 0x0000028d0785 content::BrowserMainLoop::PreMainMessageLoopRun()

[commit-bot: I haz the power, 2016-11-29 02:27:11]

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

[commit-bot: I haz the power, 2016-11-29 02:27:11]

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

[Elliot Glaysher, 2016-11-29 20:12:14]

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

[commit-bot: I haz the power, 2016-11-29 20:12:40]

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-29 21:07:05]

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

[commit-bot: I haz the power, 2016-11-29 21:07:06]

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

[Elliot Glaysher, 2016-11-29 22:46:10]

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

[commit-bot: I haz the power, 2016-11-29 22:47:10]

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-11-29 23:25:58]

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

[commit-bot: I haz the power, 2016-11-29 23:27:39]

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-11-29 23:37:08]

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

[commit-bot: I haz the power, 2016-11-29 23:38:02]

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-30 01:40:33]

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

[commit-bot: I haz the power, 2016-11-30 01:40:34]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_compile_dbg_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)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Elliot Glaysher, 2016-11-30 18:16:07]

ok, this is ready for review for real this time.

[Elliot Glaysher, 2016-11-30 19:22:35]

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

[commit-bot: I haz the power, 2016-11-30 19:23:21]

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

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

[commit-bot: I haz the power, 2016-11-30 19:45:46]

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

[Elliot Glaysher, 2016-11-30 20:07:10]

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

[commit-bot: I haz the power, 2016-11-30 20:07:37]

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-11-30 21:19:51]

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

[commit-bot: I haz the power, 2016-11-30 21:20:10]

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

[James Cook, 2016-11-30 22:16:39]

I just looked at the mojom. Apologies for not communicating earlier that I
switched SystemTray to use SetClient(). Feel free to ping me in person or IM to
look at updates to this CL.

https://codereview.chromium.org/2525563003/diff/240001/ash/common/cast_config...
File ash/common/cast_config_client_proxy.h (right):

https://codereview.chromium.org/2525563003/diff/240001/ash/common/cast_config...
ash/common/cast_config_client_proxy.h:22:
CastConfigClientProxy(service_manager::Connector* connector);
explicit

https://codereview.chromium.org/2525563003/diff/240001/ash/public/interfaces/...
File ash/public/interfaces/cast_config.mojom (right):

https://codereview.chromium.org/2525563003/diff/240001/ash/public/interfaces/...
ash/public/interfaces/cast_config.mojom:7: struct CastSink {
Can you document this? (I know the original is undocumented.)

https://codereview.chromium.org/2525563003/diff/240001/ash/public/interfaces/...
ash/public/interfaces/cast_config.mojom:19: struct CastRoute {
ditto

https://codereview.chromium.org/2525563003/diff/240001/ash/public/interfaces/...
ash/public/interfaces/cast_config.mojom:47: interface CastConfigClient {
As mentioned in person, I think we should have an interface CastConfig with a
SetClient method that chrome uses to set itself as the CastConfigClient.

CastConfig could replace CastConfigObserver (just have chrome call
CastConfig::OnDeviceUpdated directly).

https://codereview.chromium.org/2525563003/diff/240001/ash/public/interfaces/...
ash/public/interfaces/cast_config.mojom:58: // Observers are immediately
notified of the current shelf states when added.
shelf states?  system tray / system menu state?  some other state?

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

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

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

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

[Elliot Glaysher, 2016-12-01 00:32:31]

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

[commit-bot: I haz the power, 2016-12-01 00:33:07]

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-01 01:49:00]

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

[commit-bot: I haz the power, 2016-12-01 01:49:00]

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

[Elliot Glaysher, 2016-12-01 21:06:57]

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

[commit-bot: I haz the power, 2016-12-01 21:07:29]

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-01 22:24:35]

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

[commit-bot: I haz the power, 2016-12-01 22:25:05]

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-01 23:21:35]

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

[commit-bot: I haz the power, 2016-12-01 23:22:06]

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-01 23:59:13]

Chrome now calls into ash.

https://codereview.chromium.org/2525563003/diff/240001/ash/public/interfaces/...
File ash/public/interfaces/cast_config.mojom (right):

https://codereview.chromium.org/2525563003/diff/240001/ash/public/interfaces/...
ash/public/interfaces/cast_config.mojom:7: struct CastSink {
On 2016/11/30 22:16:38, James Cook wrote:
> Can you document this? (I know the original is undocumented.)

I cannot. I don't know what it is.

https://codereview.chromium.org/2525563003/diff/240001/ash/public/interfaces/...
ash/public/interfaces/cast_config.mojom:47: interface CastConfigClient {
On 2016/11/30 22:16:38, James Cook wrote:
> As mentioned in person, I think we should have an interface CastConfig with a
> SetClient method that chrome uses to set itself as the CastConfigClient.
> 
> CastConfig could replace CastConfigObserver (just have chrome call
> CastConfig::OnDeviceUpdated directly).

Done.

https://codereview.chromium.org/2525563003/diff/240001/ash/public/interfaces/...
ash/public/interfaces/cast_config.mojom:58: // Observers are immediately
notified of the current shelf states when added.
On 2016/11/30 22:16:38, James Cook wrote:
> shelf states?  system tray / system menu state?  some other state?

Done.

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

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

[commit-bot: I haz the power, 2016-12-02 01:13:56]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-12-02 04:15:52]

Thanks for doing the SetClient conversion.

https://codereview.chromium.org/2525563003/diff/320001/ash/common/cast_config...
File ash/common/cast_config_controller.cc (right):

https://codereview.chromium.org/2525563003/diff/320001/ash/common/cast_config...
ash/common/cast_config_controller.cc:26: if (Connected())
I think if (client_) is a synonym for client_.is_bound(), so maybe this could be
if (client_) and the Connected() method could go away?

https://codereview.chromium.org/2525563003/diff/320001/ash/common/cast_config...
ash/common/cast_config_controller.cc:27: client_->RequestDeviceRefresh();
It's a little weird that this is a side effect of adding an observer. I think it
would be clearer for callers to AddObserver to explicitly call
RequestDeviceRefresh() as well.

If that's too verbose or won't work for some reason, then document the side
effect by AddObserver in the header.

https://codereview.chromium.org/2525563003/diff/320001/ash/common/cast_config...
File ash/common/cast_config_controller.h (right):

https://codereview.chromium.org/2525563003/diff/320001/ash/common/cast_config...
ash/common/cast_config_controller.h:18: // information. This observer interface
send that information to all the
nit: sends

https://codereview.chromium.org/2525563003/diff/320001/ash/common/cast_config...
ash/common/cast_config_controller.h:19: // TrayCast items--there is one per
display.
Thanks for documenting the one-per-display thing.

https://codereview.chromium.org/2525563003/diff/320001/ash/common/cast_config...
ash/common/cast_config_controller.h:22: CastConfigControllerObserver() {}
We don't usually provide constructor implementations for pure-virtual observer
classes in ash. (I'm not sure why we choose one way or another, but that's what
most observers in ash do. :-P)

https://codereview.chromium.org/2525563003/diff/320001/ash/common/cast_config...
ash/common/cast_config_controller.h:23: virtual ~CastConfigControllerObserver()
{}
protected?

https://codereview.chromium.org/2525563003/diff/320001/ash/common/cast_config...
ash/common/cast_config_controller.h:29:
DISALLOW_COPY_AND_ASSIGN(CastConfigControllerObserver);
not needed since it's pure virtual

https://codereview.chromium.org/2525563003/diff/320001/ash/common/system/chro...
File ash/common/system/chromeos/cast/tray_cast.cc (right):

https://codereview.chromium.org/2525563003/diff/320001/ash/common/system/chro...
ash/common/system/chromeos/cast/tray_cast.cc:177:
base::UTF8ToUTF16(route->title), base::UTF8ToUTF16(sink->name))));
Can all strings in .mojom files be assumed to be UTF-8?  If not, comment the
mojom that they are UTF-8. (I don't know our mojom convention.)

https://codereview.chromium.org/2525563003/diff/320001/ash/common/system/chro...
ash/common/system/chromeos/cast/tray_cast.cc:385: for (auto& receiver :
sinks_routes) {
Good catch on the &

https://codereview.chromium.org/2525563003/diff/320001/ash/common/system/chro...
ash/common/system/chromeos/cast/tray_cast.cc:529: if
(WmShell::Get()->cast_config()->Connected() &&
Is the Connected() check necessary?  Is it for ash unit tests?

https://codereview.chromium.org/2525563003/diff/320001/chrome/browser/apps/ap...
File chrome/browser/apps/app_browsertest_util.cc (right):

https://codereview.chromium.org/2525563003/diff/320001/chrome/browser/apps/ap...
chrome/browser/apps/app_browsertest_util.cc:76:
ExtensionApiTest::TearDownInProcessBrowserTestFixture();
maybe do this after the SetMediaRouterForTest call?

https://codereview.chromium.org/2525563003/diff/320001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/cast_config_client_media_router.cc (right):

https://codereview.chromium.org/2525563003/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/cast_config_client_media_router.cc:153: // Connect to the
SystemTray interface in ash. Under mash the SystemTray
nit: remove "SystemTray"

https://codereview.chromium.org/2525563003/diff/320001/chrome/browser/ui/ash/...
File
chrome/browser/ui/ash/system_tray_tray_cast_browsertest_media_router_chromeos.cc
(right):

https://codereview.chromium.org/2525563003/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_tray_cast_browsertest_media_router_chromeos.cc:94:
base::MakeUnique<CastConfigClientMediaRouter>();
Why do you need to make a second CastConfigClientMediaRouter? Shouldn't the one
created in ChromeBrowserMainExtraPartsAsh suffice?

https://codereview.chromium.org/2525563003/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_tray_cast_browsertest_media_router_chromeos.cc:132:
content::RunAllPendingInMessageLoop();
Maybe comment the first one of these to say that it's needed for mojo messages
to propagate (which I'm guessing is why you need all these).

[Elliot Glaysher, 2016-12-02 19:02:20]

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

[commit-bot: I haz the power, 2016-12-02 19:03:09]

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-02 19:23:12]

https://codereview.chromium.org/2525563003/diff/320001/ash/common/cast_config...
File ash/common/cast_config_controller.cc (right):

https://codereview.chromium.org/2525563003/diff/320001/ash/common/cast_config...
ash/common/cast_config_controller.cc:26: if (Connected())
On 2016/12/02 04:15:52, James Cook wrote:
> I think if (client_) is a synonym for client_.is_bound(), so maybe this could
be
> if (client_) and the Connected() method could go away?

I can't remove Connected() entirely because it's used in the view code outside
this class.

https://codereview.chromium.org/2525563003/diff/320001/ash/common/cast_config...
ash/common/cast_config_controller.cc:27: client_->RequestDeviceRefresh();
On 2016/12/02 04:15:52, James Cook wrote:
> It's a little weird that this is a side effect of adding an observer. I think
it
> would be clearer for callers to AddObserver to explicitly call
> RequestDeviceRefresh() as well.
> 
> If that's too verbose or won't work for some reason, then document the side
> effect by AddObserver in the header.

Done.

https://codereview.chromium.org/2525563003/diff/320001/ash/common/cast_config...
File ash/common/cast_config_controller.h (right):

https://codereview.chromium.org/2525563003/diff/320001/ash/common/cast_config...
ash/common/cast_config_controller.h:18: // information. This observer interface
send that information to all the
On 2016/12/02 04:15:52, James Cook wrote:
> nit: sends

Done.

https://codereview.chromium.org/2525563003/diff/320001/ash/common/cast_config...
ash/common/cast_config_controller.h:22: CastConfigControllerObserver() {}
On 2016/12/02 04:15:52, James Cook wrote:
> We don't usually provide constructor implementations for pure-virtual observer
> classes in ash. (I'm not sure why we choose one way or another, but that's
what
> most observers in ash do. :-P)

Removing the DISALLOW... fixed this.

https://codereview.chromium.org/2525563003/diff/320001/ash/common/cast_config...
ash/common/cast_config_controller.h:23: virtual ~CastConfigControllerObserver()
{}
On 2016/12/02 04:15:52, James Cook wrote:
> protected?

Done.

https://codereview.chromium.org/2525563003/diff/320001/ash/common/cast_config...
ash/common/cast_config_controller.h:29:
DISALLOW_COPY_AND_ASSIGN(CastConfigControllerObserver);
On 2016/12/02 04:15:52, James Cook wrote:
> not needed since it's pure virtual

Done.

https://codereview.chromium.org/2525563003/diff/320001/ash/common/system/chro...
File ash/common/system/chromeos/cast/tray_cast.cc (right):

https://codereview.chromium.org/2525563003/diff/320001/ash/common/system/chro...
ash/common/system/chromeos/cast/tray_cast.cc:177:
base::UTF8ToUTF16(route->title), base::UTF8ToUTF16(sink->name))));
On 2016/12/02 04:15:52, James Cook wrote:
> Can all strings in .mojom files be assumed to be UTF-8?  If not, comment the
> mojom that they are UTF-8. (I don't know our mojom convention.)

All strings are UTF-8. (Except for the ones which are declared as a string16
datatype.)

https://codereview.chromium.org/2525563003/diff/320001/ash/common/system/chro...
ash/common/system/chromeos/cast/tray_cast.cc:529: if
(WmShell::Get()->cast_config()->Connected() &&
On 2016/12/02 04:15:52, James Cook wrote:
> Is the Connected() check necessary?  Is it for ash unit tests?

Yes. GetCastConfigDelegate() could return null in tests where the Chrome side
object wasn't present. Now that cast_config() always returns an object, the
equivalent is whether it's connected to the chrome side object.

https://codereview.chromium.org/2525563003/diff/320001/chrome/browser/apps/ap...
File chrome/browser/apps/app_browsertest_util.cc (right):

https://codereview.chromium.org/2525563003/diff/320001/chrome/browser/apps/ap...
chrome/browser/apps/app_browsertest_util.cc:76:
ExtensionApiTest::TearDownInProcessBrowserTestFixture();
On 2016/12/02 04:15:52, James Cook wrote:
> maybe do this after the SetMediaRouterForTest call?

Done.

https://codereview.chromium.org/2525563003/diff/320001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/cast_config_client_media_router.cc (right):

https://codereview.chromium.org/2525563003/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/cast_config_client_media_router.cc:153: // Connect to the
SystemTray interface in ash. Under mash the SystemTray
On 2016/12/02 04:15:52, James Cook wrote:
> nit: remove "SystemTray"

Done.

https://codereview.chromium.org/2525563003/diff/320001/chrome/browser/ui/ash/...
File
chrome/browser/ui/ash/system_tray_tray_cast_browsertest_media_router_chromeos.cc
(right):

https://codereview.chromium.org/2525563003/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_tray_cast_browsertest_media_router_chromeos.cc:94:
base::MakeUnique<CastConfigClientMediaRouter>();
On 2016/12/02 04:15:52, James Cook wrote:
> Why do you need to make a second CastConfigClientMediaRouter? Shouldn't the
one
> created in ChromeBrowserMainExtraPartsAsh suffice?

Deleted. I suspect this was a side effect of putting CastConfigClientMediaRouter
inside the RunningMash() block in extra parts that you caught yesterday.

https://codereview.chromium.org/2525563003/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_tray_cast_browsertest_media_router_chromeos.cc:132:
content::RunAllPendingInMessageLoop();
On 2016/12/02 04:15:52, James Cook wrote:
> Maybe comment the first one of these to say that it's needed for mojo messages
> to propagate (which I'm guessing is why you need all these).

Done.

[commit-bot: I haz the power, 2016-12-02 20:14:40]

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

[commit-bot: I haz the power, 2016-12-02 20:14:41]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-12-02 20:35:20]

LGTM with nit

https://codereview.chromium.org/2525563003/diff/340001/ash/common/cast_config...
File ash/common/cast_config_controller.cc (right):

https://codereview.chromium.org/2525563003/diff/340001/ash/common/cast_config...
ash/common/cast_config_controller.cc:71: void
CastConfigController::OnClientConnectionError() {
Is this still used?

[Elliot Glaysher, 2016-12-02 20:47:03]

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

[Elliot Glaysher, 2016-12-02 20:47:40]

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

[Elliot Glaysher, 2016-12-02 20:47:41]

+sky: chrome/ owners stamp
+tsepez: mojom review

[commit-bot: I haz the power, 2016-12-02 20:47:54]

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

[sky, 2016-12-02 21:23:28]

I'm assuming James reviewed chrome/browser/ui/ash . The rest LGTM

https://codereview.chromium.org/2525563003/diff/360001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/2525563003/diff/360001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:494: "//chrome/browser/media/router:test_support",
If this is only needed for chromeos move into chromeos section.

[commit-bot: I haz the power, 2016-12-02 21:50:58]

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

[commit-bot: I haz the power, 2016-12-02 21:50:59]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-12-02 22:48:00]

lgtm

[Elliot Glaysher, 2016-12-02 22:59:44]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2016-12-02 22:59:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, tsepez@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2525563003/#ps380001
(title: "move dep to chromeos section")

[commit-bot: I haz the power, 2016-12-02 23:00: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-03 00:47:28]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1480719584929080,
"parent_rev": "538a404a3a26e53e67e1c04957fedd834ba07cab", "commit_rev":
"4dca075c2d4f5764082ac71267b687e819a90178"}

[commit-bot: I haz the power, 2016-12-03 00:47:54]

Description was changed from

==========
mash: Change CastConfigDelegate to a mojoified CastConfigClient.

BUG=665074
==========

to

==========
mash: Change CastConfigDelegate to a mojoified CastConfigClient.

BUG=665074
==========

[commit-bot: I haz the power, 2016-12-03 00:47:56]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2016-12-03 00:51:33]

Description was changed from

==========
mash: Change CastConfigDelegate to a mojoified CastConfigClient.

BUG=665074
==========

to

==========
mash: Change CastConfigDelegate to a mojoified CastConfigClient.

BUG=665074

Committed: https://crrev.com/4ff45be6d3af36828f1042a839cbb5a6a60f6563
Cr-Commit-Position: refs/heads/master@{#436107}
==========

[commit-bot: I haz the power, 2016-12-03 00:51:35]

Patchset 20 (id:??) landed as
https://crrev.com/4ff45be6d3af36828f1042a839cbb5a6a60f6563
Cr-Commit-Position: refs/heads/master@{#436107}