Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
116963003:
App APIs in Pepper: C++ APIs (Closed)
|
Created:
7 years ago by yzshen1 Modified:
6 years, 11 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, raymes+watch_chromium.org, teravest+watch_chromium.org, yzshen+watch_chromium.org, nfullagar1, piman+watch_chromium.org, noelallen1, binji, ihf+watch_chromium.org, raymes Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionApp APIs in Pepper: C++ APIs
This CL contains supporting code for C++ APIs and the C++ equivalent of chrome.alarms.
BUG=327197
TEST=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243158
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 27
Patch Set 4 : #Patch Set 5 : changes according to David's suggestions. #
Total comments: 14
Patch Set 6 : #Patch Set 7 : #
Total comments: 7
Patch Set 8 : changes according to Sam's suggestions. #
Messages
Total messages: 22 (0 generated)
Hi, David and Sam. Would you please take a look? Thanks! Notes: - The alarms API and data types have the "_Dev" suffix. However, the supporting classes (e.g., Optional, Array, MayOwnPtr) currently don't have the "_Dev" suffix, although I still placed them in the /dev folder. I think this is less verbose and will save me some renaming/formating time when we move to the new dev API design. - The code makes output_traits.h include two files from the /dev folder. Once we use the new dev API design, this problem should be gone. Please let me know if you have concerns about what I have done. Thanks!
Cool stuff! I probably don't understand everything yet, but my early general feedback is... if it would be possible to have less magic by generating a little bit more specialized code, that might be easier to understand and maintain. Also, I think some example and/or test code would make it easier to understand some of the tradeoffs. https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... File ppapi/cpp/dev/struct_wrapper_base_dev.h (right): https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:13: // The only purpose of this class is for CallbackOutputTraits to determined determined->determine https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:15: class StructWrapperIdentifier { I mentioned this elsewhere, but I don't think you need this. You can do an explicit full specialization for each "struct wrapper" kind of class. https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:20: class StructWrapperBase : public StructWrapperIdentifier { It seems slightly messy to me to have this as a base class, when I think it's not really necessary. This seems to have two separate responsibilities: - A traits style class that does typedefs for you. That could be a separate traits class instead of a base class, I think. - A data holder (using MayOwnPtr). This has some weird consequences; since this class isn't copyable, you have to use operator= in the derived classes. Would it be possible to separate this in to two things: 1) A "traits" class that only has typedefs and possibly static functions? 2) A "holder" class that the generated class can hold as a member? (Possibly just MayOwnPtr, I don't know). It's very possible I'm missing something, but if there's a cleaner way that doesn't rely on implementation inheritance, I think I might like that better. WDYT? https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:35: // longer than this object. |storage| must be zero-initialized. I think you mean that the caller must zero-initialize |*storage|? It would be good to clarify. (note you mean the contents of *storage, right, not the pointer itself?) https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:36: StructWrapperBase(CType* storage, NotOwned) : storage_(storage, NOT_OWNED) {} If there's not a constructor that takes ownership, why is there all this complexity around specifying "NOT_OWNED" and having a MayOwnPtr? https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:53: protected: If subclasses aren't supposed to _invoke_ these, maybe they should be private. https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:57: virtual void NotifyEndRawUpdate() = 0; I think these wouldn't be necessary if the struct wrapper thingy was a member instead of a base class. The generated code would just provide its own StartRawUpdate that calls StartRawUpdate on all members, including the struct wrapper one. https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/to_c_type_... File ppapi/cpp/dev/to_c_type_converter_dev.h (right): https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/to_c_type_... ppapi/cpp/dev/to_c_type_converter_dev.h:45: const PP_Var& ToCInput() const { return wrapper_.ToVar(); } These classes look like you might be able to get away with just static functions, and no data members? (I haven't looked at where they are used yet; this comment is a placeholder until I do... maybe you need the storage) https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/output_traits.h File ppapi/cpp/output_traits.h (right): https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/output_traits.... ppapi/cpp/output_traits.h:220: struct CallbackOutputTraits<Array<T> > { Would it be practical to put this in the header for Array instead? I would rather not add a bunch of includes to this file for clients who don't need them. Similarly, it would be nice if the StructWrapper stuff could go somewhere else. I don't think you need StructWrapperIdentifier. Since the code is all generated anyway, each "struct wrapper" header can put an explicit full specialization of CallbackOutputTraits in its header. E.g.: template <> CallbackOutputTraits<AlarmDev> : public StructWrapperCallbackOutputTraits<AlarmDev> { };
Thanks David! The testing code can be found here: https://codereview.chromium.org/60173003/diff/440001/ppapi/examples/extension... https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... File ppapi/cpp/dev/struct_wrapper_base_dev.h (right): https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:13: // The only purpose of this class is for CallbackOutputTraits to determined On 2013/12/17 18:03:25, dmichael wrote: > determined->determine Done. https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:15: class StructWrapperIdentifier { I think one good thing about this approach is that it doesn't need to have specialized output traits defined in those "user-facing" C++ API headers (e.g., alarms_dev.h). Having less and simpler code (especially internal impl details) in the C++ API headers may be easier for developers. Although they could read our documents, I think many of them will still want to look at the headers. I would like to keep things that they don't need to care out of the API headers, if possible. And this approach is the same as what we did for pp::Resource subclasses. I think using the same approach is nice. Is it your concern that this approach causes output_traits.h to depend on a file in the /dev folder? I think once we support the new dev api design, this problem will be gone. And before that, if we want to avoid the issue, we could move this StructWrapperIdentifier class to the output_traits.h file. What do you think? Thanks! On 2013/12/17 18:03:25, dmichael wrote: > I mentioned this elsewhere, but I don't think you need this. You can do an > explicit full specialization for each "struct wrapper" kind of class. https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:20: class StructWrapperBase : public StructWrapperIdentifier { On 2013/12/17 18:03:25, dmichael wrote: > It seems slightly messy to me to have this as a base class, when I think it's > not really necessary. This seems to have two separate responsibilities: > - A traits style class that does typedefs for you. That could be a separate > traits class instead of a base class, I think. > - A data holder (using MayOwnPtr). This has some weird consequences; since this > class isn't copyable, you have to use operator= in the derived classes. > > Would it be possible to separate this in to two things: > 1) A "traits" class that only has typedefs and possibly static functions? > 2) A "holder" class that the generated class can hold as a member? (Possibly > just MayOwnPtr, I don't know). > > It's very possible I'm missing something, but if there's a cleaner way that > doesn't rely on implementation inheritance, I think I might like that better. > > WDYT? I made this StructWrapperBase class based on the following reasons: - As I mentioned above, I wish those "user-facing" headers (such as alarms_dev.h) to only contain code that developers need to care. Therefore, I tried to move things needed only by our internal impl (typedef / ToStruct / StartRawUpdate / EndRawUpdate) into this base class. I know it is somewhat cheating, they are still public in the subclasses. :) But the headers do seem cleaner to me. - Having the typedef in the struct wrapper doesn't seem too bad to me. It is reasonable that a struct wrapper has type information about the C struct that it wraps. If I make separate traits classes, the specialization has to live in those "user-facing" headers, which I would like to avoid. But probably I could remove CInputType and COutputType entirely. - Having |storage_| in the base class is nice because it has to be created before (and destroyed after) all the accessors that attach to it. (By accessors, I mean things like Alarms_Dev::name_wrapper_.) What do you think? Thanks! https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:35: // longer than this object. |storage| must be zero-initialized. Right, the contents pointed to by |storage| must be zero-initialized by the caller. Done. I revised the comments. On 2013/12/17 18:03:25, dmichael wrote: > I think you mean that the caller must zero-initialize |*storage|? It would be > good to clarify. (note you mean the contents of *storage, right, not the pointer > itself?) https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:36: StructWrapperBase(CType* storage, NotOwned) : storage_(storage, NOT_OWNED) {} On 2013/12/17 18:03:25, dmichael wrote: > If there's not a constructor that takes ownership, why is there all this > complexity around specifying "NOT_OWNED" and having a MayOwnPtr? I was thinking NOT_OWNED makes it more obvious that this constructor doesn't take ownership. You are right, no constructor will take ownership of passed-in storage. Do you think I should remove NOT_OWNED and only rely on the comments to make the ownership clear? MayOwnPtr is necessary: Other constructors will create the underlying storage which is owned by the MayOwnPtr. The reason of this design is that a struct wrapper (and many other things such as Optional<>, Array<>, StringWrapper) can act as a standalone object (owns the underlying C struct), or an accessor (doesn't own). For example: struct PP_Bar {}; struct PP_Foo { struct PP_Bar bar; }; class Foo { ... private: MayOwnPtr<PP_Foo> storage_; Bar bar_wrapper_; // this accessor is attached to |storage_->bar|, it doesn't own the memory. }; https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:53: protected: On 2013/12/17 18:03:25, dmichael wrote: > If subclasses aren't supposed to _invoke_ these, maybe they should be private. You are right. Thanks! Done. https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:57: virtual void NotifyEndRawUpdate() = 0; I used them so that the public section of the subclasses doesn't have to explicitly declare StartRawUpdate()/EndRawUpdate(). I thought that makes the user-facing headers look simpler, because developers usually don't need to care about them. (But I agree that they are still public in the subclasses.) On the other hand, removing them has the upside that this class is not virtual and is more efficient.. I am fine with either way. What do you think? On 2013/12/17 18:03:25, dmichael wrote: > I think these wouldn't be necessary if the struct wrapper thingy was a member > instead of a base class. The generated code would just provide its own > StartRawUpdate that calls StartRawUpdate on all members, including the struct > wrapper one. https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/to_c_type_... File ppapi/cpp/dev/to_c_type_converter_dev.h (right): https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/to_c_type_... ppapi/cpp/dev/to_c_type_converter_dev.h:45: const PP_Var& ToCInput() const { return wrapper_.ToVar(); } On 2013/12/17 18:03:25, dmichael wrote: > These classes look like you might be able to get away with just static > functions, and no data members? (I haven't looked at where they are used yet; > this comment is a placeholder until I do... maybe you need the storage) Yeah, |wrapper_| in this class owns a PP_Var. https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/output_traits.h File ppapi/cpp/output_traits.h (right): https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/output_traits.... ppapi/cpp/output_traits.h:220: struct CallbackOutputTraits<Array<T> > { On 2013/12/17 18:03:25, dmichael wrote: > Would it be practical to put this in the header for Array instead? I would > rather not add a bunch of includes to this file for clients who don't need them. It seems we put most output traits in this file. For example, the specialization for Var (and others) is also in this file. If we put output traits specialization in different files, it might be harder for readers to figure out which template is applied for a given type . (But I don't have a very strong preference as long as it is consistent.) > > Similarly, it would be nice if the StructWrapper stuff could go somewhere else. > I don't think you need StructWrapperIdentifier. Since the code is all generated > anyway, each "struct wrapper" header can put an explicit full specialization of > CallbackOutputTraits in its header. E.g.: > template <> > CallbackOutputTraits<AlarmDev> > : public StructWrapperCallbackOutputTraits<AlarmDev> { > }; Please see my comments in struct_wrapper_base_dev.h. Thanks!
https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... File ppapi/cpp/dev/struct_wrapper_base_dev.h (right): https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:15: class StructWrapperIdentifier { On 2013/12/17 23:52:21, yzshen1 wrote: > I think one good thing about this approach is that it doesn't need to have > specialized output traits defined in those "user-facing" C++ API headers (e.g., > alarms_dev.h). Similarly, it also leaves part of the public API out of the header. I think a little bit of inscrutable code is probably fine, if it means (a) our maintenance is easier and (b) the header is _in_total_ easier to understand, because it won't hide things in a template base that the developer has to look at and understand (filling in template parameters in their head). > Having less and simpler code (especially internal impl details) > in the C++ API headers may be easier for developers. Although they could read > our documents, I think many of them will still want to look at the headers. I agree with that. I think we might actually be making things worse by removing public API from the header. > I > would like to keep things that they don't need to care out of the API headers, > if possible. If that's important enough to us, we can separate some things out (like specializations) in to a separate header that the developer-facing one includes. E.g.: ppb_foo.h ppb_foo_internal.h I expect that wouldn't be worth the trouble, though. > > And this approach is the same as what we did for pp::Resource subclasses. I > think using the same approach is nice. In my mind, this is different from pp::Resource in a couple of important ways: 1) pp::Resource meets the "is-a" test for those classes. They inherit pp::Resource because they *are* Resources. "StructWrapperBase" feels more like implementation inheritence to me; we don't seem to actually share an API surface there. Certainly not in StructWrapperIdentifier. The only virtual functions don't really need to be virtual if we remove that inheritance. 2) The existing pp::Resource classes already existed when the CompletionCallbackWithOutput stuff was written, and they're not automatically generated. The little "trick" was applied to a base class that already existed to solve this problem for existing code (and future) hand-written code. In this CL, you introduce a base class for the sole purpose of doing the inheritance trick. > > Is it your concern that this approach causes output_traits.h to depend on a file > in the /dev folder? I think once we support the new dev api design, this problem > will be gone. And before that, if we want to avoid the issue, we could move this I don't really mind that; as you say, it's temporary. I just disagree with the tradeoff we're making here. The template goop is complicated and difficult to maintain. We shouldn't add more complexity there lightly. I like using templates for simplifying hand-written code. In this case, the code we're concerned with is generated, so I think we don't get enough benefit from adding the complexity of introducing StructWrapperBase et al. > > StructWrapperIdentifier class to the output_traits.h file. > > What do you think? Thanks! > On 2013/12/17 18:03:25, dmichael wrote: > > I mentioned this elsewhere, but I don't think you need this. You can do an > > explicit full specialization for each "struct wrapper" kind of class. > https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:20: class StructWrapperBase : public StructWrapperIdentifier { On 2013/12/17 23:52:21, yzshen1 wrote: > On 2013/12/17 18:03:25, dmichael wrote: > > It seems slightly messy to me to have this as a base class, when I think it's > > not really necessary. This seems to have two separate responsibilities: > > - A traits style class that does typedefs for you. That could be a separate > > traits class instead of a base class, I think. > > - A data holder (using MayOwnPtr). This has some weird consequences; since > this > > class isn't copyable, you have to use operator= in the derived classes. > > > > Would it be possible to separate this in to two things: > > 1) A "traits" class that only has typedefs and possibly static functions? > > 2) A "holder" class that the generated class can hold as a member? (Possibly > > just MayOwnPtr, I don't know). > > > > It's very possible I'm missing something, but if there's a cleaner way that > > doesn't rely on implementation inheritance, I think I might like that better. > > > > WDYT? > > I made this StructWrapperBase class based on the following reasons: > - As I mentioned above, I wish those "user-facing" headers (such as > alarms_dev.h) to only contain code that developers need to care. Therefore, I > tried to move things needed only by our internal impl (typedef / ToStruct / > StartRawUpdate / EndRawUpdate) into this base class. I know it is somewhat > cheating, they are still public in the subclasses. :) But the headers do seem > cleaner to me. > > - Having the typedef in the struct wrapper doesn't seem too bad to me. It is > reasonable that a struct wrapper has type information about the C struct that it > wraps. If I make separate traits classes, the specialization has to live in > those "user-facing" headers, which I would like to avoid. But probably I could > remove CInputType and COutputType entirely. Alternately, if you need the typedefs, maybe you can generate them directly in the ppb_alarms header. That would be the most straightforward to read and understand. > > - Having |storage_| in the base class is nice because it has to be created > before (and destroyed after) all the accessors that attach to it. (By accessors, > I mean things like Alarms_Dev::name_wrapper_.) The generator can make sure that happens if it's a member, though, right? That might even make the ordering dependency more explicit. > > What do you think? Thanks! https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:36: StructWrapperBase(CType* storage, NotOwned) : storage_(storage, NOT_OWNED) {} On 2013/12/17 23:52:21, yzshen1 wrote: > On 2013/12/17 18:03:25, dmichael wrote: > > If there's not a constructor that takes ownership, why is there all this > > complexity around specifying "NOT_OWNED" and having a MayOwnPtr? > > I was thinking NOT_OWNED makes it more obvious that this constructor doesn't > take ownership. You are right, no constructor will take ownership of passed-in > storage. Do you think I should remove NOT_OWNED and only rely on the comments to > make the ownership clear? > > MayOwnPtr is necessary: Other constructors will create the underlying storage > which is owned by the MayOwnPtr. > > The reason of this design is that a struct wrapper (and many other things such > as Optional<>, Array<>, StringWrapper) can act as a standalone object (owns the > underlying C struct), or an accessor (doesn't own). > > For example: > struct PP_Bar {}; > struct PP_Foo { > struct PP_Bar bar; > }; > > class Foo { > ... > private: > MayOwnPtr<PP_Foo> storage_; > Bar bar_wrapper_; // this accessor is attached to |storage_->bar|, it > doesn't own the memory. > }; > > > Sorry, I realized later that a default-constructed StructWrapperBase would own the memory, and forgot to revisit this comment. I think this is one of the things where a motivating example or test might help. I wish we didn't need these dual ways of owning the data. But I think you're looking ahead to dealing with parameters to the browser (plugin-owned) and parameters to the plugin (browser-owned), and I understand it might be necessary.
Oh, and there is precedent for us having the full specializations of OutputTraits directly in the associated header. I think all/most of our non-resource classes that are passed via CompletionCallbackWithOutput are this way: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/director... https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/dev/true... https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/private/... https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/private/...
https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... File ppapi/cpp/dev/struct_wrapper_base_dev.h (right): https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:15: class StructWrapperIdentifier { Thanks David! I think you persuaded me. I will remove StructWrapperBase/StructWrapperIdentifier and update the CL. On 2013/12/18 18:24:04, dmichael wrote: > On 2013/12/17 23:52:21, yzshen1 wrote: > > I think one good thing about this approach is that it doesn't need to have > > specialized output traits defined in those "user-facing" C++ API headers > (e.g., > > alarms_dev.h). > Similarly, it also leaves part of the public API out of the header. I think a > little bit of inscrutable code is probably fine, if it means (a) our maintenance > is easier and (b) the header is _in_total_ easier to understand, because it > won't hide things in a template base that the developer has to look at and > understand (filling in template parameters in their head). > > Having less and simpler code (especially internal impl details) > > in the C++ API headers may be easier for developers. Although they could read > > our documents, I think many of them will still want to look at the headers. > I agree with that. I think we might actually be making things worse by removing > public API from the header. > > I > > would like to keep things that they don't need to care out of the API headers, > > if possible. > If that's important enough to us, we can separate some things out (like > specializations) in to a separate header that the developer-facing one includes. > E.g.: > ppb_foo.h > ppb_foo_internal.h > > I expect that wouldn't be worth the trouble, though. > > > > > And this approach is the same as what we did for pp::Resource subclasses. I > > think using the same approach is nice. > In my mind, this is different from pp::Resource in a couple of important ways: > 1) pp::Resource meets the "is-a" test for those classes. They inherit > pp::Resource because they *are* Resources. "StructWrapperBase" feels more like > implementation inheritence to me; we don't seem to actually share an API surface > there. Certainly not in StructWrapperIdentifier. The only virtual functions > don't really need to be virtual if we remove that inheritance. > 2) The existing pp::Resource classes already existed when the > CompletionCallbackWithOutput stuff was written, and they're not automatically > generated. The little "trick" was applied to a base class that already existed > to solve this problem for existing code (and future) hand-written code. In this > CL, you introduce a base class for the sole purpose of doing the inheritance > trick. > > > > > Is it your concern that this approach causes output_traits.h to depend on a > file > > in the /dev folder? I think once we support the new dev api design, this > problem > > will be gone. And before that, if we want to avoid the issue, we could move > this > I don't really mind that; as you say, it's temporary. I just disagree with the > tradeoff we're making here. The template goop is complicated and difficult to > maintain. We shouldn't add more complexity there lightly. I like using templates > for simplifying hand-written code. In this case, the code we're concerned with > is generated, so I think we don't get enough benefit from adding the complexity > of introducing StructWrapperBase et al. > > > > > StructWrapperIdentifier class to the output_traits.h file. > > > > What do you think? Thanks! > > On 2013/12/17 18:03:25, dmichael wrote: > > > I mentioned this elsewhere, but I don't think you need this. You can do an > > > explicit full specialization for each "struct wrapper" kind of class. > > > https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:36: StructWrapperBase(CType* storage, NotOwned) : storage_(storage, NOT_OWNED) {} > I think this is one of the things where a motivating example or test might help. > I wish we didn't need these dual ways of owning the data. I did the MayOwnPtr<> design mostly for accessing structure members and array elements. For example, struct PP_Bar {}; struct PP_Foo { struct PP_Bar bar; }; The memory of |bar| is owned by the containing struct PP_Foo. When creating an accessor to wrap |bar|, it cannot manage the memory. Take Alarm_Dev as an example, |period_in_minutes_wrapper_| is directly accessing the member |period_in_minutes| in PP_Alarms_AlarmCreateInfo_Dev. It cannot own that memory. > But I think you're > looking ahead to dealing with parameters to the browser (plugin-owned) and > parameters to the plugin (browser-owned), and I understand it might be > necessary. I considered to support accessor for const pointers (so that we can wrap those input parameters passing from the browser to the plugin, e.g., for event listeners). But I finally decided that the performance gain didn't outweigh the complexity. (I would be happy to talk more about it if you are interested.) For those browser->plugin parameter, currently I am using the Foo(const PP_Foo&) constructor which does a deep copy.
Hi, David. I have made the change to remove StructWrapperIdentifier and StructWrapperBase. Please take a look. Thanks for the suggestions!
Oops, looks like I forgot to publish some comments yesterday. Shouldn't affect what you've done for the new patch. https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... File ppapi/cpp/dev/struct_wrapper_base_dev.h (right): https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:15: class StructWrapperIdentifier { On 2013/12/18 19:18:56, yzshen1 wrote: > Thanks David! > > I think you persuaded me. I will remove > StructWrapperBase/StructWrapperIdentifier and update the CL. Thanks, Yuzhu! I hope it turns out clean. If it causes problems, let me know and we can re-evaluate. https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:36: StructWrapperBase(CType* storage, NotOwned) : storage_(storage, NOT_OWNED) {} On 2013/12/18 19:18:56, yzshen1 wrote: > > I think this is one of the things where a motivating example or test might > help. > > I wish we didn't need these dual ways of owning the data. > I did the MayOwnPtr<> design mostly for accessing structure members and array > elements. > For example, > > struct PP_Bar {}; > struct PP_Foo { > struct PP_Bar bar; > }; > > The memory of |bar| is owned by the containing struct PP_Foo. When creating an > accessor to wrap |bar|, it cannot manage the memory. > > Take Alarm_Dev as an example, |period_in_minutes_wrapper_| is directly accessing > the member |period_in_minutes| in PP_Alarms_AlarmCreateInfo_Dev Do you mean in PP_Alarms_Alarm_Dev? >. It cannot own > that memory. Oops, I missed that detail before. I think I'm more confused :). I might have guessed that the straightforward way to deal with it would be to make the accessor/setter work directly on the PP_Alarms_Alarm_Dev's field? If I was hand-writing it, that's how I think I would do it. Especially if we remove the base class and make the struct a member (either directly or with some helper/wrapper around it), if you can make the code generator just reach directly in to the struct, that seems easier to understand. WDYT? > > > But I think you're > > looking ahead to dealing with parameters to the browser (plugin-owned) and > > parameters to the plugin (browser-owned), and I understand it might be > > necessary. > > I considered to support accessor for const pointers (so that we can wrap those > input parameters passing from the browser to the plugin, e.g., for event > listeners). But I finally decided that the performance gain didn't outweigh the > complexity. (I would be happy to talk more about it if you are interested.) For > those browser->plugin parameter, currently I am using the Foo(const PP_Foo&) > constructor which does a deep copy.
Thanks for making that change; I think it's easier to understand now, and I hope you like it too. As per my last set of comments, I'd like it if we could avoid MayOwnPtr and dual ownership semantics, but I'm still not understanding why it is that way. Is it because you want things like "Optional<>" to be usable as wrappers around a field in a C struct, but _also_ as a way to own a field? My earlier suggestion was to access the struct directly in the generated code. I bet that's possible, but if it's too ugly... Another option might be to make two kinds of Optional. One that's an NonOwningOptional (or something) and a regular Optional. The non-owning version would be for internal use only, so those semantics wouldn't leak out and lead to user confusion or mistakes. Again... it's very likely I'm not understanding all your requirements yet, so I'm sorry if I'm asking silly questions. Thanks! https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/alarms_dev.h File ppapi/cpp/dev/alarms_dev.h (right): https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/alarms_dev... ppapi/cpp/dev/alarms_dev.h:115: void EndRawUpdate(); optional: It might be possible to add friendship to internal::StructWrapperOuptutTraits<AlarmCreateInfo_Dev> and make these private, if you want. https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/optional_d... File ppapi/cpp/dev/optional_dev.h (right): https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/optional_d... ppapi/cpp/dev/optional_dev.h:21: class Optional<double> { optional suggestion: I think when you have more optional types, it might be possible to have 1 template that covers the majority of types (at least fundamental ones like double). It looks like you could use a similar strategy to the Callback traits to get the parameter, return, and storage types. OTOH, if there aren't too many, this style you have now is very readable. So... I guess, just take it as a suggestion you can do in the future to reduce code if it helps.
Hi, David. Thanks for the suggestions! I like the outcomes of removing StructWrapperBase/Identifier. With regard to MayOwnPtr<> and dual ownership, I think we could have an offline discussion. That may be faster. Thanks! :) https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... File ppapi/cpp/dev/struct_wrapper_base_dev.h (right): https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:15: class StructWrapperIdentifier { On 2013/12/19 15:57:44, dmichael wrote: > On 2013/12/18 19:18:56, yzshen1 wrote: > > Thanks David! > > > > I think you persuaded me. I will remove > > StructWrapperBase/StructWrapperIdentifier and update the CL. > Thanks, Yuzhu! I hope it turns out clean. If it causes problems, let me know and > we can re-evaluate. I think it looks nice now. You have a good point that putting part of the public methods in the base class doesn't make it more readable/cleaner. Thanks! https://codereview.chromium.org/116963003/diff/40001/ppapi/cpp/dev/struct_wra... ppapi/cpp/dev/struct_wrapper_base_dev.h:36: StructWrapperBase(CType* storage, NotOwned) : storage_(storage, NOT_OWNED) {} > > Take Alarm_Dev as an example, |period_in_minutes_wrapper_| is directly > accessing > > the member |period_in_minutes| in PP_Alarms_AlarmCreateInfo_Dev > Do you mean in PP_Alarms_Alarm_Dev? Yes. Typo. :) > >. It cannot own > > that memory. > Oops, I missed that detail before. I think I'm more confused :). I might have > guessed that the straightforward way to deal with it would be to make the > accessor/setter work directly on the PP_Alarms_Alarm_Dev's field? If I was > hand-writing it, that's how I think I would do it. Currently I only directly access struct members of primitive types, such as Alarm_Dev::scheduled_time(). All other things are accessed with the help of accessor objects, including Optional<double>. I think it is okay to expand the logic of simple things like Optional<double> into their owner classes. Other things, for example, StringWrapper, OptionalStringWrapper, Array<>, Optional<Array<> > are more complex accessors, it is not suitable to expand the logic into their owner classes, IMO. And also consider struct members (still use the Foo/Bar example mentioned above): class Foo { // We will need to create an accessor for PP_Foo.bar struct member anyway. Bar& bar(); const Bar& bar() const; }; What do you think? > Especially if we remove the base class and make the struct a member (either > directly or with some helper/wrapper around it), if you can make the code > generator just reach directly in to the struct, that seems easier to understand. > > WDYT? https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/alarms_dev.h File ppapi/cpp/dev/alarms_dev.h (right): https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/alarms_dev... ppapi/cpp/dev/alarms_dev.h:115: void EndRawUpdate(); On 2013/12/19 16:34:30, dmichael wrote: > optional: It might be possible to add friendship to > internal::StructWrapperOuptutTraits<AlarmCreateInfo_Dev> and make these private, > if you want. All classes using class T as a member accessor, or Arrary<T>, Optional<T> will need to use to use its Start/EndRawUpdate() as well. So I think it is probably not suitable to add friends. What do you think? https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/optional_d... File ppapi/cpp/dev/optional_dev.h (right): https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/optional_d... ppapi/cpp/dev/optional_dev.h:21: class Optional<double> { On 2013/12/19 16:34:30, dmichael wrote: > optional suggestion: I think when you have more optional types, it might be > possible to have 1 template that covers the majority of types (at least > fundamental ones like double). > It looks like you could use a similar strategy to > the Callback traits to get the parameter, return, and storage types. > > OTOH, if there aren't too many, this style you have now is very readable. So... > I guess, just take it as a suggestion you can do in the future to reduce code if > it helps. I am thinking we could use the generic one for all struct wrapper classes, and add specialized ones for primitive types. What do you think? I haven't added the generic version because it is not needed the alarms API. I think we could add it later. Thanks!
So... as I expected, now that I understand why you did MayOwnPtr, I haven't
thought of anything that's clearly better.
I do have two ideas, both with their own drawbacks. I'd be interested in whether
either sounds interesting to you:
(1)
Instead of making the C++ classes "wrappers", just make them carry the same
fields. They would have a way to convert to/from the C structs, but other than
that, they would be independent.
E.g., if the C looks like this:
struct PP_Bar {
int bar_field;
};
struct PP_Foo {
int foo_field
struct PP_Bar bar;
};
then the C++ would look like this (with stuff omitted):
class Bar {
int bar_field_
public:
Bar();
explicit Bar(const PP_Bar&);
int bar_field() const;
void set_bar_field();
PP_Bar ToCStruct();
};
class Foo {
int foo_field_;
Bar bar_;
public:
Foo();
explicit Foo(const PP_Foo& foo);
int foo_field() const;
void set_foo_field();
Bar& bar(); // simply returns bar_
const Bar& bar(); // ditto
void set_bar(const Bar& bar);
PP_Foo ToCStruct();
};
This would be kind of gross to build by hand, but it seems fine for generated
code. The API will be the same, and the generated code *might* be verbose, but
it will at least be really easy to understand. There's also a slight bit of
performance cost to creating the structs and copying the fields to convert them,
but it's probably not bad (or for cases where it is too expensive, see idea
(2)).
(Optional: You could also only do this style for objects where there is nesting.
Any type that doesn't own other structs could still just wrap its C type)
(2)
Use PP_Resources for these instead.
As I said, these might not be better than what you have... but I wanted to
offer them for your consideration. The MayOwnPtr and NotOwned concerns me
because I think it may be confusing and mistake-prone for plugin authors.
https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/alarms_dev.cc
File ppapi/cpp/dev/alarms_dev.cc (right):
https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/alarms_dev...
ppapi/cpp/dev/alarms_dev.cc:51: return operator=(*other.storage_);
Hmm, this is kind of subtle (especially seeing as it's used in the copy
constructor and the constructor from C struct ).
I gather it works because storage_ is default constructed, which (because of how
MayOwnPtr is written) allocates a new PP_Alarms_Alarm_Dev.
Clearly the Alarm_Dev copy constructor and assignment need to do a full copy
(rather than do NotOwned), and they do. But the way this is written doesn't make
it immediately obvious that that is what happens.
https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/alarms_dev.h
File ppapi/cpp/dev/alarms_dev.h (right):
https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/alarms_dev...
ppapi/cpp/dev/alarms_dev.h:115: void EndRawUpdate();
On 2013/12/19 17:57:58, yzshen1 wrote:
> On 2013/12/19 16:34:30, dmichael wrote:
> > optional: It might be possible to add friendship to
> > internal::StructWrapperOuptutTraits<AlarmCreateInfo_Dev> and make these
> private,
> > if you want.
>
> All classes using class T as a member accessor, or Arrary<T>, Optional<T> will
> need to use to use its Start/EndRawUpdate() as well. So I think it is probably
> not suitable to add friends. What do you think?
Good point, agreed.
https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/may_own_pt...
File ppapi/cpp/dev/may_own_ptr_dev.h (right):
https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/may_own_pt...
ppapi/cpp/dev/may_own_ptr_dev.h:25: MayOwnPtr() : value_(new T()), owned_(true)
{}
(requires that T have a default constructor... probably fine, just something
this design forces)
Thanks for the detailed suggestions! """(1)Instead of making the C++ classes "wrappers", just make them carry the same fields.""" In fact, I followed this approach when I was working on the old, pointer-for-optional-field design. It is simpler WRT storage management. However, it also has some drawbacks: - performance wise, it is more costly. - because Foo and PP_Foo are independent, we need to have helpers to destruct PP_Foo instead of nicely making it the responsibility of Foo destructor. For example, when we want to pass Foo to C API calls, we call Foo::ToStruct() to create a new PP_Foo, which may contain things like PP_Var that require proper destruction, we will need to have a helper DestructStruct(const PP_Foo*). """(Optional: You could also only do this style for objects where there is nesting. Any type that doesn't own other structs could still just wrap its C type)""" Hmm, it seems to me that having a consistent way may be easier to understand. """(2) Use PP_Resources for these instead.""" IMO, that may bloat our thunk and interface registration. Besides, as PP_Resources, we allow them to be accessible from multiple threads. We will need to acquire lock when accessing the data fields. I am afraid that will be too costly. """The MayOwnPtr and NotOwned concerns me because I think it may be confusing and mistake-prone for plugin authors.""" I personally think it should be okay: - MayOwnPtr<> is an implementation detail (inside the internal:: namespace). The plugin authors are not supposed to use it or care about it. - The public API of those struct wrappers such as Alarms_Dev is (hopefully) not complex: * constructors / assignment operators. * accessors to data fields. * conversion to C struct (const and non-const). Even if we choose another storage approach, we probably will have a very similar public API. - The only annoyance to me is the NOT_OWNED constructor. :/ I don't have a good way to hide it from the public section. I added comments for it, and also explicitly added the NOT_OWNED annotation. I hope it won't confuse plugin authors. (Maybe I should also add "For internal implementation, you don't need to use this constructor!") What do you think? Thanks! https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/alarms_dev.cc File ppapi/cpp/dev/alarms_dev.cc (right): https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/alarms_dev... ppapi/cpp/dev/alarms_dev.cc:51: return operator=(*other.storage_); On 2013/12/20 18:38:00, dmichael wrote: > Hmm, this is kind of subtle (especially seeing as it's used in the copy > constructor and the constructor from C struct ). > > I gather it works because storage_ is default constructed, which (because of how > MayOwnPtr is written) allocates a new PP_Alarms_Alarm_Dev. > > Clearly the Alarm_Dev copy constructor and assignment need to do a full copy > (rather than do NotOwned), and they do. But the way this is written doesn't make > it immediately obvious that that is what happens. My thoughts when I designed this logic were: - |storage_| is not allowed to be copied or assigned itself. (I disabled copy and assign on MayOwnPtr<>) - The assignment of struct wrappers (or things like Array<T>) calls those accessors which are attached to |storage_|, they will do deep copy (e.g., array) and ref-counting (e.g., PP_Var) correctly and modify |storage_|. What do you think? https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/may_own_pt... File ppapi/cpp/dev/may_own_ptr_dev.h (right): https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/may_own_pt... ppapi/cpp/dev/may_own_ptr_dev.h:25: MayOwnPtr() : value_(new T()), owned_(true) {} On 2013/12/20 18:38:00, dmichael wrote: > (requires that T have a default constructor... probably fine, just something > this design forces) Right, all things that we will used with MayOwnPtr<> are C types. So when we do T(), it will do zero initialization. I added a comment.
lgtm, thanks for being patient with me coming to understand why you've chosen to go this way. I just have a couple of comments about comments. https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/alarms_dev.cc File ppapi/cpp/dev/alarms_dev.cc (right): https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/alarms_dev... ppapi/cpp/dev/alarms_dev.cc:51: return operator=(*other.storage_); On 2013/12/20 19:50:43, yzshen1 wrote: > On 2013/12/20 18:38:00, dmichael wrote: > > Hmm, this is kind of subtle (especially seeing as it's used in the copy > > constructor and the constructor from C struct ). > > > > I gather it works because storage_ is default constructed, which (because of > how > > MayOwnPtr is written) allocates a new PP_Alarms_Alarm_Dev. > > > > Clearly the Alarm_Dev copy constructor and assignment need to do a full copy > > (rather than do NotOwned), and they do. But the way this is written doesn't > make > > it immediately obvious that that is what happens. > > My thoughts when I designed this logic were: > - |storage_| is not allowed to be copied or assigned itself. (I disabled copy > and assign on MayOwnPtr<>) > - The assignment of struct wrappers (or things like Array<T>) calls those > accessors which are attached to |storage_|, they will do deep copy (e.g., array) > and ref-counting (e.g., PP_Var) correctly and modify |storage_|. I see, that's why it would be wrong to simply make MayOwnPtr copyable/assignable. > > What do you think? I could see it was right, but was wishing there was a simpler way :). I pointed it out as one of the things about the MayOwnPtr design that was a shame. I still don't have a clearly better answer. https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/array_dev.h File ppapi/cpp/dev/array_dev.h (right): https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/array_dev.... ppapi/cpp/dev/array_dev.h:125: void Reset(uint32_t size) { Hmm, I wonder if/when you start using Array in-params, if we'll want more sophisticated ways to build an Array efficiently (like push_back, basically). I think you're doing the right thing for now. But something to keep in mind for later, perhaps. By the way: I think it would be good to add a comment making it clear what this does. I think the name is excellent, but there's still a small chance somebody would make the mistake of thinking it has "resize()" semantics. https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/string_wra... File ppapi/cpp/dev/string_wrapper_dev.cc (right): https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/string_wra... ppapi/cpp/dev/string_wrapper_dev.cc:21: : storage_(storage, NOT_OWNED) { May be worth noting that while it does *not* own the PP_Var memory in this case, this wrapper still owns a reference. That confused me for a little bit.
Thanks David! And have a nice holiday! :) https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/array_dev.h File ppapi/cpp/dev/array_dev.h (right): https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/array_dev.... ppapi/cpp/dev/array_dev.h:125: void Reset(uint32_t size) { On 2013/12/20 22:01:09, dmichael wrote: > Hmm, I wonder if/when you start using Array in-params, if we'll want more > sophisticated ways to build an Array efficiently (like push_back, basically). I > think you're doing the right thing for now. But something to keep in mind for > later, perhaps. We could use a std::vector<> as underlying storage if we want to support push_back. When ToStruct() is called, make a C array struct whose |elements| pointer points to the underlying storage of std::vector<>. That should be quite efficient. It should be easy to extend this class later in that direction with backward compatibility. So I think I won't do it for now. > By the way: I think it would be good to add a comment making it clear what this > does. I think the name is excellent, but there's still a small chance somebody > would make the mistake of thinking it has "resize()" semantics. Done. Thanks! https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/string_wra... File ppapi/cpp/dev/string_wrapper_dev.cc (right): https://codereview.chromium.org/116963003/diff/80001/ppapi/cpp/dev/string_wra... ppapi/cpp/dev/string_wrapper_dev.cc:21: : storage_(storage, NOT_OWNED) { On 2013/12/20 22:01:09, dmichael wrote: > May be worth noting that while it does *not* own the PP_Var memory in this case, > this wrapper still owns a reference. > > That confused me for a little bit. Done. I added comments for .*StringWrapper and also Array<>.
+binji as OWNER for library.dsc. Thanks, Ben!
native_client_sdk lgtm
https://codereview.chromium.org/116963003/diff/180001/ppapi/cpp/dev/alarms_dev.h File ppapi/cpp/dev/alarms_dev.h (right): https://codereview.chromium.org/116963003/diff/180001/ppapi/cpp/dev/alarms_de... ppapi/cpp/dev/alarms_dev.h:56: bool is_period_in_minutes_set() const; I would prefer has_period_in_minutes() and clear_period_in_minutes() to match protobufs. https://codereview.chromium.org/116963003/diff/180001/ppapi/cpp/dev/array_dev.h File ppapi/cpp/dev/array_dev.h (right): https://codereview.chromium.org/116963003/diff/180001/ppapi/cpp/dev/array_dev... ppapi/cpp/dev/array_dev.h:154: void EndRawUpdate() { CreateWrappers(); } In this case the storage passed to the wrapper constructor won't be zero-initialized. https://codereview.chromium.org/116963003/diff/180001/ppapi/cpp/dev/optional_... File ppapi/cpp/dev/optional_dev.h (right): https://codereview.chromium.org/116963003/diff/180001/ppapi/cpp/dev/optional_... ppapi/cpp/dev/optional_dev.h:79: Optional(const std::string& value) : wrapper_(value) {} How about a const char* overload?
Thanks Sam! Please take another look. https://codereview.chromium.org/116963003/diff/180001/ppapi/cpp/dev/alarms_dev.h File ppapi/cpp/dev/alarms_dev.h (right): https://codereview.chromium.org/116963003/diff/180001/ppapi/cpp/dev/alarms_de... ppapi/cpp/dev/alarms_dev.h:56: bool is_period_in_minutes_set() const; On 2013/12/22 22:44:33, Sam McNally wrote: > I would prefer has_period_in_minutes() and clear_period_in_minutes() to match > protobufs. IMO, it is good to match the definition of PP_Optional_* (say, PP_Optinoal_Double) which has |is_set|. I also made Option<> use is_set(). WRT to unset v.s. clear, I don't have a strong opinion. Maybe it is not very important to match protobufs, because it doesn't have anything to do with protobufs. What do you think? https://codereview.chromium.org/116963003/diff/180001/ppapi/cpp/dev/array_dev.h File ppapi/cpp/dev/array_dev.h (right): https://codereview.chromium.org/116963003/diff/180001/ppapi/cpp/dev/array_dev... ppapi/cpp/dev/array_dev.h:154: void EndRawUpdate() { CreateWrappers(); } Good catch! Thanks! I didn't notice this issue when I switched from the PP_ArrayOutput-in-each-array-struct approach to the current general-array-allocator-in-method-signature approach. Previously it was okay to require that wrappers only accept zero-init storage, because Array<> does the memory allocation itself and is able to create wrappers right after allocation. But now I have to change to allow storage with meaningful data to be passed into wrappers. Thanks! On 2013/12/22 22:44:33, Sam McNally wrote: > In this case the storage passed to the wrapper constructor won't be > zero-initialized. https://codereview.chromium.org/116963003/diff/180001/ppapi/cpp/dev/optional_... File ppapi/cpp/dev/optional_dev.h (right): https://codereview.chromium.org/116963003/diff/180001/ppapi/cpp/dev/optional_... ppapi/cpp/dev/optional_dev.h:79: Optional(const std::string& value) : wrapper_(value) {} On 2013/12/22 22:44:33, Sam McNally wrote: > How about a const char* overload? Sounds good. Added.
lgtm https://codereview.chromium.org/116963003/diff/180001/ppapi/cpp/dev/alarms_dev.h File ppapi/cpp/dev/alarms_dev.h (right): https://codereview.chromium.org/116963003/diff/180001/ppapi/cpp/dev/alarms_de... ppapi/cpp/dev/alarms_dev.h:56: bool is_period_in_minutes_set() const; On 2013/12/26 19:19:51, yzshen1 wrote: > On 2013/12/22 22:44:33, Sam McNally wrote: > > I would prefer has_period_in_minutes() and clear_period_in_minutes() to match > > protobufs. > > IMO, it is good to match the definition of PP_Optional_* (say, > PP_Optinoal_Double) which has |is_set|. I also made Option<> use is_set(). > > WRT to unset v.s. clear, I don't have a strong opinion. > > Maybe it is not very important to match protobufs, because it doesn't have > anything to do with protobufs. What do you think? They aren't related, but they have a similar feel and purpose. I don't feel strongly about it, but I'd prefer to not have yet another naming scheme for methods that basically do the same thing.
> They aren't related, but they have a similar feel and purpose. I don't feel > strongly about it, but I'd prefer to not have yet another naming scheme for > methods that basically do the same thing. I think maybe it is more important to match Mojo bindings. I took a quick look at how Mojo names optional fields. But I didn't find it. Here seems to be an optional field |name|: https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/bindin... Here is the corresponding accessor of |name|: https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/mojo... I will go ahead and land this CL. But I will think more and get back to you about the naming issue. Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yzshen@chromium.org/116963003/430001
Message was sent while issue was closed.
Change committed as 243158 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
