Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
103993006:
Add PPB_Alarms_Dev interface definition. (Closed)
|
Created:
7 years ago by yzshen1 Modified:
7 years ago CC:
chromium-reviews, yusukes+watch_chromium.org, raymes+watch_chromium.org, jam, yzshen+watch_chromium.org, joi+watch-content_chromium.org, teravest+watch_chromium.org, darin-cc_chromium.org, Sam Clegg, nfullagar1, piman+watch_chromium.org, noelallen1, binji, ihf+watch_chromium.org, Sam McNally, Mark Seaborn Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd PPB_Alarms_Dev interface definition.
The C++ wrapper will be in a separate CL.
BUG=327197, 233439
TEST=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240013
Patch Set 1 #
Total comments: 26
Patch Set 2 : . #Patch Set 3 : . #
Total comments: 8
Patch Set 4 : #Patch Set 5 : . #Patch Set 6 : #
Total comments: 9
Patch Set 7 : . #
Messages
Total messages: 27 (0 generated)
Hi, David and Bill. Would you please take a look? Thanks! This CL is part of the following CL (which contains IDL/C/C++/example): https://codereview.chromium.org/60173003/ (I haven't updated the design doc, I will do that soon.) I am still using the old dev API style. I will switch once Justin lands his work to support new dev APIs. Thanks!
https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... File ppapi/api/dev/ppb_alarms_dev.idl (right): https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... ppapi/api/dev/ppb_alarms_dev.idl:49: PP_Optional_Double period_in_minutes; 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. https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... ppapi/api/dev/ppb_alarms_dev.idl:54: [size_is(count)] PP_Alarms_Alarm_Dev[] elements; 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. https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... ppapi/api/dev/ppb_alarms_dev.idl:83: * <code>1</code> will not be honored and will cause a warning. What kind of warning? JavaScript console? Just a log message? https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... ppapi/api/dev/ppb_alarms_dev.idl:115: * output result. 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. https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... ppapi/api/dev/ppb_alarms_dev.idl:141: [in] PP_CompletionCallback callback); 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? https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... ppapi/api/dev/ppb_alarms_dev.idl:148: * alarm to clear. Defaults to the empty string. 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? https://codereview.chromium.org/103993006/diff/1/ppapi/generators/idl_c_heade... File ppapi/generators/idl_c_header.py (right): https://codereview.chromium.org/103993006/diff/1/ppapi/generators/idl_c_heade... ppapi/generators/idl_c_header.py:150: # PP_Alarms_OnAlarm_Dev to pass a struct pointer. Have you checked with teravest or somebody on PNaCl? This restriction has to do with the PNaCl shim, and I don't remember the details. https://codereview.chromium.org/103993006/diff/1/ppapi/thunk/ppb_alarms_dev_t... File ppapi/thunk/ppb_alarms_dev_thunk.cc (right): https://codereview.chromium.org/103993006/diff/1/ppapi/thunk/ppb_alarms_dev_t... ppapi/thunk/ppb_alarms_dev_thunk.cc:16: // TODO(yzshen): Implement the thunk. might be good to reference the bug. https://codereview.chromium.org/103993006/diff/1/ppapi/thunk/ppb_alarms_dev_t... ppapi/thunk/ppb_alarms_dev_thunk.cc:21: } You could put NOTIMPLEMENTED() in all of these if you wanted.
Thanks David! Please take another look. +Justin: Would you please take a look at idl_c_header.py? Thanks! https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... File ppapi/api/dev/ppb_alarms_dev.idl (right): https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... ppapi/api/dev/ppb_alarms_dev.idl:49: PP_Optional_Double period_in_minutes; On 2013/12/06 17:44:07, dmichael wrote: > 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? > 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 at a minimum we might want to document that relationship between these > types and the existing pepper time types. 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. https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... ppapi/api/dev/ppb_alarms_dev.idl:54: [size_is(count)] PP_Alarms_Alarm_Dev[] elements; On 2013/12/06 17:44:07, dmichael wrote: > So I guess the idea is that Pepper will set |elements| and |size| after > allocating the buffer using PP_ArrayOutput? Yes. That's the plan. After the browser calls PP_ArrayOutput, it is responsible to update |elements| and |size|. > 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. Right, it is a waste in the in-param case, but I think the cost is not too much -- two pointers for each array. > It's also slightly confusing because here, I think |output| is (roughly) an > in-param, while |size| and |elements| are out-params. Agreed. That is one thing that I don't like about this approach. > > 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. 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? https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... ppapi/api/dev/ppb_alarms_dev.idl:83: * <code>1</code> will not be honored and will cause a warning. On 2013/12/06 17:44:07, dmichael wrote: > What kind of warning? JavaScript console? Just a log message? I just copied the IDL comments. I think it is a JS console warning. :) https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... ppapi/api/dev/ppb_alarms_dev.idl:115: * output result. On 2013/12/06 17:44:07, dmichael wrote: > May be worth a note that the caller is responsible for the ref-count of the > |alarm.name| string var... 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.) > > 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. Yeah, without destructor the plugin has to do it. But I think C developers should not be too surprised about this. https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... ppapi/api/dev/ppb_alarms_dev.idl:141: [in] PP_CompletionCallback callback); On 2013/12/06 17:44:07, dmichael wrote: > 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? 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. https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... ppapi/api/dev/ppb_alarms_dev.idl:148: * alarm to clear. Defaults to the empty string. 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: > What does it mean to say it defaults to the empty string here? Does anything > happen if you pass an empty string? 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. > > What happens if you pass a string that doesn't match any existing alarms? Just > nothing, I guess? I think so. https://codereview.chromium.org/103993006/diff/1/ppapi/generators/idl_c_heade... File ppapi/generators/idl_c_header.py (right): https://codereview.chromium.org/103993006/diff/1/ppapi/generators/idl_c_heade... ppapi/generators/idl_c_header.py:150: # PP_Alarms_OnAlarm_Dev to pass a struct pointer. On 2013/12/06 17:44:07, dmichael wrote: > Have you checked with teravest or somebody on PNaCl? This restriction has to do > with the PNaCl shim, and I don't remember the details. I could add Justin as a reviewer. This check was added to prevent us from adding more pass-by-value structs. https://codereview.chromium.org/103993006/diff/1/ppapi/thunk/ppb_alarms_dev_t... File ppapi/thunk/ppb_alarms_dev_thunk.cc (right): https://codereview.chromium.org/103993006/diff/1/ppapi/thunk/ppb_alarms_dev_t... ppapi/thunk/ppb_alarms_dev_thunk.cc:16: // TODO(yzshen): Implement the thunk. I think it is not something that we will forget. :) So if you don't mind, I will skip the bug filing. (But please feel free to ask me to file one if you still think it is necessary.) On 2013/12/06 17:44:07, dmichael wrote: > might be good to reference the bug. https://codereview.chromium.org/103993006/diff/1/ppapi/thunk/ppb_alarms_dev_t... ppapi/thunk/ppb_alarms_dev_thunk.cc:21: } On 2013/12/06 17:44:07, dmichael wrote: > You could put NOTIMPLEMENTED() in all of these if you wanted. Done.
https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... File ppapi/api/dev/ppb_alarms_dev.idl (right): https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... ppapi/api/dev/ppb_alarms_dev.idl:54: [size_is(count)] PP_Alarms_Alarm_Dev[] elements; 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?
On 2013/12/09 17:56:10, yzshen1 wrote: > https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... > File ppapi/api/dev/ppb_alarms_dev.idl (right): > > https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... > ppapi/api/dev/ppb_alarms_dev.idl:54: [size_is(count)] PP_Alarms_Alarm_Dev[] > elements; > 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? We should probably work out what the C++ wrappers will look like for this. Could you add those to this CL?
On 2013/12/09 18:00:18, bbudge1 wrote: > On 2013/12/09 17:56:10, yzshen1 wrote: > > > https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... > > File ppapi/api/dev/ppb_alarms_dev.idl (right): > > > > > https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... > > ppapi/api/dev/ppb_alarms_dev.idl:54: [size_is(count)] PP_Alarms_Alarm_Dev[] > > elements; > > 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? > > We should probably work out what the C++ wrappers will look like for this. Could > you add those to this CL? I will write it down in https://codereview.chromium.org/60173003/ (which is a draft CL containing IDL/C/C++/C++ utilities/example) Because the C++ wrappers require quite a lot of supporting code, I wish I could make it a separate CL. What do you think?
On 2013/12/09 18:06:48, yzshen1 wrote: > On 2013/12/09 18:00:18, bbudge1 wrote: > > On 2013/12/09 17:56:10, yzshen1 wrote: > > > > > > https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... > > > File ppapi/api/dev/ppb_alarms_dev.idl (right): > > > > > > > > > https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... > > > ppapi/api/dev/ppb_alarms_dev.idl:54: [size_is(count)] PP_Alarms_Alarm_Dev[] > > > elements; > > > 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? > > > > We should probably work out what the C++ wrappers will look like for this. > Could > > you add those to this CL? > > I will write it down in https://codereview.chromium.org/60173003/ (which is a > draft CL containing IDL/C/C++/C++ utilities/example) > > Because the C++ wrappers require quite a lot of supporting code, I wish I could > make it a separate CL. > > What do you think? I forgot about your note in the description of the CL. As long as we work out how the wrapper will look to make sure it doesn't have any bad consequences, it's fine in another CL.
On 2013/12/09 18:11:50, bbudge1 wrote: > On 2013/12/09 18:06:48, yzshen1 wrote: > > On 2013/12/09 18:00:18, bbudge1 wrote: > > > On 2013/12/09 17:56:10, yzshen1 wrote: > > > > > > > > > > https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... > > > > File ppapi/api/dev/ppb_alarms_dev.idl (right): > > > > > > > > > > > > > > https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... > > > > ppapi/api/dev/ppb_alarms_dev.idl:54: [size_is(count)] > PP_Alarms_Alarm_Dev[] > > > > elements; > > > > 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? > > > > > > We should probably work out what the C++ wrappers will look like for this. > > Could > > > you add those to this CL? > > > > I will write it down in https://codereview.chromium.org/60173003/ (which is a > > draft CL containing IDL/C/C++/C++ utilities/example) > > > > Because the C++ wrappers require quite a lot of supporting code, I wish I > could > > make it a separate CL. > > > > What do you think? > > I forgot about your note in the description of the CL. > > As long as we work out how the wrapper will look to make sure it doesn't have > any bad consequences, it's fine in another CL. I have updated 60173003 to use the new approach, please take a look at: https://codereview.chromium.org/60173003/diff2/360001:380001/ppapi/cpp/dev/ar... https://codereview.chromium.org/60173003/diff2/360001:380001/ppapi/cpp/dev/al...
lgtm, definitely good enough for a first draft. I think I like how the array output turned out (and it looks like it somewhat simplified the C++ side). I'm slightly concerned about the documentation problems we're going to run in to if we force ourselves to use the same comments as the platform API IDL. https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... File ppapi/api/dev/ppb_alarms_dev.idl (right): https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... ppapi/api/dev/ppb_alarms_dev.idl:49: PP_Optional_Double period_in_minutes; On 2013/12/06 21:38:21, yzshen1 wrote: > On 2013/12/06 17:44:07, dmichael wrote: > > 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? > > > > > 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 at a minimum we might want to document that relationship between these > > types and the existing pepper time types. > 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. 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. https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... ppapi/api/dev/ppb_alarms_dev.idl:54: [size_is(count)] PP_Alarms_Alarm_Dev[] elements; On 2013/12/09 17:56:11, yzshen1 wrote: > 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? I think that sounds good. We might ultimately need a broader developer-facing document to help explain some of the conventions you're introducing. https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... ppapi/api/dev/ppb_alarms_dev.idl:148: * alarm to clear. Defaults to the empty string. On 2013/12/06 21:38:21, yzshen1 wrote: > 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? 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. > > On 2013/12/06 17:44:07, dmichael wrote: > > What does it mean to say it defaults to the empty string here? Does anything > > happen if you pass an empty string? > 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. > > > > What happens if you pass a string that doesn't match any existing alarms? Just > > nothing, I guess? > I think so. 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. https://codereview.chromium.org/103993006/diff/1/ppapi/thunk/ppb_alarms_dev_t... File ppapi/thunk/ppb_alarms_dev_thunk.cc (right): https://codereview.chromium.org/103993006/diff/1/ppapi/thunk/ppb_alarms_dev_t... ppapi/thunk/ppb_alarms_dev_thunk.cc:16: // TODO(yzshen): Implement the thunk. On 2013/12/06 21:38:21, yzshen1 wrote: > I think it is not something that we will forget. :) So if you don't mind, I will > skip the bug filing. > (But please feel free to ask me to file one if you still think it is necessary.) I'm really talking about the larger work. Either the work of making Alarms, or the broader work of converting Platform APIs to Pepper. We definitely need a tracking bug for that if we don't have one. No need to file a smaller bug for implementing the thunk, though.
CCing mseaborn https://codereview.chromium.org/103993006/diff/80001/ppapi/api/dev/ppb_alarms... File ppapi/api/dev/ppb_alarms_dev.idl (right): https://codereview.chromium.org/103993006/diff/80001/ppapi/api/dev/ppb_alarms... ppapi/api/dev/ppb_alarms_dev.idl:68: [in] PP_Alarms_Alarm_Dev alarm); Why is it okay to pass a struct by value here? See https://code.google.com/p/chromium/issues/detail?id=233439. Why will this work on PNaCl, when the following is problematic? // From ppapi/c/extensions/dev/ppb_ext_alarms_dev.h: typedef void (*PP_Ext_Alarms_OnAlarm_Func_Dev_0_1)( uint32_t listener_id, void* user_data, PP_Ext_Alarms_Alarm_Dev alarm); // "alarm" is call-by-value struct I don't like changing the logic in idl_c_header.py to permit this, without understanding why this won't cause a problem for PNaCl.
https://codereview.chromium.org/103993006/diff/80001/ppapi/api/dev/ppb_alarms... File ppapi/api/dev/ppb_alarms_dev.idl (right): https://codereview.chromium.org/103993006/diff/80001/ppapi/api/dev/ppb_alarms... ppapi/api/dev/ppb_alarms_dev.idl:68: [in] PP_Alarms_Alarm_Dev alarm); On 2013/12/10 14:56:05, teravest wrote: > Why is it okay to pass a struct by value here? See > https://code.google.com/p/chromium/issues/detail?id=233439. > > Why will this work on PNaCl, when the following is problematic? > // From ppapi/c/extensions/dev/ppb_ext_alarms_dev.h: > typedef void (*PP_Ext_Alarms_OnAlarm_Func_Dev_0_1)( > uint32_t listener_id, > void* user_data, > PP_Ext_Alarms_Alarm_Dev alarm); // "alarm" is call-by-value struct > > I don't like changing the logic in idl_c_header.py to permit this, without > understanding why this won't cause a problem for PNaCl. Indeed. This is a problem and won't work under PNaCl. You have to pass structs to callbacks by pointer instead of by value. https://codereview.chromium.org/103993006/diff/80001/ppapi/generators/idl_c_h... File ppapi/generators/idl_c_header.py (right): https://codereview.chromium.org/103993006/diff/80001/ppapi/generators/idl_c_h... ppapi/generators/idl_c_header.py:152: 'PP_Alarms_OnAlarm_Dev'] Don't add new entries to this whitelist, please. Entries here are callbacks that don't work on PNaCl, as you should find when you add tests for this interface and enable them under PNaCl. When this list was added in April (in https://codereview.chromium.org/13973011/), the intention was that PP_Ext_Alarms_OnAlarm_Func_Dev_0_1 would get fixed, not that it is an acceptable use of call-by-value.
Hi, Justin and Mark. Thanks for your input. Please see my in-line reply. https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... File ppapi/api/dev/ppb_alarms_dev.idl (right): https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... ppapi/api/dev/ppb_alarms_dev.idl:54: [size_is(count)] PP_Alarms_Alarm_Dev[] elements; On 2013/12/09 22:50:09, dmichael wrote: > On 2013/12/09 17:56:11, yzshen1 wrote: > > 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? > I think that sounds good. We might ultimately need a broader developer-facing > document to help explain some of the conventions you're introducing. That sounds good. I added an work item in https://docs.google.com/a/google.com/spreadsheet/ccc?key=0Ahvxf7Ow1IjEdGZlVG1... https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... ppapi/api/dev/ppb_alarms_dev.idl:148: * alarm to clear. Defaults to the empty string. On 2013/12/09 22:50:09, dmichael wrote: > On 2013/12/06 21:38:21, yzshen1 wrote: > > 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? > 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. 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. > > 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. 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>. > > > > > On 2013/12/06 17:44:07, dmichael wrote: > > > What does it mean to say it defaults to the empty string here? Does anything > > > happen if you pass an empty string? > > 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. > > > > > > What happens if you pass a string that doesn't match any existing alarms? > Just > > > nothing, I guess? > > I think so. > 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. 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! https://codereview.chromium.org/103993006/diff/80001/ppapi/api/dev/ppb_alarms... File ppapi/api/dev/ppb_alarms_dev.idl (right): https://codereview.chromium.org/103993006/diff/80001/ppapi/api/dev/ppb_alarms... ppapi/api/dev/ppb_alarms_dev.idl:68: [in] PP_Alarms_Alarm_Dev alarm); On 2013/12/10 14:56:05, teravest wrote: > Why is it okay to pass a struct by value here? See > https://code.google.com/p/chromium/issues/detail?id=233439. > > Why will this work on PNaCl, when the following is problematic? > // From ppapi/c/extensions/dev/ppb_ext_alarms_dev.h: > typedef void (*PP_Ext_Alarms_OnAlarm_Func_Dev_0_1)( > uint32_t listener_id, > void* user_data, > PP_Ext_Alarms_Alarm_Dev alarm); // "alarm" is call-by-value struct I agree with both of you. :) The whole PPB_Ext_Alarms_Dev interface has been removed yesterday by me: https://codereview.chromium.org/103093004 So the example you used before is gone now. Here PP_Alarms_OnAlarm_Dev passes PP_Alarms_Alarm_Dev *by reference*, so I think it is okay. But the check in idl_c_header.py mistakenly regards it as pass-by-value and issues an presumbit error. That is what I would like to fix. > > I don't like changing the logic in idl_c_header.py to permit this, without > understanding why this won't cause a problem for PNaCl. >
On Tue, Dec 10, 2013 at 10:37 AM, <yzshen@chromium.org> wrote: > Hi, Justin and Mark. > > Thanks for your input. Please see my in-line reply. > > > > https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... > File ppapi/api/dev/ppb_alarms_dev.idl (right): > > https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... > ppapi/api/dev/ppb_alarms_dev.idl:54: [size_is(count)] > PP_Alarms_Alarm_Dev[] elements; > On 2013/12/09 22:50:09, dmichael wrote: >> >> On 2013/12/09 17:56:11, yzshen1 wrote: >> > 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? >> I think that sounds good. We might ultimately need a broader > > developer-facing >> >> document to help explain some of the conventions you're introducing. > > > That sounds good. I added an work item in > https://docs.google.com/a/google.com/spreadsheet/ccc?key=0Ahvxf7Ow1IjEdGZlVG1... > > > https://codereview.chromium.org/103993006/diff/1/ppapi/api/dev/ppb_alarms_dev... > ppapi/api/dev/ppb_alarms_dev.idl:148: * alarm to clear. Defaults to the > empty string. > On 2013/12/09 22:50:09, dmichael wrote: >> >> On 2013/12/06 21:38:21, yzshen1 wrote: >> > 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? >> >> 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. > > > 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. > > >> 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. > > > 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>. > > > >> > >> > On 2013/12/06 17:44:07, dmichael wrote: >> > > What does it mean to say it defaults to the empty string here? > > Does anything >> >> > > happen if you pass an empty string? >> > 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. >> >> > > >> > > What happens if you pass a string that doesn't match any existing > > alarms? >> >> Just >> > > nothing, I guess? >> > I think so. >> 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. > > > 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! > > > https://codereview.chromium.org/103993006/diff/80001/ppapi/api/dev/ppb_alarms... > File ppapi/api/dev/ppb_alarms_dev.idl (right): > > https://codereview.chromium.org/103993006/diff/80001/ppapi/api/dev/ppb_alarms... > ppapi/api/dev/ppb_alarms_dev.idl:68: [in] PP_Alarms_Alarm_Dev alarm); > On 2013/12/10 14:56:05, teravest wrote: >> >> Why is it okay to pass a struct by value here? See >> https://code.google.com/p/chromium/issues/detail?id=233439. > > >> Why will this work on PNaCl, when the following is problematic? >> // From ppapi/c/extensions/dev/ppb_ext_alarms_dev.h: >> typedef void (*PP_Ext_Alarms_OnAlarm_Func_Dev_0_1)( >> uint32_t listener_id, >> void* user_data, >> PP_Ext_Alarms_Alarm_Dev alarm); // "alarm" is call-by-value struct > > > I agree with both of you. :) > > The whole PPB_Ext_Alarms_Dev interface has been removed yesterday by me: > https://codereview.chromium.org/103093004 > So the example you used before is gone now. > > Here PP_Alarms_OnAlarm_Dev passes PP_Alarms_Alarm_Dev *by reference*, so > I think it is okay. But the check in idl_c_header.py mistakenly regards > it as pass-by-value and issues an presumbit error. That is what I would > like to fix. Okay, thanks. I looks like it's passed by pointer, but not by reference: 92 typedef void (*PP_Alarms_OnAlarm_Dev)( 93 uint32_t listener_id, 94 void* user_data, 95 const struct PP_Alarms_Alarm_Dev* alarm); ...but your point still stands. As long as it's not being passed by value, it's fine by me (and we should fix up the logic in idl_c_header; sorry about that). lgtm > > > >> I don't like changing the logic in idl_c_header.py to permit this, > > without >> >> understanding why this won't cause a problem for PNaCl. > > > > https://codereview.chromium.org/103993006/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/103993006/diff/80001/ppapi/api/dev/ppb_alarms... File ppapi/api/dev/ppb_alarms_dev.idl (right): https://codereview.chromium.org/103993006/diff/80001/ppapi/api/dev/ppb_alarms... ppapi/api/dev/ppb_alarms_dev.idl:68: [in] PP_Alarms_Alarm_Dev alarm); On 2013/12/10 17:37:41, yzshen1 wrote: > On 2013/12/10 14:56:05, teravest wrote: > > Why is it okay to pass a struct by value here? See > > https://code.google.com/p/chromium/issues/detail?id=233439. > > > > Why will this work on PNaCl, when the following is problematic? > > // From ppapi/c/extensions/dev/ppb_ext_alarms_dev.h: > > typedef void (*PP_Ext_Alarms_OnAlarm_Func_Dev_0_1)( > > uint32_t listener_id, > > void* user_data, > > PP_Ext_Alarms_Alarm_Dev alarm); // "alarm" is call-by-value struct > > I agree with both of you. :) > > The whole PPB_Ext_Alarms_Dev interface has been removed yesterday by me: > https://codereview.chromium.org/103093004 > So the example you used before is gone now. > > Here PP_Alarms_OnAlarm_Dev passes PP_Alarms_Alarm_Dev *by reference*, so I think > it is okay. But the check in idl_c_header.py mistakenly regards it as > pass-by-value and issues an presumbit error. That is what I would like to fix. > > > > > I don't like changing the logic in idl_c_header.py to permit this, without > > understanding why this won't cause a problem for PNaCl. > > > The generated code here looks like this: typedef void (*PP_Alarms_OnAlarm_Dev)( uint32_t listener_id, void* user_data, const struct PP_Alarms_Alarm_Dev* alarm); Note that PP_Alarms_Alarm_Dev is passed by const-pointer, as Yuzhu noted. I think the problem is not at this spot, but below. See comment there... https://codereview.chromium.org/103993006/diff/80001/ppapi/api/dev/ppb_alarms... ppapi/api/dev/ppb_alarms_dev.idl:179: [in] PP_Alarms_OnAlarm_Dev callback, I think maybe this is the part that's getting flagged? The generated code looks like this: uint32_t (*AddOnAlarmListener)(PP_Instance instance, PP_Alarms_OnAlarm_Dev callback, void* user_data); See how PP_Alarms_OnAlarm_Dev appears to be by-value? However, it's a function pointer, so it's probably safe (Mark, please correct me if I'm wrong). We're going to run in to more and more function pointers with the new APIs Yuzhu's working on. If it's at all possible, it would be best to make the IDL generator understand that it's a function pointer and therefore safe to pass this way. But I think using the whitelist for now is probably fine.
Thanks, reviewers! Some issues related to the comments in alarms IDL require me to do some tests before I can clarify. So I will do that later. And eventually I wish those clarifications could be upstream. https://codereview.chromium.org/103993006/diff/1/ppapi/thunk/ppb_alarms_dev_t... File ppapi/thunk/ppb_alarms_dev_thunk.cc (right): https://codereview.chromium.org/103993006/diff/1/ppapi/thunk/ppb_alarms_dev_t... ppapi/thunk/ppb_alarms_dev_thunk.cc:16: // TODO(yzshen): Implement the thunk. This is a good point. I have filed a meta bug and some concrete bugs that block the meta bug to track the progress. (https://code.google.com/p/chromium/issues/detail?id=327193) Thanks! https://codereview.chromium.org/103993006/diff/80001/ppapi/generators/idl_c_h... File ppapi/generators/idl_c_header.py (right): https://codereview.chromium.org/103993006/diff/80001/ppapi/generators/idl_c_h... ppapi/generators/idl_c_header.py:152: 'PP_Alarms_OnAlarm_Dev'] I have just removed PP_Ext_Alarms_OnAlarm_Func_Dev_0_1 yesterday. Sorry I didn't do it earlier because we held off the development of this project for a while and that interface was never implemented. Please note that the new entry I added (PP_Alarms_OnAlarm_Dev) passes struct by reference. I think that is allowed, right? This check is problematic to issue an error. On 2013/12/10 15:56:17, Mark Seaborn wrote: > Don't add new entries to this whitelist, please. Entries here are callbacks > that don't work on PNaCl, as you should find when you add tests for this > interface and enable them under PNaCl. > > When this list was added in April (in > https://codereview.chromium.org/13973011/), the intention was that > PP_Ext_Alarms_OnAlarm_Func_Dev_0_1 would get fixed, not that it is an acceptable > use of call-by-value.
https://codereview.chromium.org/103993006/diff/80001/ppapi/generators/idl_c_h... File ppapi/generators/idl_c_header.py (right): https://codereview.chromium.org/103993006/diff/80001/ppapi/generators/idl_c_h... ppapi/generators/idl_c_header.py:152: 'PP_Alarms_OnAlarm_Dev'] A little bit clarification, when I said "pass by reference", I meant "pass by pointer". :)
Sorry, I think my understanding is wrong. Justin and I were looking, and it appears that the check for parameters being passed by value is too strict. It disallows structs (and their typedefs) as in-params. However, the generator appears to correctly emit (most) struct params as const-pointer params. So there's an inconsistency there; apparently the old Alarm API didn't work because it had a PP_Var generated by-value. It looks like we special-case PP_Var passByValue and returnByValue. Maybe you can add a TODO(teravest) or file a bug for this... he's going to fix up the check to be less restrictive, to only flag in-param structs that are "passByValue", since the rest will be generated as const-pointer params in the C header.
In fact, I think this will fix it:
diff --git a/ppapi/generators/idl_c_header.py
b/ppapi/generators/idl_c_header.py
index a3b8688..97414fb 100755
--- a/ppapi/generators/idl_c_header.py
+++ b/ppapi/generators/idl_c_header.py
@@ -162,7 +162,7 @@ def CheckTypedefs(filenode, releases):
t = param.GetType(build_list[0])
while t.IsA('Typedef'):
t = t.GetType(build_list[0])
- if t.IsA('Struct'):
+ if t.IsA('Struct') and t.GetProperty('passByValue'):
raise Exception('%s is a struct in callback %s. '
'See http://crbug.com/233439' %
(t.GetName(), node.GetName()))
Sorry about that!
On Tue, Dec 10, 2013 at 11:12 AM, <dmichael@chromium.org> wrote:
> Sorry, I think my understanding is wrong. Justin and I were looking, and it
> appears that the check for parameters being passed by value is too strict.
> It
> disallows structs (and their typedefs) as in-params. However, the generator
> appears to correctly emit (most) struct params as const-pointer params. So
> there's an inconsistency there; apparently the old Alarm API didn't work
> because
> it had a PP_Var generated by-value. It looks like we special-case PP_Var
> passByValue and returnByValue.
>
> Maybe you can add a TODO(teravest) or file a bug for this... he's going to
> fix
> up the check to be less restrictive, to only flag in-param structs that are
> "passByValue", since the rest will be generated as const-pointer params in
> the C
> header.
>
> https://codereview.chromium.org/103993006/
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
On 2013/12/10 18:15:31, teravest wrote:
> In fact, I think this will fix it:
>
> diff --git a/ppapi/generators/idl_c_header.py
> b/ppapi/generators/idl_c_header.py
> index a3b8688..97414fb 100755
> --- a/ppapi/generators/idl_c_header.py
> +++ b/ppapi/generators/idl_c_header.py
> @@ -162,7 +162,7 @@ def CheckTypedefs(filenode, releases):
> t = param.GetType(build_list[0])
> while t.IsA('Typedef'):
> t = t.GetType(build_list[0])
> - if t.IsA('Struct'):
> + if t.IsA('Struct') and t.GetProperty('passByValue'):
> raise Exception('%s is a struct in callback %s. '
> 'See http://crbug.com/233439%27 %
> (t.GetName(), node.GetName()))
>
> Sorry about that!
>
> On Tue, Dec 10, 2013 at 11:12 AM, <mailto:dmichael@chromium.org> wrote:
> > Sorry, I think my understanding is wrong. Justin and I were looking, and it
> > appears that the check for parameters being passed by value is too strict.
> > It
> > disallows structs (and their typedefs) as in-params. However, the generator
> > appears to correctly emit (most) struct params as const-pointer params. So
> > there's an inconsistency there; apparently the old Alarm API didn't work
> > because
> > it had a PP_Var generated by-value. It looks like we special-case PP_Var
> > passByValue and returnByValue.
> >
> > Maybe you can add a TODO(teravest) or file a bug for this... he's going to
> > fix
> > up the check to be less restrictive, to only flag in-param structs that are
> > "passByValue", since the rest will be generated as const-pointer params in
> > the C
> > header.
> >
> > https://codereview.chromium.org/103993006/
>
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
Thanks David and Justin!
I have merged Justin's fix into this CL.
+noelallen Hi, Noel. Would you please do an OWNER review for native_client_sdk/*? (And welcome to comment on other files, too.) Thanks!
LGTM with a few questions. https://codereview.chromium.org/103993006/diff/100002/ppapi/api/dev/ppb_alarm... File ppapi/api/dev/ppb_alarms_dev.idl (right): https://codereview.chromium.org/103993006/diff/100002/ppapi/api/dev/ppb_alarm... ppapi/api/dev/ppb_alarms_dev.idl:15: struct PP_Alarms_Alarm_Dev { This naming scheme seems a little strange to me. I'm accustomed to things like PP_FileInfo, not PP_FileIO_FileInfo. I would expect this to be PP_Alarm_Dev. Is there a reason we use this longer name? Is it to avoid some other API possibly needing an alarm struct? https://codereview.chromium.org/103993006/diff/100002/ppapi/api/dev/ppb_alarm... ppapi/api/dev/ppb_alarms_dev.idl:175: * void UnregisterListener(PP_instance instance, uint32_t listener_id); Why not put UnregisterListener here? https://codereview.chromium.org/103993006/diff/100002/ppapi/api/dev/ppb_alarm... ppapi/api/dev/ppb_alarms_dev.idl:177: uint32_t AddOnAlarmListener( Why not name this RegisterOnAlarmListener?
https://codereview.chromium.org/103993006/diff/100002/ppapi/api/dev/ppb_alarm... File ppapi/api/dev/ppb_alarms_dev.idl (right): https://codereview.chromium.org/103993006/diff/100002/ppapi/api/dev/ppb_alarm... ppapi/api/dev/ppb_alarms_dev.idl:15: struct PP_Alarms_Alarm_Dev { On 2013/12/10 21:52:33, bbudge1 wrote: > This naming scheme seems a little strange to me. I'm accustomed to things like > PP_FileInfo, not PP_FileIO_FileInfo. I would expect this to be PP_Alarm_Dev. > > Is there a reason we use this longer name? Is it to avoid some other API > possibly needing an alarm struct? Yeah, the apps APIs have namespace, so the type names in many cases are too vague by itself. :) For example, it makes sense to me to convert socket.CreateInfo to PP_Socket_CreateInfo instead of PP_CreateInfo. https://codereview.chromium.org/103993006/diff/100002/ppapi/api/dev/ppb_alarm... ppapi/api/dev/ppb_alarms_dev.idl:175: * void UnregisterListener(PP_instance instance, uint32_t listener_id); Because we don't need one UnregisterListener for each event or each API. We only need a general UnregisterListener() for all the events. On 2013/12/10 21:52:33, bbudge1 wrote: > Why not put UnregisterListener here? https://codereview.chromium.org/103993006/diff/100002/ppapi/api/dev/ppb_alarm... ppapi/api/dev/ppb_alarms_dev.idl:177: uint32_t AddOnAlarmListener( Ah, I just realized that my TODO suggested Unregister*() and here I used Add*(). Thanks for pointing out the mismatch. I think I will use Add*Listener() and RemoveListener(), because the JS APIs use addListener() and removeListener(). What do you think? On 2013/12/10 21:52:33, bbudge1 wrote: > Why not name this RegisterOnAlarmListener?
https://codereview.chromium.org/103993006/diff/100002/ppapi/api/dev/ppb_alarm... File ppapi/api/dev/ppb_alarms_dev.idl (right): https://codereview.chromium.org/103993006/diff/100002/ppapi/api/dev/ppb_alarm... ppapi/api/dev/ppb_alarms_dev.idl:15: struct PP_Alarms_Alarm_Dev { On 2013/12/10 22:16:06, yzshen1 wrote: > On 2013/12/10 21:52:33, bbudge1 wrote: > > This naming scheme seems a little strange to me. I'm accustomed to things like > > PP_FileInfo, not PP_FileIO_FileInfo. I would expect this to be PP_Alarm_Dev. > > > > Is there a reason we use this longer name? Is it to avoid some other API > > possibly needing an alarm struct? > > Yeah, the apps APIs have namespace, so the type names in many cases are too > vague by itself. :) > > For example, it makes sense to me to convert socket.CreateInfo to > PP_Socket_CreateInfo instead of PP_CreateInfo. > OK, I see. https://codereview.chromium.org/103993006/diff/100002/ppapi/api/dev/ppb_alarm... ppapi/api/dev/ppb_alarms_dev.idl:175: * void UnregisterListener(PP_instance instance, uint32_t listener_id); On 2013/12/10 22:16:06, yzshen1 wrote: > Because we don't need one UnregisterListener for each event or each API. > > We only need a general UnregisterListener() for all the events. > > On 2013/12/10 21:52:33, bbudge1 wrote: > > Why not put UnregisterListener here? > I suppose this is how the JS APIs work. OK. https://codereview.chromium.org/103993006/diff/100002/ppapi/api/dev/ppb_alarm... ppapi/api/dev/ppb_alarms_dev.idl:177: uint32_t AddOnAlarmListener( On 2013/12/10 22:16:06, yzshen1 wrote: > Ah, I just realized that my TODO suggested Unregister*() and here I used Add*(). > Thanks for pointing out the mismatch. > > I think I will use Add*Listener() and RemoveListener(), because the JS APIs use > addListener() and removeListener(). What do you think? > > On 2013/12/10 21:52:33, bbudge1 wrote: > > Why not name this RegisterOnAlarmListener? > SGTM
Should that be PP_Optional_Double_Dev? Otherwise LGTM.
On 2013/12/10 23:52:25, noelallen1 wrote: > Should that be PP_Optional_Double_Dev? > > Otherwise LGTM. Done. Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yzshen@chromium.org/103993006/150001
Message was sent while issue was closed.
Change committed as 240013 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
