Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2234823005:
Initial docs on using the C bindings. (Closed)
|
Created:
4 years, 4 months ago by vardhan Modified:
4 years, 3 months ago Reviewers:
viettrungluu CC:
mojo-reviews_chromium.org, gregsimon, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org, yzshen+mojopublicwatch_chromium.org Base URL:
git@github.com:domokit/mojo.git@master Target Ref:
refs/heads/master Project:
mojo Visibility:
Public. |
DescriptionInitial docs on using the C bindings.
R=viettrungluu@chromium.org
Committed: https://chromium.googlesource.com/external/mojo/+/af959944defdfefa43fed8790da2e55d68bb5c71
Patch Set 1 #Patch Set 2 : spacing #
Total comments: 13
Patch Set 3 : address comments, and rephrase a few more things #
Total comments: 16
Patch Set 4 : address comments #
Total comments: 8
Patch Set 5 : address comments #Patch Set 6 : Initial doc for the mojom protocol. #Messages
Total messages: 11 (1 generated)
ptal
You'll want to rebase. I've reorganized mojo/public/c. This should either go in a new directory, //mojo/public/docs/bindings. Or somewhere under //docs (maybe in a new subdirectory of it).
https://codereview.chromium.org/2234823005/diff/20001/mojo/public/c/bindings/... File mojo/public/c/bindings/README.md (right): https://codereview.chromium.org/2234823005/diff/20001/mojo/public/c/bindings/... mojo/public/c/bindings/README.md:17: Lets look at what the generated code looks like for the following struct: Let's https://codereview.chromium.org/2234823005/diff/20001/mojo/public/c/bindings/... mojo/public/c/bindings/README.md:61: present in the mojom. It is important to read the generated struct to make sure The sentence beginning "It is important" doesn't make sense to me. https://codereview.chromium.org/2234823005/diff/20001/mojo/public/c/bindings/... mojo/public/c/bindings/README.md:64: Since mojom objects appear in the order of when they are referenced, we can use "when" doesn't really make sense. https://codereview.chromium.org/2234823005/diff/20001/mojo/public/c/bindings/... mojo/public/c/bindings/README.md:126: The generated `example_Person_EncodePointersAndHandles(..)` is used to encode You really love the passive voice, don't you? https://codereview.chromium.org/2234823005/diff/20001/mojo/public/c/bindings/... mojo/public/c/bindings/README.md:316: array contains only the 8-byte pointers (or offsets in its encoded form) -- the objects' data It'd be nice to wrap text (outside of quoted code, obviously) to 80 cols. https://codereview.chromium.org/2234823005/diff/20001/mojo/public/c/bindings/... mojo/public/c/bindings/README.md:334: Since strings are just arrays of 1-byte characters, you can use Strictly speaking, strings are more than that, since they're supposed to be UTF-8 encoded.
On 2016/08/11 17:10:09, viettrungluu wrote: > You'll want to rebase. > > I've reorganized mojo/public/c. > > This should either go in a new directory, //mojo/public/docs/bindings. Or > somewhere under //docs (maybe in a new subdirectory of it). moved to //mojo/public/c/docs (so that other repos consuming just `mojo/public/c` will get the docs)
ptal https://codereview.chromium.org/2234823005/diff/20001/mojo/public/c/bindings/... File mojo/public/c/bindings/README.md (right): https://codereview.chromium.org/2234823005/diff/20001/mojo/public/c/bindings/... mojo/public/c/bindings/README.md:17: Lets look at what the generated code looks like for the following struct: On 2016/08/11 17:15:38, viettrungluu wrote: > Let's Done. https://codereview.chromium.org/2234823005/diff/20001/mojo/public/c/bindings/... mojo/public/c/bindings/README.md:61: present in the mojom. It is important to read the generated struct to make sure On 2016/08/11 17:15:39, viettrungluu wrote: > The sentence beginning "It is important" doesn't make sense to me. clarified. https://codereview.chromium.org/2234823005/diff/20001/mojo/public/c/bindings/... mojo/public/c/bindings/README.md:64: Since mojom objects appear in the order of when they are referenced, we can use On 2016/08/11 17:15:38, viettrungluu wrote: > "when" doesn't really make sense. I changed this to "appear in depth-first order relative to their parent object," which i think makes sense but probably needs an extra explanation, but I think that explanation belongs in a different doc (one that explains the mojom encoding format) https://codereview.chromium.org/2234823005/diff/20001/mojo/public/c/bindings/... mojo/public/c/bindings/README.md:126: The generated `example_Person_EncodePointersAndHandles(..)` is used to encode On 2016/08/11 17:15:38, viettrungluu wrote: > You really love the passive voice, don't you? Maybe rephrasing this to be "C structs can be encoded using `..`" but i feel that keeping this common sentence structure while describing the generated functions makes the text easier to consume *shrug*. happy to change it if you find it is bothersome though. https://codereview.chromium.org/2234823005/diff/20001/mojo/public/c/bindings/... mojo/public/c/bindings/README.md:316: array contains only the 8-byte pointers (or offsets in its encoded form) -- the objects' data On 2016/08/11 17:15:38, viettrungluu wrote: > It'd be nice to wrap text (outside of quoted code, obviously) to 80 cols. Done. https://codereview.chromium.org/2234823005/diff/20001/mojo/public/c/bindings/... mojo/public/c/bindings/README.md:334: Since strings are just arrays of 1-byte characters, you can use On 2016/08/11 17:15:38, viettrungluu wrote: > Strictly speaking, strings are more than that, since they're supposed to be > UTF-8 encoded. Done.
https://codereview.chromium.org/2234823005/diff/20001/mojo/public/c/bindings/... File mojo/public/c/bindings/README.md (right): https://codereview.chromium.org/2234823005/diff/20001/mojo/public/c/bindings/... mojo/public/c/bindings/README.md:126: The generated `example_Person_EncodePointersAndHandles(..)` is used to encode On 2016/08/11 21:48:33, vardhan wrote: > On 2016/08/11 17:15:38, viettrungluu wrote: > > You really love the passive voice, don't you? > > Maybe rephrasing this to be "C structs can be encoded using `..`" but i feel > that keeping this common sentence structure while describing the generated > functions makes the text easier to consume *shrug*. > > happy to change it if you find it is bothersome though. It is not particularly bothersome to me, but it is recommended by most style guides to prefer the active voice over the passive. https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... File mojo/public/c/docs/bindings/TUTORIAL.md (right): https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:190: request message needs to be constructed. An interface request message for s/needs to/must/ https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:194: - the message ordinal (generated above) which represents which message it is. nit: 80 cols https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:214: the `ordinal` describing the message. By checking if it's any of The sentence "By checking ..." seems a bit strange to me: a) If you're the "server" end, then you should validate that it is a valid request by checking that it's a known ordinal. b) Whether a request expects a response depends on the ordinal (though the flag should also be checked). Conversely, if you're the "client" end, then everything must be (checked to be) a response. (You still have to check the ordinal, of course, and additionally check that it's a response that you're expecting.) (Also, the "or" in that sentence is rather ambiguous.) https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:303: tags. The unknown tag isn't meant to be encoded over the wire, and exists as an We really should rename "unknown" to "unset" (not in this CL). https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:304: initial value for a union's tag, but the tag should be set to something else "should be" -> "must be" "something else" is probably too vague; maybe "a valid tag". https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:306: are no functions generated for unions like they are for structs, since unions "like they are for" -> "unlike for" https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:307: aren't ever encoded as a top-level data type that the programmer should have to "aren't ever" -> "are never" Also, "that the programmer ..." is probably unnecessary, but if you want to keep it, it should be "write to *the* wire". https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:359: of elements, and neither arrays can be null. s/arrays/array/ (or just write it as "neither may be null") s/can/may/
ptal https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... File mojo/public/c/docs/bindings/TUTORIAL.md (right): https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:190: request message needs to be constructed. An interface request message for On 2016/08/11 22:02:42, viettrungluu wrote: > s/needs to/must/ Done. https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:194: - the message ordinal (generated above) which represents which message it is. On 2016/08/11 22:02:41, viettrungluu wrote: > nit: 80 cols oops, these are tabs instead of spaces. https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:214: the `ordinal` describing the message. By checking if it's any of On 2016/08/11 22:02:41, viettrungluu wrote: > The sentence "By checking ..." seems a bit strange to me: > > a) If you're the "server" end, then you should validate that it is a valid > request by checking that it's a known ordinal. > b) Whether a request expects a response depends on the ordinal (though the flag > should also be checked). > > Conversely, if you're the "client" end, then everything must be (checked to be) > a response. (You still have to check the ordinal, of course, and additionally > check that it's a response that you're expecting.) > > (Also, the "or" in that sentence is rather ambiguous.) Done. I think there need to be more things generated for interfaces in C that help with "is this a valid message, should this request expect a response, etc."; c/tests/validation_unittest.cc has a whole lot of boilerplate code to deal with it, so anyone using interfaces in C will have to suffer a tiny bit.. https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:303: tags. The unknown tag isn't meant to be encoded over the wire, and exists as an On 2016/08/11 22:02:42, viettrungluu wrote: > We really should rename "unknown" to "unset" (not in this CL). Acknowledged. https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:304: initial value for a union's tag, but the tag should be set to something else On 2016/08/11 22:02:41, viettrungluu wrote: > "should be" -> "must be" > > "something else" is probably too vague; maybe "a valid tag". I think it should be "should be" because the validation tests check that unknown tags are valid on the wire.. https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:307: aren't ever encoded as a top-level data type that the programmer should have to On 2016/08/11 22:02:42, viettrungluu wrote: > "aren't ever" -> "are never" > > Also, "that the programmer ..." is probably unnecessary, but if you want to keep > it, it should be "write to *the* wire". I changed it to "never encoded as a top level object on the wire" https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:359: of elements, and neither arrays can be null. On 2016/08/11 22:02:41, viettrungluu wrote: > s/arrays/array/ (or just write it as "neither may be null") > s/can/may/ Done.
lgtm w/minor fixes https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... File mojo/public/c/docs/bindings/TUTORIAL.md (right): https://codereview.chromium.org/2234823005/diff/40001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:304: initial value for a union's tag, but the tag should be set to something else On 2016/08/12 00:15:14, vardhan wrote: > On 2016/08/11 22:02:41, viettrungluu wrote: > > "should be" -> "must be" > > > > "something else" is probably too vague; maybe "a valid tag". > > I think it should be "should be" because the validation tests check that > unknown tags are valid on the wire.. As discussed, those tests are trying to test for something different, so "must" is correct. https://codereview.chromium.org/2234823005/diff/60001/mojo/public/c/docs/bind... File mojo/public/c/docs/bindings/TUTORIAL.md (right): https://codereview.chromium.org/2234823005/diff/60001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:145: example_Person`. If valid, returns `MOJOM_VALIDATION_ERROR_NONE`, and can be "If valid ..." is not a sentence: it's missing a subject. Moreover, it has faulty parallelism: the subjects of "returns" and "can be decoded" are not the same. https://codereview.chromium.org/2234823005/diff/60001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:151: referenced objects alone; communication happens via interface calls, so we need Maybe clarify that "interface calls" really means "sending messages" (specified by an interface). https://codereview.chromium.org/2234823005/diff/60001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:153: for interfaces. Consider an interface `Population` with a method `GetPerson()` s/method/message/ (or at least clarify that calling a "method" really just means sending a message) https://codereview.chromium.org/2234823005/diff/60001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:159: [ServiceName="example::EmployeeRegistry"] We really should stop using "::" in service names, and use '.' instead.
https://codereview.chromium.org/2234823005/diff/60001/mojo/public/c/docs/bind... File mojo/public/c/docs/bindings/TUTORIAL.md (right): https://codereview.chromium.org/2234823005/diff/60001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:145: example_Person`. If valid, returns `MOJOM_VALIDATION_ERROR_NONE`, and can be On 2016/08/12 16:47:21, viettrungluu wrote: > "If valid ..." is not a sentence: it's missing a subject. > > Moreover, it has faulty parallelism: the subjects of "returns" and "can be > decoded" are not the same. Done. https://codereview.chromium.org/2234823005/diff/60001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:151: referenced objects alone; communication happens via interface calls, so we need On 2016/08/12 16:47:21, viettrungluu wrote: > Maybe clarify that "interface calls" really means "sending messages" (specified > by an interface). Done. https://codereview.chromium.org/2234823005/diff/60001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:153: for interfaces. Consider an interface `Population` with a method `GetPerson()` On 2016/08/12 16:47:21, viettrungluu wrote: > s/method/message/ (or at least clarify that calling a "method" really just means > sending a message) Done. https://codereview.chromium.org/2234823005/diff/60001/mojo/public/c/docs/bind... mojo/public/c/docs/bindings/TUTORIAL.md:159: [ServiceName="example::EmployeeRegistry"] On 2016/08/12 16:47:21, viettrungluu wrote: > We really should stop using "::" in service names, and use '.' instead. I'll change it there to '.' and make a bug to change :: to . in ServiceNames
Description was changed from ========== Initial docs on using the C bindings. R=viettrungluu@chromium.org ========== to ========== Initial docs on using the C bindings. R=viettrungluu@chromium.org Committed: https://chromium.googlesource.com/external/mojo/+/af959944defdfefa43fed8790da... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as af959944defdfefa43fed8790da2e55d68bb5c71 (presubmit successful). |
