Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7553026:
New C++ Docs. (Closed)
|
Created:
9 years, 4 months ago by jond Modified:
9 years, 4 months ago Reviewers:
dmichael (off chromium) CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionNew C++ Docs.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96399
Patch Set 1 #
Total comments: 32
Patch Set 2 : '' #
Total comments: 11
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #
Messages
Total messages: 8 (0 generated)
http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/module.h File ppapi/cpp/module.h (right): http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/module.h#newcode20 ppapi/cpp/module.h:20: /// This file defines a Module object which uniquely identifies the module Maybe keep it simple: 'This file defines the Module class.' The Module should have some class documentation, maybe something like this (stolen from sdk hello_world.cc): The Module class. The browser calls the CreateInstance() method to create an instance of your module on the web page. The browser creates a new instance for each <embed> tag with <code>type="application/x-nacl"</code>. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/module_embedder.h File ppapi/cpp/module_embedder.h (right): http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/module_embedder.h#new... ppapi/cpp/module_embedder.h:12: /// A Module object associated with this module. Just remove this comment. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/module_embedder.h#new... ppapi/cpp/module_embedder.h:13: class Module; Please add a carriage return here. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/module_embedder.h#new... ppapi/cpp/module_embedder.h:21: // embedder" at the top. not sure what that means. It means that the developer of the plugin/NaCl module needs to implement this method. See the last bit of hello_world.cc in the SDK for an example. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_aggregator.h File ppapi/cpp/paint_aggregator.h (right): http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_aggregator.h#ne... ppapi/cpp/paint_aggregator.h:107: /// This function repaints the provided rect. I don't think the repaint happens right away. It's probably more correct to say something like: "Invalidate the rect so that it will be repainted." http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_aggregator.h#ne... ppapi/cpp/paint_aggregator.h:112: /// This function scrolls the provided rect by the provided amount. Ditto here, reword because the scroll doesn't happen here. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_manager.h File ppapi/cpp/paint_manager.h (right): http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_manager.h#newco... ppapi/cpp/paint_manager.h:35: /// @code Why are you using @code here instead of <code> like everywhere else? http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_manager.h#newco... ppapi/cpp/paint_manager.h:73: /// OnPaint() paints the given invalid area of the instance to the given Do we wrap function names with <code> tags? If so, you should do OnPaint() here and Flush() below. Do we have a place where we list our documentation conventions? Maybe we should... http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_manager.h#newco... ppapi/cpp/paint_manager.h:143: /// Initialize() is called before using if you use the 0-arg constructor. Reword as a 'You must' kind of thing; the developer must call this if they use the 0-arg constructor. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_manager.h#newco... ppapi/cpp/paint_manager.h:193: /// This function intended to be called from <code>ViewChanged</code> with you lost the 'is' before intentended http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_manager.h#newco... ppapi/cpp/paint_manager.h:226: /// ScrollRect() scrolls the provided <code>Rect</code> by the provided 'by the provided' -> 'by the given'? It might read better to not use 'provided' twice.
http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/module.h File ppapi/cpp/module.h (right): http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/module.h#newcode20 ppapi/cpp/module.h:20: /// This file defines a Module object which uniquely identifies the module On 2011/08/03 15:50:49, dmichael wrote: > Maybe keep it simple: > 'This file defines the Module class.' > > The Module should have some class documentation, maybe something like this > (stolen from sdk hello_world.cc): > The Module class. The browser calls the CreateInstance() method to create > an instance of your module on the web page. The browser creates a new > instance for each <embed> tag with > <code>type="application/x-nacl"</code>. Done. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/module_embedder.h File ppapi/cpp/module_embedder.h (right): http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/module_embedder.h#new... ppapi/cpp/module_embedder.h:13: class Module; On 2011/08/03 15:50:49, dmichael wrote: > Please add a carriage return here. Done. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/module_embedder.h#new... ppapi/cpp/module_embedder.h:21: // embedder" at the top. not sure what that means. So, do I need this note or will people learn this from hello_world.cc? I put a new note in. On 2011/08/03 15:50:49, dmichael wrote: > It means that the developer of the plugin/NaCl module needs to implement this > method. > > See the last bit of hello_world.cc in the SDK for an example. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_aggregator.h File ppapi/cpp/paint_aggregator.h (right): http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_aggregator.h#ne... ppapi/cpp/paint_aggregator.h:107: /// This function repaints the provided rect. On 2011/08/03 15:50:49, dmichael wrote: > I don't think the repaint happens right away. It's probably more correct to say > something like: > "Invalidate the rect so that it will be repainted." Done. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_aggregator.h#ne... ppapi/cpp/paint_aggregator.h:112: /// This function scrolls the provided rect by the provided amount. On 2011/08/03 15:50:49, dmichael wrote: > Ditto here, reword because the scroll doesn't happen here. Done. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_aggregator.h#ne... ppapi/cpp/paint_aggregator.h:112: /// This function scrolls the provided rect by the provided amount. On 2011/08/03 15:50:49, dmichael wrote: > Ditto here, reword because the scroll doesn't happen here. Done. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_aggregator.h#ne... ppapi/cpp/paint_aggregator.h:112: /// This function scrolls the provided rect by the provided amount. On 2011/08/03 15:50:49, dmichael wrote: > Ditto here, reword because the scroll doesn't happen here. Done. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_aggregator.h#ne... ppapi/cpp/paint_aggregator.h:112: /// This function scrolls the provided rect by the provided amount. On 2011/08/03 15:50:49, dmichael wrote: > Ditto here, reword because the scroll doesn't happen here. Done. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_aggregator.h#ne... ppapi/cpp/paint_aggregator.h:112: /// This function scrolls the provided rect by the provided amount. On 2011/08/03 15:50:49, dmichael wrote: > Ditto here, reword because the scroll doesn't happen here. Done. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_manager.h File ppapi/cpp/paint_manager.h (right): http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_manager.h#newco... ppapi/cpp/paint_manager.h:35: /// @code We're using @code and @endcode for large code blocks. No real reason other than a few things used it and I stayed with it. On 2011/08/03 15:50:49, dmichael wrote: > Why are you using @code here instead of <code> like everywhere else? http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_manager.h#newco... ppapi/cpp/paint_manager.h:73: /// OnPaint() paints the given invalid area of the instance to the given <code></code> is only used for things that exist in code exactly as they are in the docs. You never see Flush() in code, for example, so its not surrounded by code tags. I need to update the conventions a bit, but we have them. 2011/08/03 15:50:49, dmichael wrote: > Do we wrap function names with <code> tags? If so, you should do OnPaint() here > and Flush() below. > Do we have a place where we list our documentation conventions? Maybe we > should... http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_manager.h#newco... ppapi/cpp/paint_manager.h:143: /// Initialize() is called before using if you use the 0-arg constructor. On 2011/08/03 15:50:49, dmichael wrote: > Reword as a 'You must' kind of thing; the developer must call this if they use > the 0-arg constructor. Done. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_manager.h#newco... ppapi/cpp/paint_manager.h:143: /// Initialize() is called before using if you use the 0-arg constructor. On 2011/08/03 15:50:49, dmichael wrote: > Reword as a 'You must' kind of thing; the developer must call this if they use > the 0-arg constructor. Done. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_manager.h#newco... ppapi/cpp/paint_manager.h:226: /// ScrollRect() scrolls the provided <code>Rect</code> by the provided On 2011/08/03 15:50:49, dmichael wrote: > 'by the provided' -> 'by the given'? > It might read better to not use 'provided' twice. Done. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_manager.h#newco... ppapi/cpp/paint_manager.h:226: /// ScrollRect() scrolls the provided <code>Rect</code> by the provided On 2011/08/03 15:50:49, dmichael wrote: > 'by the provided' -> 'by the given'? > It might read better to not use 'provided' twice. Done.
http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/module_embedder.h File ppapi/cpp/module_embedder.h (right): http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/module_embedder.h#new... ppapi/cpp/module_embedder.h:21: // embedder" at the top. not sure what that means. On 2011/08/03 20:15:08, jond wrote: > So, do I need this note or will people learn this from hello_world.cc? I put a > new note in. > > On 2011/08/03 15:50:49, dmichael wrote: > > It means that the developer of the plugin/NaCl module needs to implement this > > method. > > > > See the last bit of hello_world.cc in the SDK for an example. > Yes, the API needs to be documented here. The examples give sort of a tour through the API, but the ppapi code has to serve as the reference. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_manager.h File ppapi/cpp/paint_manager.h (right): http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/paint_manager.h#newco... ppapi/cpp/paint_manager.h:35: /// @code On 2011/08/03 20:15:08, jond wrote: > We're using @code and @endcode for large code blocks. No real reason other than > a few things used it and I stayed with it. > > On 2011/08/03 15:50:49, dmichael wrote: > > Why are you using @code here instead of <code> like everywhere else? > Is the @code tag very widespread? There doesn't seem to be a reason to use a different tag; I find <code> more readable anyway. Can we just use <code> and </code> so we're more consistent? http://codereview.chromium.org/7553026/diff/4001/ppapi/cpp/paint_aggregator.h File ppapi/cpp/paint_aggregator.h (right): http://codereview.chromium.org/7553026/diff/4001/ppapi/cpp/paint_aggregator.h... ppapi/cpp/paint_aggregator.h:112: /// This function invalidate the rect so it can be scrolled. Maybe instead: This function adds a pending scroll update. ...or something http://codereview.chromium.org/7553026/diff/4001/ppapi/cpp/paint_manager.h File ppapi/cpp/paint_manager.h (right): http://codereview.chromium.org/7553026/diff/4001/ppapi/cpp/paint_manager.h#ne... ppapi/cpp/paint_manager.h:193: /// This function intended to be called from <code>ViewChanged</code> with Still missing an 'is' here http://codereview.chromium.org/7553026/diff/4001/ppapi/cpp/paint_manager.h#ne... ppapi/cpp/paint_manager.h:226: /// ScrollRect() scrolls the provided <code>clip_rect</code> by the provided It's not a huge deal, but it still says provided twice here.
http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/module_embedder.h File ppapi/cpp/module_embedder.h (right): http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/module_embedder.h#new... ppapi/cpp/module_embedder.h:12: /// A Module object associated with this module. On 2011/08/03 15:50:49, dmichael wrote: > Just remove this comment. Done. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/module_embedder.h#new... ppapi/cpp/module_embedder.h:13: class Module; On 2011/08/03 15:50:49, dmichael wrote: > Please add a carriage return here. Done. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/module_embedder.h#new... ppapi/cpp/module_embedder.h:21: // embedder" at the top. not sure what that means. On 2011/08/03 15:50:49, dmichael wrote: > It means that the developer of the plugin/NaCl module needs to implement this > method. > > See the last bit of hello_world.cc in the SDK for an example. Done. http://codereview.chromium.org/7553026/diff/1/ppapi/cpp/module_embedder.h#new... ppapi/cpp/module_embedder.h:21: // embedder" at the top. not sure what that means. On 2011/08/04 16:33:38, dmichael wrote: > On 2011/08/03 20:15:08, jond wrote: > > So, do I need this note or will people learn this from hello_world.cc? I put a > > new note in. > > > > On 2011/08/03 15:50:49, dmichael wrote: > > > It means that the developer of the plugin/NaCl module needs to implement > this > > > method. > > > > > > See the last bit of hello_world.cc in the SDK for an example. > > > > Yes, the API needs to be documented here. The examples give sort of a tour > through the API, but the ppapi code has to serve as the reference. Done.
It looks like maybe you didn't see the last set of comments I made? http://codereview.chromium.org/7553026/diff/4001/ppapi/cpp/paint_aggregator.h File ppapi/cpp/paint_aggregator.h (right): http://codereview.chromium.org/7553026/diff/4001/ppapi/cpp/paint_aggregator.h... ppapi/cpp/paint_aggregator.h:112: /// This function invalidate the rect so it can be scrolled. On 2011/08/04 16:33:38, dmichael wrote: > Maybe instead: > This function adds a pending scroll update. > ...or something Can you fix this comment? At the very leasy, invalidate should be changed to invalidates. Even better would be rewording it to give the sense that calling this function adds a pending update which will cause a scroll. The scroll doesn't happen immediately, as I understand it. http://codereview.chromium.org/7553026/diff/4001/ppapi/cpp/paint_manager.h File ppapi/cpp/paint_manager.h (right): http://codereview.chromium.org/7553026/diff/4001/ppapi/cpp/paint_manager.h#ne... ppapi/cpp/paint_manager.h:193: /// This function intended to be called from <code>ViewChanged</code> with On 2011/08/04 16:33:38, dmichael wrote: > Still missing an 'is' here Still still missing an 'is' here. http://codereview.chromium.org/7553026/diff/4001/ppapi/cpp/paint_manager.h#ne... ppapi/cpp/paint_manager.h:226: /// ScrollRect() scrolls the provided <code>clip_rect</code> by the provided On 2011/08/04 16:33:38, dmichael wrote: > It's not a huge deal, but it still says provided twice here. Still says provided twice...
http://codereview.chromium.org/7553026/diff/4001/ppapi/cpp/paint_aggregator.h File ppapi/cpp/paint_aggregator.h (right): http://codereview.chromium.org/7553026/diff/4001/ppapi/cpp/paint_aggregator.h... ppapi/cpp/paint_aggregator.h:112: /// This function invalidate the rect so it can be scrolled. On 2011/08/04 16:33:38, dmichael wrote: > Maybe instead: > This function adds a pending scroll update. > ...or something Done. http://codereview.chromium.org/7553026/diff/4001/ppapi/cpp/paint_aggregator.h... ppapi/cpp/paint_aggregator.h:112: /// This function invalidate the rect so it can be scrolled. On 2011/08/04 16:33:38, dmichael wrote: > Maybe instead: > This function adds a pending scroll update. > ...or something Done. http://codereview.chromium.org/7553026/diff/4001/ppapi/cpp/paint_manager.h File ppapi/cpp/paint_manager.h (right): http://codereview.chromium.org/7553026/diff/4001/ppapi/cpp/paint_manager.h#ne... ppapi/cpp/paint_manager.h:193: /// This function intended to be called from <code>ViewChanged</code> with On 2011/08/04 16:33:38, dmichael wrote: > Still missing an 'is' here Done. http://codereview.chromium.org/7553026/diff/4001/ppapi/cpp/paint_manager.h#ne... ppapi/cpp/paint_manager.h:226: /// ScrollRect() scrolls the provided <code>clip_rect</code> by the provided On 2011/08/04 16:33:38, dmichael wrote: > It's not a huge deal, but it still says provided twice here. Done. http://codereview.chromium.org/7553026/diff/4001/ppapi/cpp/paint_manager.h#ne... ppapi/cpp/paint_manager.h:226: /// ScrollRect() scrolls the provided <code>clip_rect</code> by the provided On 2011/08/04 16:33:38, dmichael wrote: > It's not a huge deal, but it still says provided twice here. Done.
LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
