Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6246117:
Cleaning up and adding new docs (Closed)
|
Created:
9 years, 10 months ago by jond Modified:
9 years, 7 months ago Reviewers:
dmichael(do not use this one) CC:
chromium-reviews, piman+watch_chromium.org, josiew_google.com, awatson1, jhartman_google.com Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionPatch Set 1 #
Total comments: 52
Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 10
Messages
Total messages: 8 (0 generated)
More documentation updates for Pepper C API. A couple of things: I did not rewrite everything, so some stuff in the PPB_ files might not be great. Just trying to get a handle on how to format it and technical accuracy. Also, missing some explanations in pp_input_event.h for mouse wheel events. Hopefully you can provide some explanation.
Some comments. Possibly more to come. Sorry for the delay. http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h File ppapi/c/pp_input_event.h (right): http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcode10 ppapi/c/pp_input_event.h:10: * This file defines the APIs used to handle mouse and keyboard modifier input remove 'modifier ' http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcode26 ppapi/c/pp_input_event.h:26: * This enumeration contains enumorators representing each mouse button. Sounds redundant... maybe 'PP_InputEvent_MouseButton is an enumerated type representing the mouse button which is associated with a mouse event.' (Or something http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcode45 ppapi/c/pp_input_event.h:45: * This enumeration contains mouse and keyboard event enumorators. Similarly maybe reword to take out 1 of the 'enum' words. Also, enumorators should be 'enumerators'. http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcode72 ppapi/c/pp_input_event.h:72: * This enumeration contains modifier key event enumorators. Similar... and s/modifier key event/event modifier (Mouse events can get modified as well) http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcod... ppapi/c/pp_input_event.h:104: * If the user looses focus on the module while a key is down, a key up s/looses/loses http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcod... ppapi/c/pp_input_event.h:114: /** This value represents a combination of the EVENT_MODIFIER flags. */ maybe should point out it's a bit field... http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcod... ppapi/c/pp_input_event.h:177: * The PP_InputEvent_Mouse struct represents all mouse events other except remove 'other' http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcod... ppapi/c/pp_input_event.h:181: /** This value represents a combination of the EVENT_MODIFIER flags. */ as a bit field. http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcod... ppapi/c/pp_input_event.h:192: * This values represents x coordinate of the mouse when the event s/x/the x/ http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcod... ppapi/c/pp_input_event.h:202: * This values represents y coordinate of the mouse when the event s/y/the y/ http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcod... ppapi/c/pp_input_event.h:232: /** This value represents */ represents what? (I'm not totally clear on that myself :o)) http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_stdint.h File ppapi/c/pp_stdint.h (right): http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_stdint.h#newcode22 ppapi/c/pp_stdint.h:22: /** This value represents a guaraneteed unsighed 8 bit integer */ s/guaraneteed/guaranteed/ s/unsighed/unsigned/ (and guaraneteed shows up in a few below, too) Also, these should probably all end with a period. http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb.h File ppapi/c/ppb.h (right): http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb.h#newcode11 ppapi/c/ppb.h:11: * by the browser. Not sure about this wording... maybe instead say 'This file defines a function pointer type for the PPB_GetInterface function.' Or something.
http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb.h File ppapi/c/ppb.h (right): http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb.h#newcode20 ppapi/c/ppb.h:20: * This value contains a pointer to an interface supported by the browser This is defining a function type, not a value. It's going to be tricky to be both accurate and understandable. The good news is, it should be pretty familiar to experienced C developers. Maybe "This function pointer type defines the signature for the PPB_GetInterface function. [then the paragraph that's currently 2nd describing how you get the PPB_GetInterface function pointer that you can actually invoke]. I'd move the 2nd part of this paragraph in to its own paragraph. http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb.h#newcode21 ppapi/c/ppb.h:21: * (given the interface name). Browser interface names should be ASCII "should be ASCII" is worded for PPAPI developers who are defining new interfaces. If our audience is primarily users of PPAPI, we should instead say "Browser interface names are ASCII strings..." (note "should be" -> "are") http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb_instance.h File ppapi/c/ppb_instance.h (right): http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb_instance.h#newcode32 ppapi/c/ppb_instance.h:32: * This value represents a pointer to a function that determines This is not a value, it's a function pointer type. But I wonder if it would be good to dispense with absolute accuracy and instead say 'This function returns... (more like the original but in a cleaner, more complete sentence). I think it maps to how programmers think of it. And all the other function definitions here should be that way too. This struct is roughly like defining a class in C++ with a set of methods; in this case, we should probably speak to developers' mental models and dispense with overly verbose correctness. http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb_instance.h#newcode35 ppapi/c/ppb_instance.h:35: * @param[in] instance A PP_Instance indentifying one instance of a module. 'A PP_Instance identifying one instance of a module.' -> 'The PP_Instance whose WindowObject should be retrieved.'? Point being it's not just any PP_Instance. http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppp_instance.h File ppapi/c/ppp_instance.h (right): http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppp_instance.h#newcode32 ppapi/c/ppp_instance.h:32: * unless you want your model to handel events such as a change of focus or s/model/module s/handel/handle I also don't think the last sentence is true. The methods need to have bodies, but the bodies can be trivial (0 lines or 1 line to return a default return). http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppp_instance.h#newcode56 ppapi/c/ppp_instance.h:56: * embed id="nacl_module" dimensions="2"> s/embed/<embed http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppp_instance.h#newcode58 ppapi/c/ppp_instance.h:58: * @param[in] argv An array of argument values. These are the values of the "These are the values of the arguments listed in the <embed> tag." Redundant... delete? http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppp_instance.h#newcode59 ppapi/c/ppp_instance.h:59: * arguments listed in the <embed> tag. In the previous example, there will 'the previous'->'this'
http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h File ppapi/c/pp_input_event.h (right): http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcode10 ppapi/c/pp_input_event.h:10: * This file defines the APIs used to handle mouse and keyboard modifier input On 2011/02/08 17:04:38, dmichael wrote: > remove 'modifier ' Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcode26 ppapi/c/pp_input_event.h:26: * This enumeration contains enumorators representing each mouse button. You're rewrite seemed too verbose. From what I can tell enumeration containing enumerators is correct. Sounds reduant, but I don't think it is, but it does sound a bit awkward. I'll use the word constants if you like that better. On 2011/02/08 17:04:38, dmichael wrote: > Sounds redundant... maybe 'PP_InputEvent_MouseButton is an enumerated type > representing the mouse button which is associated with a mouse event.' (Or > something http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcode45 ppapi/c/pp_input_event.h:45: * This enumeration contains mouse and keyboard event enumorators. I changed to "constants." On 2011/02/08 17:04:38, dmichael wrote: > Similarly maybe reword to take out 1 of the 'enum' words. Also, enumorators > should be 'enumerators'. http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcode72 ppapi/c/pp_input_event.h:72: * This enumeration contains modifier key event enumorators. On 2011/02/08 17:04:38, dmichael wrote: > Similar... and s/modifier key event/event modifier > (Mouse events can get modified as well) Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcode72 ppapi/c/pp_input_event.h:72: * This enumeration contains modifier key event enumorators. On 2011/02/08 17:04:38, dmichael wrote: > Similar... and s/modifier key event/event modifier > (Mouse events can get modified as well) Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcod... ppapi/c/pp_input_event.h:104: * If the user looses focus on the module while a key is down, a key up On 2011/02/08 17:04:38, dmichael wrote: > s/looses/loses Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcod... ppapi/c/pp_input_event.h:104: * If the user looses focus on the module while a key is down, a key up On 2011/02/08 17:04:38, dmichael wrote: > s/looses/loses Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcod... ppapi/c/pp_input_event.h:114: /** This value represents a combination of the EVENT_MODIFIER flags. */ On 2011/02/08 17:04:38, dmichael wrote: > maybe should point out it's a bit field... Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcod... ppapi/c/pp_input_event.h:177: * The PP_InputEvent_Mouse struct represents all mouse events other except On 2011/02/08 17:04:38, dmichael wrote: > remove 'other' Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcod... ppapi/c/pp_input_event.h:181: /** This value represents a combination of the EVENT_MODIFIER flags. */ On 2011/02/08 17:04:38, dmichael wrote: > as a bit field. Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcod... ppapi/c/pp_input_event.h:181: /** This value represents a combination of the EVENT_MODIFIER flags. */ On 2011/02/08 17:04:38, dmichael wrote: > as a bit field. Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcod... ppapi/c/pp_input_event.h:192: * This values represents x coordinate of the mouse when the event On 2011/02/08 17:04:38, dmichael wrote: > s/x/the x/ Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcod... ppapi/c/pp_input_event.h:192: * This values represents x coordinate of the mouse when the event On 2011/02/08 17:04:38, dmichael wrote: > s/x/the x/ Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcod... ppapi/c/pp_input_event.h:202: * This values represents y coordinate of the mouse when the event On 2011/02/08 17:04:38, dmichael wrote: > s/y/the y/ Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_stdint.h File ppapi/c/pp_stdint.h (right): http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_stdint.h#newcode22 ppapi/c/pp_stdint.h:22: /** This value represents a guaraneteed unsighed 8 bit integer */ On 2011/02/08 17:04:38, dmichael wrote: > s/guaraneteed/guaranteed/ > s/unsighed/unsigned/ > (and guaraneteed shows up in a few below, too) > Also, these should probably all end with a period. Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb.h File ppapi/c/ppb.h (right): http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb.h#newcode11 ppapi/c/ppb.h:11: * by the browser. On 2011/02/08 17:04:38, dmichael wrote: > Not sure about this wording... maybe instead say 'This file defines a function > pointer type for the PPB_GetInterface function.' Or something. Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb.h#newcode11 ppapi/c/ppb.h:11: * by the browser. On 2011/02/08 17:04:38, dmichael wrote: > Not sure about this wording... maybe instead say 'This file defines a function > pointer type for the PPB_GetInterface function.' Or something. Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb.h#newcode20 ppapi/c/ppb.h:20: * This value contains a pointer to an interface supported by the browser I think I got it. You'll have to check. On 2011/02/08 21:51:17, dmichael wrote: > This is defining a function type, not a value. It's going to be tricky to be > both accurate and understandable. The good news is, it should be pretty > familiar to experienced C developers. > > Maybe "This function pointer type defines the signature for the PPB_GetInterface > function. [then the paragraph that's currently 2nd describing how you get the > PPB_GetInterface function pointer that you can actually invoke]. I'd move the > 2nd part of this paragraph in to its own paragraph. http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb.h#newcode21 ppapi/c/ppb.h:21: * (given the interface name). Browser interface names should be ASCII On 2011/02/08 21:51:17, dmichael wrote: > "should be ASCII" is worded for PPAPI developers who are defining new > interfaces. If our audience is primarily users of PPAPI, we should instead say > "Browser interface names are ASCII strings..." (note "should be" -> "are") Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb.h#newcode21 ppapi/c/ppb.h:21: * (given the interface name). Browser interface names should be ASCII On 2011/02/08 21:51:17, dmichael wrote: > "should be ASCII" is worded for PPAPI developers who are defining new > interfaces. If our audience is primarily users of PPAPI, we should instead say > "Browser interface names are ASCII strings..." (note "should be" -> "are") Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb_instance.h File ppapi/c/ppb_instance.h (right): http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb_instance.h#newcode32 ppapi/c/ppb_instance.h:32: * This value represents a pointer to a function that determines I queried the group and we're going to go with this: "GetWindowObject is a pointer to a function that determines the DOM window containing this module instance." - Jon On 2011/02/08 21:51:17, dmichael wrote: > This is not a value, it's a function pointer type. But I wonder if it would be > good to dispense with absolute accuracy and instead say 'This function > returns... (more like the original but in a cleaner, more complete sentence). > I think it maps to how programmers think of it. And all the other function > definitions here should be that way too. This struct is roughly like defining a > class in C++ with a set of methods; in this case, we should probably speak to > developers' mental models and dispense with overly verbose correctness. http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb_instance.h#newcode35 ppapi/c/ppb_instance.h:35: * @param[in] instance A PP_Instance indentifying one instance of a module. Is this not implied? That is, wouldn't developers know to provide the instance whose WIndowObject should be retrieved? On 2011/02/08 21:51:17, dmichael wrote: > 'A PP_Instance identifying one instance of a module.' -> 'The PP_Instance whose > WindowObject should be retrieved.'? Point being it's not just any PP_Instance. http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppp_instance.h File ppapi/c/ppp_instance.h (right): http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppp_instance.h#newcode32 ppapi/c/ppp_instance.h:32: * unless you want your model to handel events such as a change of focus or On 2011/02/08 21:51:17, dmichael wrote: > s/model/module > s/handel/handle > I also don't think the last sentence is true. The methods need to have bodies, > but the bodies can be trivial (0 lines or 1 line to return a default return). Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppp_instance.h#newcode56 ppapi/c/ppp_instance.h:56: * embed id="nacl_module" dimensions="2"> On 2011/02/08 21:51:17, dmichael wrote: > s/embed/<embed Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppp_instance.h#newcode56 ppapi/c/ppp_instance.h:56: * embed id="nacl_module" dimensions="2"> On 2011/02/08 21:51:17, dmichael wrote: > s/embed/<embed Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppp_instance.h#newcode58 ppapi/c/ppp_instance.h:58: * @param[in] argv An array of argument values. These are the values of the On 2011/02/08 21:51:17, dmichael wrote: > "These are the values of the arguments listed in the <embed> tag." > Redundant... delete? Done. http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppp_instance.h#newcode59 ppapi/c/ppp_instance.h:59: * arguments listed in the <embed> tag. In the previous example, there will On 2011/02/08 21:51:17, dmichael wrote: > 'the previous'->'this' Done.
Ack, sorry... I was sitting on these comments and had forgotten to do Publish+Mail. Mostly nits. http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h File ppapi/c/pp_input_event.h (right): http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcode26 ppapi/c/pp_input_event.h:26: * This enumeration contains enumorators representing each mouse button. On 2011/02/09 16:42:04, jond wrote: > You're rewrite seemed too verbose. From what I can tell enumeration containing > enumerators is correct. Sounds reduant, but I don't think it is, but it does > sound a bit awkward. I'll use the word constants if you like that better. > > On 2011/02/08 17:04:38, dmichael wrote: > > Sounds redundant... maybe 'PP_InputEvent_MouseButton is an enumerated type > > representing the mouse button which is associated with a mouse event.' (Or > > something > Thanks, Jon. I like your new version. http://codereview.chromium.org/6246117/diff/1/ppapi/c/pp_input_event.h#newcod... ppapi/c/pp_input_event.h:232: /** This value represents */ On 2011/02/08 17:04:38, dmichael wrote: > represents what? > (I'm not totally clear on that myself :o)) You missed these 'This value represents' things. I think for this version, maybe just take them out if we don't know what they are? Or check with someone who knows? I have a test that would probably help me understand it enough (event_example.html in the NaCl tree) to help write an explanation, but I don't have time to run it and play with it right now. http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb_instance.h File ppapi/c/ppb_instance.h (right): http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb_instance.h#newcode35 ppapi/c/ppb_instance.h:35: * @param[in] instance A PP_Instance indentifying one instance of a module. On 2011/02/09 16:42:04, jond wrote: > Is this not implied? That is, wouldn't developers know to provide the instance > whose WIndowObject should be retrieved? > > On 2011/02/08 21:51:17, dmichael wrote: > > 'A PP_Instance identifying one instance of a module.' -> 'The PP_Instance > whose > > WindowObject should be retrieved.'? Point being it's not just any > PP_Instance. > True, but I also think 'identifying on instance of a module' isn't helpful here. Usually parameters should be documented without a great deal about what the type is (that should be in the type's documentation), but how that parameter is used in the method. In this case, I hope everything is pretty obvious, so there's almost nothing to say. But if we say anything, I think it should be about how the method uses the parameter. Does that make sense? http://codereview.chromium.org/6246117/diff/11001/ppapi/c/ppb.h File ppapi/c/ppb.h (right): http://codereview.chromium.org/6246117/diff/11001/ppapi/c/ppb.h#newcode20 ppapi/c/ppb.h:20: * function. A generic PPB_GetInterface pointer is passed to nit: 2 spaces after a period. http://codereview.chromium.org/6246117/diff/11001/ppapi/c/ppb.h#newcode21 ppapi/c/ppb.h:21: * PPP_InitializedModule when your module is loaded. You can use this pointer nit: 2 spaces after a period. http://codereview.chromium.org/6246117/diff/11001/ppapi/c/ppb.h#newcode22 ppapi/c/ppb.h:22: * to request a pointer to a specific browser interface. Browser interface nit: 2 spaces after period http://codereview.chromium.org/6246117/diff/11001/ppapi/c/ppb.h#newcode24 ppapi/c/ppb.h:24: * interface, such as PP_AUDIO_INTERFACE found in ppb.audio.h or ppb.audio.h -> ppb_audio.h http://codereview.chromium.org/6246117/diff/11001/ppapi/c/ppp_instance.h File ppapi/c/ppp_instance.h (right): http://codereview.chromium.org/6246117/diff/11001/ppapi/c/ppp_instance.h#newc... ppapi/c/ppp_instance.h:32: * or 1 line to return the default return value) unless you want your module I didn't really expect that 0 lines or 1 line wording to show up here... but I think it's fine if you think it's helpful. I think you could take out the whole part in parentheses if you want to. http://codereview.chromium.org/6246117/diff/11001/ppapi/c/ppp_instance.h#newc... ppapi/c/ppp_instance.h:60: * arguments listed in the <embed> tag, for example nit: period to end the last sentence.
http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb_instance.h File ppapi/c/ppb_instance.h (right): http://codereview.chromium.org/6246117/diff/1/ppapi/c/ppb_instance.h#newcode35 ppapi/c/ppb_instance.h:35: * @param[in] instance A PP_Instance indentifying one instance of a module. On 2011/02/11 16:42:04, dmichael wrote: > On 2011/02/09 16:42:04, jond wrote: > > Is this not implied? That is, wouldn't developers know to provide the instance > > whose WIndowObject should be retrieved? > > > > On 2011/02/08 21:51:17, dmichael wrote: > > > 'A PP_Instance identifying one instance of a module.' -> 'The PP_Instance > > whose > > > WindowObject should be retrieved.'? Point being it's not just any > > PP_Instance. > > > > True, but I also think 'identifying on instance of a module' isn't helpful here. > Usually parameters should be documented without a great deal about what the > type is (that should be in the type's documentation), but how that parameter is > used in the method. In this case, I hope everything is pretty obvious, so > there's almost nothing to say. But if we say anything, I think it should be > about how the method uses the parameter. Does that make sense? Done. http://codereview.chromium.org/6246117/diff/11001/ppapi/c/ppp_instance.h File ppapi/c/ppp_instance.h (right): http://codereview.chromium.org/6246117/diff/11001/ppapi/c/ppp_instance.h#newc... ppapi/c/ppp_instance.h:32: * or 1 line to return the default return value) unless you want your module On 2011/02/11 16:42:04, dmichael wrote: > I didn't really expect that 0 lines or 1 line wording to show up here... but I > think it's fine if you think it's helpful. I think you could take out the whole > part in parentheses if you want to. Done. http://codereview.chromium.org/6246117/diff/11001/ppapi/c/ppp_instance.h#newc... ppapi/c/ppp_instance.h:60: * arguments listed in the <embed> tag, for example On 2011/02/11 16:42:04, dmichael wrote: > nit: period to end the last sentence. Not sure where this is needed. The sentence goes beyond "for example..."
http://codereview.chromium.org/6246117/diff/11001/ppapi/c/ppp_instance.h File ppapi/c/ppp_instance.h (right): http://codereview.chromium.org/6246117/diff/11001/ppapi/c/ppp_instance.h#newc... ppapi/c/ppp_instance.h:32: * or 1 line to return the default return value) unless you want your module On 2011/02/11 16:42:04, dmichael wrote: > I didn't really expect that 0 lines or 1 line wording to show up here... but I > think it's fine if you think it's helpful. I think you could take out the whole > part in parentheses if you want to. I meant you could take out '(0 lines or 1 line to return the default return value)', though I'm okay with it if you don't. But that paren after mouse down below should probably go back where it was (after events). http://codereview.chromium.org/6246117/diff/11001/ppapi/c/ppp_instance.h#newc... ppapi/c/ppp_instance.h:60: * arguments listed in the <embed> tag, for example On 2011/02/15 17:02:47, jond wrote: > On 2011/02/11 16:42:04, dmichael wrote: > > nit: period to end the last sentence. > > Not sure where this is needed. The sentence goes beyond "for example..." > You're right, my mistake.
LGTM |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
