Cast extension private API for ChromeOS system tray.

Cast extension private API for ChromeOS system tray.

Introduce cast.devicesPrivate.startCast(receiverId), cast.devicesPrivate.stopCast(reason), cast.devicesPrivate.updateDevices(devices), and cast.devicesPrivate.updateDevicesRequested().

startCast: Starts a casting session for the given receiver. The extension handles the UI for selecting what to mirror.
stopCast: Terminate the active cast.
updateDevices: Send the list of receivers and their activities to chrome.
updateDevicesRequested: An event triggered by chrome that requests the extension to resend the list of devices.

This API is used in https://codereview.chromium.org/1291113002/.
This API gets tested in https://codereview.chromium.org/1287023010/.

BUG=514952

[jdufault, 2015-08-14 22:03:04]

Patchset #1 (id:1) has been deleted

[jdufault, 2015-08-14 22:03:55]

jdufault@chromium.org changed reviewers:
+ achuith@chromium.org, kalman@chromium.org, rockot@chromium.org

[jdufault, 2015-08-14 22:12:10]

Achuith and either Ken or Benjamin, would you please take a look? Thanks!

[Ken Rockot(use gerrit already), 2015-08-14 23:20:09]

generally looks good, with a few comments

https://codereview.chromium.org/1291113002/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc
(right):

https://codereview.chromium.org/1291113002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc:44:
static ash::CastConfigDelegate::Receiver ConvertType(
instead of using static for these internal functions, move them into an
anonymous namespace near the top of the file. so you'll have:

namespace extension {

namespace {

ash::CastConfigDelegate::Receiver ConvertType(
 ...

ash::CastConfigDelegate::Activity ConvertType(
 ...

}  // namespace

https://codereview.chromium.org/1291113002/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.h
(right):

https://codereview.chromium.org/1291113002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.h:21:

Since it seems like this will be fine if created on-demand, can you please
define

static const bool kServiceIsCreatedWithBrowserContext = false;

in this class definition? (see
extensions/browser/browser_context_keyed_api_factory.h for docs in unclear)

https://codereview.chromium.org/1291113002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.h:52:
class CastDevicesPrivateUpdateDevicesFunction : public AsyncExtensionFunction {
It's preferable for new code to inherit UIThreadExtensionFunction as the base
ExtensionFunction interface now provides a more readable interface for doing
asynchronous work.

See docs in extension_function.h, but basically you implement Run instead of
RunAsync, and you can return a RespondLater() action to indicate that you'll
eventually call Respond().

[jdufault, 2015-08-15 00:04:52]

https://codereview.chromium.org/1291113002/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc
(right):

https://codereview.chromium.org/1291113002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc:44:
static ash::CastConfigDelegate::Receiver ConvertType(
On 2015/08/14 23:20:09, Ken Rockot wrote:
> instead of using static for these internal functions, move them into an
> anonymous namespace near the top of the file. so you'll have:
> 
> namespace extension {
> 
> namespace {
> 
> ash::CastConfigDelegate::Receiver ConvertType(
>  ...
> 
> ash::CastConfigDelegate::Activity ConvertType(
>  ...
> 
> }  // namespace
> 
> 

Done.

https://codereview.chromium.org/1291113002/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.h
(right):

https://codereview.chromium.org/1291113002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.h:21:

On 2015/08/14 23:20:09, Ken Rockot wrote:
> Since it seems like this will be fine if created on-demand, can you please
> define
> 
> static const bool kServiceIsCreatedWithBrowserContext = false;
> 
> in this class definition? (see
> extensions/browser/browser_context_keyed_api_factory.h for docs in unclear)

Done.

https://codereview.chromium.org/1291113002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.h:52:
class CastDevicesPrivateUpdateDevicesFunction : public AsyncExtensionFunction {
On 2015/08/14 23:20:09, Ken Rockot wrote:
> It's preferable for new code to inherit UIThreadExtensionFunction as the base
> ExtensionFunction interface now provides a more readable interface for doing
> asynchronous work.
> 
> See docs in extension_function.h, but basically you implement Run instead of
> RunAsync, and you can return a RespondLater() action to indicate that you'll
> eventually call Respond().

Done.

[Ken Rockot(use gerrit already), 2015-08-15 00:07:45]

lgtm

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc
(right):

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc:82:
CastDevicesPrivateUpdateDevicesFunction::Run() {
nit: in this case (return type on its own line) proper style is

ExtensionFunction::ResponseAction
CastDevicesPrivate... 

i.e. no indentation on the function name.

[jdufault, 2015-08-15 00:18:17]

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

[jdufault, 2015-08-15 00:19:19]

Ilya, would you mind taking a look at the histogram files? I need an owners
approval on every file. Thanks!

[Ilya Sherman, 2015-08-17 21:14:15]

histograms lgtm

[Ilya Sherman, 2015-08-17 21:15:21]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-08-17 21:15:41]

The CQ bit was unchecked by isherman@chromium.org

[Ilya Sherman, 2015-08-17 21:16:48]

Actually, one small thing: Could you please update the CL description so that
the title (a.k.a. "subject") is also the first line of the description? 
Otherwise, it will be missing from the CL description in the source control
revision history.

[jdufault, 2015-08-17 22:21:21]

On 2015/08/17 21:16:48, Ilya Sherman wrote:
> Actually, one small thing: Could you please update the CL description so that
> the title (a.k.a. "subject") is also the first line of the description? 
> Otherwise, it will be missing from the CL description in the source control
> revision history.

Sure, done. Thanks!

[Ilya Sherman, 2015-08-17 22:24:48]

LGTM, thanks.

[achuithb, 2015-08-18 06:19:31]

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc
(right):

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc:15:
ash::CastConfigDelegate::Receiver ConvertType(
Rename to ConvertReceiverType?

I don't think there's a strong case to overload here.

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc:23:
ash::CastConfigDelegate::Activity ConvertType(
Rename to ConvertActivityType?

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc:32:
ash::CastConfigDelegate::ReceiverAndActivity ConvertType(
Rename to ConvertReceiverAndActivityType?

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc:62:
CastDeviceUpdateListeners::~CastDeviceUpdateListeners() {}
newline before this line?

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc:92:
CastDeviceUpdateListeners* storage =
auto is fine here?

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.h
(right):

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.h:38:
const std::vector<ash::CastConfigDelegate::ReceiverAndActivity>& devices);
How about in the private part of this class:
using ReceiverAndActivityList =
std::vector<ash::CastConfigDelegate::ReceiverAndActivity>;

https://codereview.chromium.org/1291113002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/cast_devices_private.idl (right):

https://codereview.chromium.org/1291113002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/cast_devices_private.idl:5: // The
<code>chrome.cast.devicesPrivate</code> API manages starting stopping casts
Line break at stopping

[achuithb, 2015-08-18 06:20:33]

Let's flesh out the description of this bug to include the new private api
calls.

[jdufault, 2015-08-18 19:53:01]

I've added some more comments to the description - is that what you had in mind?
Thanks

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc
(right):

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc:15:
ash::CastConfigDelegate::Receiver ConvertType(
On 2015/08/18 06:19:30, achuithb wrote:
> Rename to ConvertReceiverType?
> 
> I don't think there's a strong case to overload here.

Done.

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc:23:
ash::CastConfigDelegate::Activity ConvertType(
On 2015/08/18 06:19:30, achuithb wrote:
> Rename to ConvertActivityType?

Done.

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc:32:
ash::CastConfigDelegate::ReceiverAndActivity ConvertType(
On 2015/08/18 06:19:30, achuithb wrote:
> Rename to ConvertReceiverAndActivityType?

Done.

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc:62:
CastDeviceUpdateListeners::~CastDeviceUpdateListeners() {}
On 2015/08/18 06:19:31, achuithb wrote:
> newline before this line?

Done.

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc:82:
CastDevicesPrivateUpdateDevicesFunction::Run() {
On 2015/08/15 00:07:45, Ken Rockot wrote:
> nit: in this case (return type on its own line) proper style is
> 
> ExtensionFunction::ResponseAction
> CastDevicesPrivate... 
> 
> i.e. no indentation on the function name.

Done.

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc:92:
CastDeviceUpdateListeners* storage =
On 2015/08/18 06:19:30, achuithb wrote:
> auto is fine here?

Done.

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.h
(right):

https://codereview.chromium.org/1291113002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.h:38:
const std::vector<ash::CastConfigDelegate::ReceiverAndActivity>& devices);
On 2015/08/18 06:19:31, achuithb wrote:
> How about in the private part of this class:
> using ReceiverAndActivityList =
> std::vector<ash::CastConfigDelegate::ReceiverAndActivity>;

Done.

https://codereview.chromium.org/1291113002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/cast_devices_private.idl (right):

https://codereview.chromium.org/1291113002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/cast_devices_private.idl:5: // The
<code>chrome.cast.devicesPrivate</code> API manages starting stopping casts
On 2015/08/18 06:19:31, achuithb wrote:
> Line break at stopping

Done.

[achuithb, 2015-08-18 22:26:12]

On 2015/08/18 19:53:01, jdufault wrote:
> I've added some more comments to the description - is that what you had in
mind?

more along the lines of:
Introduce cast.devicesPrivate.startCast(receiverId),
cast.devicesPrivate.stopCast(reason),
cast.devicesPrivate.updateDevices(devices). updateDevices is called by the cast
extension to pass the list of receivers and their activities to chrome.

[achuithb, 2015-08-18 22:26:37]

Don't you need test code for the apis?

[achuithb, 2015-08-18 22:30:14]

https://codereview.chromium.org/1291113002/diff/60001/chrome/browser/extensio...
File
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc
(right):

https://codereview.chromium.org/1291113002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc:72:
const CastDeviceUpdateListeners::ReceiverAndActivityList& devices) {
Do you actually need CastDeviceUpdateListeners:: in the function argument?

https://codereview.chromium.org/1291113002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc:94:
auto storage = CastDeviceUpdateListeners::Get(browser_context());
why storage instead of say listeners?

[jdufault, 2015-08-18 23:31:52]

This is tested via the Chromecast system tray item browser test. If the system
tray item were to get removed, so too would these APIs.

https://codereview.chromium.org/1291113002/diff/60001/chrome/browser/extensio...
File
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc
(right):

https://codereview.chromium.org/1291113002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc:72:
const CastDeviceUpdateListeners::ReceiverAndActivityList& devices) {
On 2015/08/18 22:30:14, achuithb wrote:
> Do you actually need CastDeviceUpdateListeners:: in the function argument?

Done.

https://codereview.chromium.org/1291113002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.cc:94:
auto storage = CastDeviceUpdateListeners::Get(browser_context());
On 2015/08/18 22:30:14, achuithb wrote:
> why storage instead of say listeners?

Done.

[achuithb, 2015-08-18 23:59:31]

lgtm

[jdufault, 2015-08-21 19:13:36]

Landed in https://codereview.chromium.org/1288073005/