Add PPB_Alarms_Dev interface definition.

ppapi/api/dev/ppb_alarms_dev.idl

+/* Copyright (c) 2013 The Chromium Authors. All rights reserved.
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+/**
+ * This file defines the Pepper equivalent of the <code>chrome.alarms</code>
+ * extension API.
+ */
+
+label Chrome {
+  M33 = 0.1
+};
+
+struct PP_Alarms_Alarm_Dev {
+  /**
+   * Name of this alarm.
+   */
+  PP_Var name;
+  /**
+   * Time at which this alarm was scheduled to fire, in milliseconds past the
+   * epoch. For performance reasons, the alarm may have been delayed an
+   * arbitrary amount beyond this.
+   */
+  double_t scheduled_time;
+  /**
+   * If set, the alarm is a repeating alarm and will fire again in
+   * <code>period_in_minutes</code> minutes.
+   */
+  PP_Optional_Double period_in_minutes;
+};
+
+struct PP_Alarms_AlarmCreateInfo_Dev {
+  /**
+   * Time at which the alarm should fire, in milliseconds past the epoch.
+   */
+  PP_Optional_Double when;
+  /**
+   * Length of time in minutes after which the
+   * <code>PP_Alarms_OnAlarm_Dev</code> event should fire.
+   */
+  PP_Optional_Double delay_in_minutes;
+  /**
+   * If set, the <code>PP_Alarms_OnAlarm_Dev</code> event should fire every
+   * <code>period_in_minutes</code> minutes after the initial event specified by
+   * <code>when</code> or <code>delay_in_minutes</code>. If not set, the alarm
+   * will only fire once.
+   */
+  PP_Optional_Double period_in_minutes;

[dmichael (off chromium), 2013/12/06 17:44:07]

Hmm, perhaps ideally |when| would be PP_Optional_Time and the others maybe
PP_Optional_TimeDelta (i.e., "optional" structs holding PP_Time and PP_TimeDelta
respectively). That would be more self-documenting and clearer w.r.t. the
relationship with PPB_Core::GetTime. But that wouldn't be so easy to automate.

Is there an opportunity to do that level of customization while still being able
to automate most of it? Do you think it's worth it?

I think at a minimum we might want to document that relationship between these
types and the existing pepper time types.

[yzshen1, 2013/12/06 21:38:21]

We need to do data format conversion (minute <--> seconds for
delay_in_minutes/period_in_minutes; milliseconds <--> seconds for when). We will
have to change the variable names accordingly. I think we could find out some
way to do the customization, but it might be not worth the effort. Besides,
having a straightforward mapping between the JS API and the Pepper API seems
like a good thing. Therefor, I think maybe it is okay to still use
PP_Optional_Double?

What do you think?
I think the current document is okay but I am happy to update it if you think I
need to elaborate:
For example, |when| has the comment "in milliseconds past the epoch"; and
PP_Time (which GetTime returns) is currently documented as "number of seconds
since the Epoch". If the user wants to use "now" as the value of |when|, he
could easily figure out he should use GetTime() * 1000.

[dmichael (off chromium), 2013/12/09 22:50:09]

Okay, I see, I stupidly wasn't thinking about units.

Might that inconsistency be confusing for |when|?

Oh well, it's probably fine. We need to prioritize ease of automation, I think.

+};
+
+struct PP_Alarms_Alarm_Array_Dev {
+  uint32_t size;
+  [size_is(count)] PP_Alarms_Alarm_Dev[] elements;

[dmichael (off chromium), 2013/12/06 17:44:07]

So I guess the idea is that Pepper will set |elements| and |size| after
allocating the buffer using PP_ArrayOutput?

I wonder if it would actually make sense to keep the PP_ArrayOutput parameter
separate? If we have array types that are used for in-params in some functions
and out-params in others, it wouldn't really make sense to provide
PP_ArrayOutput to the in-params.

It's also slightly confusing because here, I think |output| is (roughly) an
in-param, while |size| and |elements| are out-params.

And if there are functions with multiple array out-params...  we might be able
to have only 1 PP_ArrayOutput parameter. E.g.:
interface PPB_Foo {
  void DoSomething(
      [out] PP_Bar_Array bars,
      [out] PP_Baz_Array bazes,
      [in] PP_ArrayOutput array_output);
};
I.e., we can probably use 1 PP_ArrayOutput for both arrays. It gives the app
maybe a little less flexibility, but it probably covers most (if not all) actual
uses. The most likely definition just turns around and calls something like
malloc. Just an idea...  this part might not really make sense.

[yzshen1, 2013/12/06 21:38:21]

Yes. That's the plan. After the browser calls PP_ArrayOutput, it is responsible
to update |elements| and |size|.
Right, it is a waste in the in-param case, but I think the cost is not too much
-- two pointers for each array.
 
Agreed. That is one thing that I don't like about this approach.
I think so far we use a PP_ArrayOutput to correspond to one output array, what
you suggested is a more general "allocator callback". This is interesting.

I think the PP_ArrayOutput-as-struct-member approach has some benefits. It is a
little bit more "strongly typed". In the callback of the PP_ArrayOutput, we know
what the memory blocks are used for and can do size check or have different
memory allocation strategies.

However, when it comes to nested arrays, the PP_ArrayOutput-as-struct-member
approach is not very elegant. The general-allocator approach seems better.
Assume that we have the following data structures:
struct BarArray {
PP_ArrayOutput output;
// other fields.
};

struct Bar {
struct FooArray foo_array;
}

struct FooArray {
PP_ArrayOutput output;
// other fields.
}

struct Foo {
};

And we pass a BarArray output parameter to the browser.

The browser calls |BarArray.output| to allocate memory blocks for Bar instances.
In the allocator, the plugin knows that the memory is for Bar instances, it
needs to properly initialize |Bar.foo_array.output| of each Bar instance before
it returns the memory block (as void* :/) to the browser, so that the browser
can use |Bar.foo_array.output| to allocate Foo instances.

I am kind of sitting on the fence right now... What do you think?

[yzshen1, 2013/12/09 17:56:11]

Follow up:
I talked with Bill and he also likes your idea. I also agree that it have quite
some advantages, especially in the nested array case. So I will make the change
according to your suggestion. Thanks!

One thing that I would like to make sure you are okay with:

int32_t GetAll(
       [in] PP_Instance instance,
       [out] PP_Alarms_Alarm_Array_Dev alarms,
       [in] PP_ArrayOutput array_allocator,
       [in] PP_CompletionCallback callback);

In the example above, although it only returns a single array, the function has
an output |alarms| parameter. This is different than the existing APIs that
return arrays, which don't have any "output" parameters.

I do it this way so that it is more consistent with the cases that return
multiple arrays or nested arrays.

And accordingly, I named the PP_ArrayOutput as "array_allocator" instead of
"output".

What do you think?

[dmichael (off chromium), 2013/12/09 22:50:09]

I think that sounds good. We might ultimately need a broader developer-facing
document to help explain some of the conventions you're introducing.

[yzshen1, 2013/12/10 17:37:41]

That sounds good. I added an work item in
https://docs.google.com/a/google.com/spreadsheet/ccc?key=0Ahvxf7Ow1IjEdGZlVG1...

+  PP_ArrayOutput output;
+};
+
+/**
+ * Fired when an alarm has elapsed. Useful for event pages.
+ *
+ * @param[in] listener_id The listener ID.
+ * @param[inout] user_data The opaque pointer that was used when registering the
+ * listener.
+ * @param[in] alarm The alarm that has elapsed.
+ */
+typedef void PP_Alarms_OnAlarm_Dev(
+    [in] uint32_t listener_id,
+    [inout] mem_t user_data,
+    [in] PP_Alarms_Alarm_Dev alarm);
+
+interface PPB_Alarms_Dev {
+  /**
+   * Creates an alarm.  Near the time(s) specified by <code>alarm_info</code>,
+   * the <code>PP_Alarms_OnAlarm_Dev</code> event is fired. If there is another
+   * alarm with the same name (or no name if none is specified), it will be
+   * cancelled and replaced by this alarm.
+   *
+   * In order to reduce the load on the user's machine, Chrome limits alarms
+   * to at most once every 1 minute but may delay them an arbitrary amount more.
+   * That is, setting
+   * <code>PP_Alarms_AlarmCreateInfo_Dev.delay_in_minutes</code> or
+   * <code>PP_Alarms_AlarmCreateInfo_Dev.period_in_minutes</code> to less than
+   * <code>1</code> will not be honored and will cause a warning.

[dmichael (off chromium), 2013/12/06 17:44:07]

What kind of warning? JavaScript console? Just a log message?

[yzshen1, 2013/12/06 21:38:21]

I just copied the IDL comments. I think it is a JS console warning. :)

+   * <code>PP_Alarms_AlarmCreateInfo_Dev.when</code> can be set to less than 1
+   * minute after "now" without warning but won't actually cause the alarm to
+   * fire for at least 1 minute.
+   *
+   * To help you debug your app or extension, when you've loaded it unpacked,
+   * there's no limit to how often the alarm can fire.
+   *
+   * @param[in] instance A <code>PP_Instance</code>.
+   * @param[in] name A string or undefined <code>PP_Var</code>. Optional name to
+   * identify this alarm. Defaults to the empty string.
+   * @param[in] alarm_info Describes when the alarm should fire. The initial
+   * time must be specified by either <code>when</code> or
+   * <code>delay_in_minutes</code> (but not both).  If
+   * <code>period_in_minutes</code> is set, the alarm will repeat every
+   * <code>period_in_minutes</code> minutes after the initial event.  If neither
+   * <code>when</code> or <code>delay_in_minutes</code> is set for a repeating
+   * alarm, <code>period_in_minutes</code> is used as the default for
+   * <code>delay_in_minutes</code>.
+   */
+  void Create(
+      [in] PP_Instance instance,
+      [in] PP_Var name,
+      [in] PP_Alarms_AlarmCreateInfo_Dev alarm_info);
+
+  /**
+   * Retrieves details about the specified alarm.
+   *
+   * @param[in] instance A <code>PP_Instance</code>.
+   * @param[in] name A string or undefined <code>PP_Var</code>. The name of the
+   * alarm to get. Defaults to the empty string.
+   * @param[out] alarm A <code>PP_Alarms_Alarm_Dev</code> struct to store the
+   * output result.

[dmichael (off chromium), 2013/12/06 17:44:07]

May be worth a note that the caller is responsible for the ref-count of the
|alarm.name| string var...

It's kind of a shame in the C API; it's even worse for the array in GetAll. But
I don't think I have a good alternative.

[yzshen1, 2013/12/06 21:38:21]

Do you think we should comment for each place?
I wish we could tell the developers the common rules (output PP_Var/PP_Resource
has one ref that needs to be managed by the caller) and only comment the
exceptions.

(If we want to add ref-related comments, we would have to customize the comments
of the Apps IDL if the Pepper IDL is auto-generated. But we probably have to do
comment customization for other things anyway.)
Yeah, without destructor the plugin has to do it. But I think C developers
should not be too surprised about this.

+   * @param[in] callback A <code>PP_CompletionCallback</code> to be called upon
+   * completion.
+   *
+   * @return An error code from <code>pp_errors.h</code>
+   */
+  int32_t Get(
+      [in] PP_Instance instance,
+      [in] PP_Var name,
+      [out] PP_Alarms_Alarm_Dev alarm,
+      [in] PP_CompletionCallback callback);
+
+  /**
+   * Gets an array of all the alarms.
+   *
+   * @param[in] instance A <code>PP_Instance</code>.
+   * @param[out] alarms A <code>PP_Alarms_Alarm_Array_Dev</code> to store the
+   * output result.
+   * @param[in] callback A <code>PP_CompletionCallback</code> to be called upon
+   * completion.
+   *
+   * @return An error code from <code>pp_errors.h</code>
+   */
+  int32_t GetAll(
+      [in] PP_Instance instance,
+      [out] PP_Alarms_Alarm_Array_Dev alarms,
+      [in] PP_CompletionCallback callback);

[dmichael (off chromium), 2013/12/06 17:44:07]

When a 1-time Alarm has already fired, do we remove it from the active set that
would be returned here? Maybe that should be documented somewhere?

[yzshen1, 2013/12/06 21:38:21]

I think we should improve the Apps IDL in this case. :) I actually don't know
the answer. I could test the JS API to tell.

+
+  /**
+   * Clears the alarm with the given name.
+   *
+   * @param[in] instance A <code>PP_Instance</code>.
+   * @param[in] name A string or undefined <code>PP_Var</code>. The name of the
+   * alarm to clear. Defaults to the empty string.

[dmichael (off chromium), 2013/12/06 17:44:07]

What does it mean to say it defaults to the empty string here? Does anything
happen if you pass an empty string?

What happens if you pass a string that doesn't match any existing alarms? Just
nothing, I guess?

[yzshen1, 2013/12/06 21:38:21]

Copied from the Apps IDL. :/ If you think it is necessary, maybe I could have a
separate work item to improve the Apps IDL comments with the original author?

On 2013/12/06 17:44:07, dmichael wrote:
I think "empty string" is also treated as a name just as other non-empty
strings.

So you can add an alarm with empty name and clear it with an empty string.
I think so.

[dmichael (off chromium), 2013/12/09 22:50:09]

I think this is a case where the Apps IDL comments aren't sufficient. We can't
really do a default parameter in our C APIs, but it makes perfect sense in
JavaScript.

So is the idea that we only ever change the comments upstream? I'm worried we'll
run in to places where the differences are more pronounced between JavaScript
and C. C++ will also be slightly different.
Okay, that kind of thing could be improved in the Platform API documentation,
perhaps. I'm more worried about places where the differences between JS and C
will make our documentation weird or wrong.

[yzshen1, 2013/12/10 17:37:41]

I think the comment tried to say:
if the optional parameter |name| is not set (in Pepper API, setting it to an
undefined PP_Var), it is treated as an empty string.

But I agree that it should be more clear.
I haven't considered the details about how to deal with the differences. I wish
we could upstream the comments, for example, we could have markups in the apps
IDL such as <js-specific>, <pepper-specific>.
I totally agree that it is a must to have accurate comments for both JS and
Pepper APIs. And will keep that in mind. Thanks!

+   */
+  void Clear(
+      [in] PP_Instance instance,
+      [in] PP_Var name);
+
+  /**
+   * Clears all alarms.
+   *
+   * @param[in] instance A <code>PP_Instance</code>.
+   */
+  void ClearAll(
+      [in] PP_Instance instance);
+
+  /**
+   * Registers <code>PP_Alarms_OnAlarm_Dev</code> event.
+   *
+   * @param[in] instance A <code>PP_Instance</code>.
+   * @param[in] callback The callback to receive notifications.
+   * @param[inout] user_data An opaque pointer that will be passed to
+   * <code>callback</code>.
+   *
+   * @return A listener ID, or 0 if failed.
+   *
+   * TODO(yzshen): add a PPB_Events_Dev interface for unregistering:
+   * void UnregisterListener(PP_instance instance, uint32_t listener_id);
+   */
+  uint32_t AddOnAlarmListener(
+      [in] PP_Instance instance,
+      [in] PP_Alarms_OnAlarm_Dev callback,
+      [inout] mem_t user_data);
+};