Add API stubs for GCD device commands.

Add API stubs for GCD device commands.
Changed registration API to work for one network at once.
Fixed comments.
Replaced argument GCDDevice with just string id.

BUG=383167
R=alexsemenov@chromium.org, asargent@chromium.org, asvitkine@chromium.org, noamsml@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282099

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282273

[alexsemenov, 2014-07-02 18:17:26]

https://codereview.chromium.org/363883002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/gcd_private.idl (right):

https://codereview.chromium.org/363883002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/gcd_private.idl:79: static void
getCommandDefs(GCDDevice device, CommandDefsCallback callback);
Let's rename to getCommandDefinitions and CommandDefinitionsCallback

https://codereview.chromium.org/363883002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/gcd_private.idl:83: long responseAwaitMs,
Only need expiration time

https://codereview.chromium.org/363883002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/gcd_private.idl:85: CommandDefsCallback callback);
Same here: CommandDefinitionsCallback

https://codereview.chromium.org/363883002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/gcd_private.idl:85: CommandDefsCallback callback);
CommandDefsCallback -> CommandCallback here?

https://codereview.chromium.org/363883002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/gcd_private.idl:88: DOMString mediaPath,
Let's remove mediaPath for now

[Vitaly Buka (NO REVIEWS), 2014-07-02 22:20:01]

https://codereview.chromium.org/363883002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/gcd_private.idl (right):

https://codereview.chromium.org/363883002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/gcd_private.idl:79: static void
getCommandDefs(GCDDevice device, CommandDefsCallback callback);
On 2014/07/02 18:17:25, alexsemenov wrote:
> Let's rename to getCommandDefinitions and CommandDefinitionsCallback

Done.

https://codereview.chromium.org/363883002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/gcd_private.idl:83: long responseAwaitMs,
On 2014/07/02 18:17:25, alexsemenov wrote:
> Only need expiration time

Done.

https://codereview.chromium.org/363883002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/gcd_private.idl:85: CommandDefsCallback callback);
On 2014/07/02 18:17:26, alexsemenov wrote:
> CommandDefsCallback -> CommandCallback here?

Done.

https://codereview.chromium.org/363883002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/gcd_private.idl:88: DOMString mediaPath,
On 2014/07/02 18:17:25, alexsemenov wrote:
> Let's remove mediaPath for now

Done.

[alexsemenov, 2014-07-02 22:22:28]

lgtm

[Alexei Svitkine (slow), 2014-07-03 19:38:40]

lgtm for histograms.xml, but I've not looked at the rest.

[Vitaly Buka (NO REVIEWS), 2014-07-08 00:03:56]

+noamsml

[Noam Samuel, 2014-07-08 18:26:32]

https://codereview.chromium.org/363883002/diff/220001/chrome/common/extension...
File chrome/common/extensions/api/gcd_private.idl (right):

https://codereview.chromium.org/363883002/diff/220001/chrome/common/extension...
chrome/common/extensions/api/gcd_private.idl:33: callback
CommandDefinitionsCallback = void(object commandDefinitions);
Is the object schema in this case a CDD?  I think it's worth it to add comments
to each of these methods explaining what the unschema'd objects are.

https://codereview.chromium.org/363883002/diff/220001/chrome/common/extension...
chrome/common/extensions/api/gcd_private.idl:86: static void
insertCommand(GCDDevice device,
Does this transparently switch between local and cloud as we discussed?

https://codereview.chromium.org/363883002/diff/220001/chrome/common/extension...
chrome/common/extensions/api/gcd_private.idl:88: object command,
Is the schema for the command the GCD command schema?

https://codereview.chromium.org/363883002/diff/220001/chrome/common/extension...
chrome/common/extensions/api/gcd_private.idl:94: static void
getCommand(DOMString commandId,
Does this transparently handle local and cloud? If so, should the command ID be
opaquely "munged" by the API layer to signify that a command is a local or cloud
command?

https://codereview.chromium.org/363883002/diff/220001/chrome/common/extension...
chrome/common/extensions/api/gcd_private.idl:109: static void
getCommandsList(GCDDevice device,
Is this function cloud-only?

https://codereview.chromium.org/363883002/diff/220001/chrome/common/extension...
chrome/common/extensions/api/gcd_private.idl:118: // devices available using
cloud.
Does this mean we want to start a cloud request when users subscribe to this
event?

[Vitaly Buka (NO REVIEWS), 2014-07-08 18:49:06]

https://codereview.chromium.org/363883002/diff/220001/chrome/common/extension...
File chrome/common/extensions/api/gcd_private.idl (right):

https://codereview.chromium.org/363883002/diff/220001/chrome/common/extension...
chrome/common/extensions/api/gcd_private.idl:33: callback
CommandDefinitionsCallback = void(object commandDefinitions);
On 2014/07/08 18:26:32, Noam Samuel wrote:
> Is the object schema in this case a CDD?  I think it's worth it to add
comments
> to each of these methods explaining what the unschema'd objects are.

Done.

https://codereview.chromium.org/363883002/diff/220001/chrome/common/extension...
chrome/common/extensions/api/gcd_private.idl:86: static void
insertCommand(GCDDevice device,
yes
On 2014/07/08 18:26:32, Noam Samuel wrote:
> Does this transparently switch between local and cloud as we discussed?

https://codereview.chromium.org/363883002/diff/220001/chrome/common/extension...
chrome/common/extensions/api/gcd_private.idl:88: object command,
On 2014/07/08 18:26:32, Noam Samuel wrote:
> Is the schema for the command the GCD command schema?

Done.

https://codereview.chromium.org/363883002/diff/220001/chrome/common/extension...
chrome/common/extensions/api/gcd_private.idl:94: static void
getCommand(DOMString commandId,
implementation should take care about this.

On 2014/07/08 18:26:32, Noam Samuel wrote:
> Does this transparently handle local and cloud? If so, should the command ID
be
> opaquely "munged" by the API layer to signify that a command is a local or
cloud
> command?

https://codereview.chromium.org/363883002/diff/220001/chrome/common/extension...
chrome/common/extensions/api/gcd_private.idl:109: static void
getCommandsList(GCDDevice device,
No. It going to be local queue for local devices.
Cloud devices would have global queue.
But on API level is irrelevant.

On 2014/07/08 18:26:32, Noam Samuel wrote:
> Is this function cloud-only?

https://codereview.chromium.org/363883002/diff/220001/chrome/common/extension...
chrome/common/extensions/api/gcd_private.idl:118: // devices available using
cloud.
Maybe we would need subscribeForDeviceChanges(GCDevice device) method to avoid
updating all devices. Let's assume for now that we get updates for all devices
here.

On 2014/07/08 18:26:32, Noam Samuel wrote:
> Does this mean we want to start a cloud request when users subscribe to this
> event?

[Noam Samuel, 2014-07-08 19:04:30]

lgtm + 1 nit

https://codereview.chromium.org/363883002/diff/220001/chrome/common/extension...
File chrome/common/extensions/api/gcd_private.idl (right):

https://codereview.chromium.org/363883002/diff/220001/chrome/common/extension...
chrome/common/extensions/api/gcd_private.idl:118: // devices available using
cloud.
On 2014/07/08 18:49:05, Vitaly Buka wrote:
> Maybe we would need subscribeForDeviceChanges(GCDevice device) method to avoid
> updating all devices. Let's assume for now that we get updates for all devices
> here.
> 
> On 2014/07/08 18:26:32, Noam Samuel wrote:
> > Does this mean we want to start a cloud request when users subscribe to this
> > event?
> 

Maybe "and status updates for devices available through the cloud"?

[Vitaly Buka (NO REVIEWS), 2014-07-08 19:08:25]

On 2014/07/08 19:04:30, Noam Samuel wrote:
> lgtm + 1 nit
> 
>
https://codereview.chromium.org/363883002/diff/220001/chrome/common/extension...
> File chrome/common/extensions/api/gcd_private.idl (right):
> 
>
https://codereview.chromium.org/363883002/diff/220001/chrome/common/extension...
> chrome/common/extensions/api/gcd_private.idl:118: // devices available using
> cloud.
> On 2014/07/08 18:49:05, Vitaly Buka wrote:
> > Maybe we would need subscribeForDeviceChanges(GCDevice device) method to
avoid
> > updating all devices. Let's assume for now that we get updates for all
devices
> > here.
> > 
> > On 2014/07/08 18:26:32, Noam Samuel wrote:
> > > Does this mean we want to start a cloud request when users subscribe to
this
> > > event?
> > 
> 
> Maybe "and status updates for devices available through the cloud"?

done

[asargent_no_longer_on_chrome, 2014-07-08 19:59:00]

lgtm with the following caveat:

The use of "object" types mean that you lose the benefit of the built-in runtime
method/callback schema checking. We usually reserve these for cases where we
can't accurately represent a kind of object with our IDL; from a quick look it
seems like some of the cases where you're using an object type (eg the "state"
field of https://developers.google.com/cloud-devices/v1/reference/devices) could
be represented using a Dictionary in IDL. You might want to examine each case
and, for the ones where the set of key names is fixed, use a dictionary instead
of an opaque object.

[Vitaly Buka (NO REVIEWS), 2014-07-08 23:46:58]

On 2014/07/08 19:59:00, Antony Sargent wrote:
> lgtm with the following caveat:
> 
> The use of "object" types mean that you lose the benefit of the built-in
runtime
> method/callback schema checking. We usually reserve these for cases where we
> can't accurately represent a kind of object with our IDL; from a quick look it
> seems like some of the cases where you're using an object type (eg the "state"
> field of https://developers.google.com/cloud-devices/v1/reference/devices)
could
> be represented using a Dictionary in IDL. You might want to examine each case
> and, for the ones where the set of key names is fixed, use a dictionary
instead
> of an opaque object.

Thanks, it's definitely makes sense. Server side spec is not final yet, so I
have added TODO for that.
Particular for state, we decided to move that from GCDDevice. State is expensive
to get, so it should not be part of onDeviceStateChanged event.

[Vitaly Buka (NO REVIEWS), 2014-07-09 04:49:16]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-07-09 04:51:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/363883002/340001

[commit-bot: I haz the power, 2014-07-09 08:48:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Vitaly Buka (NO REVIEWS), 2014-07-09 08:58:31]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-07-09 08:59:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/363883002/360002

[commit-bot: I haz the power, 2014-07-09 10:49:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-09 10:53:09]

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

[commit-bot: I haz the power, 2014-07-09 10:53:10]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Vitaly Buka (NO REVIEWS), 2014-07-09 19:35:31]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-07-09 19:37:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/363883002/450001

[Vitaly Buka (NO REVIEWS), 2014-07-09 19:38:52]

The CQ bit was unchecked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-07-09 19:41:23]

Committed patchset #15 manually as r282099 (presubmit successful).

[dgrogan, 2014-07-09 23:37:02]

Reverted. See https://codereview.chromium.org/385483002/

[Vitaly Buka (NO REVIEWS), 2014-07-09 23:48:20]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-07-09 23:50:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/363883002/470001

[Vitaly Buka (NO REVIEWS), 2014-07-10 00:24:38]

The CQ bit was unchecked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-07-10 00:24:45]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-07-10 00:29:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/363883002/470001

[commit-bot: I haz the power, 2014-07-10 03:12:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-10 03:15:39]

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

[commit-bot: I haz the power, 2014-07-10 03:15:40]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Vitaly Buka (NO REVIEWS), 2014-07-10 06:57:29]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-07-10 06:58:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/363883002/490001

[Vitaly Buka (NO REVIEWS), 2014-07-10 07:14:35]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-07-10 07:16:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/363883002/530001

[Vitaly Buka (NO REVIEWS), 2014-07-10 07:47:08]

Committed patchset #16 manually as r282273 (presubmit successful).