Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7565017:
New instance.h documentation (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 instance.h documentation
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96082
Patch Set 1 #
Total comments: 39
Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Messages
Total messages: 3 (0 generated)
http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h File ppapi/cpp/instance.h (left): http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#oldcode393 ppapi/cpp/instance.h:393: /// @see HandleMessage() for receiving events from JavaScript. What was wrong with this? I thought it was useful. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h File ppapi/cpp/instance.h (right): http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode9 ppapi/cpp/instance.h:9: /// Defines the C++ wrapper for a module instance. can we just say 'an instance' instead of 'a module instance'? or just Defines the Instance class? The Instance class itself should have class documentation. Maybe you should see what you can steal from PPB_Instance, PP_Instance, PPP_Instance, or the SDK examples. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode73 ppapi/cpp/instance.h:73: /// function will be called immediately after the instance object is created. created->constructed I know it's not as nice sounding, but it has specific meaning in this case. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode96 ppapi/cpp/instance.h:96: /// DidChangeView() is called when the position, the size, of the clip 'of'->'or' http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode114 ppapi/cpp/instance.h:114: /// the top left of the module's coordinate system (not the page). If the module->instance http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode115 ppapi/cpp/instance.h:115: /// module is invisible, <code>clip</code> will be (0, 0, 0, 0). module->instance http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode188 ppapi/cpp/instance.h:188: /// HandleDocumentLoad() is called after initialize for a full-frame initialize->Init() ? http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode189 ppapi/cpp/instance.h:189: /// module that was instantiated based on the MIME type of a DOMWindow module->instance http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode292 ppapi/cpp/instance.h:292: /// RequestFilteringInputEvents() for this class of input. Oops, this part is no longer true. Could you remove this paragraph while you're here? http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode310 ppapi/cpp/instance.h:310: /// delivered and not bubbled to the page. This means that even if you could you leave some kind of emphasis on 'not'? If you don't like <i>, maybe something else? It's very important that the readers' eyes don't overlook that 'not'. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode312 ppapi/cpp/instance.h:312: /// a the message. remove 'a' (Didn't like 'a crack at'? :-p) http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode314 ppapi/cpp/instance.h:314: /// <strong>Example:</code> I think you meant </strong> http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode316 ppapi/cpp/instance.h:316: /// @code I would rather use <code> everywhere instead of @code http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode321 ppapi/cpp/instance.h:321: /// @endcode </code> http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode334 ppapi/cpp/instance.h:334: /// events, these must use RequestFilteringInputEvents(). The last sentence is no longer true; please delete it. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode356 ppapi/cpp/instance.h:356: /// @code <code> http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode362 ppapi/cpp/instance.h:362: /// @endcode </code>
http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h File ppapi/cpp/instance.h (left): http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#oldcode393 ppapi/cpp/instance.h:393: /// @see HandleMessage() for receiving events from JavaScript. I'll put it back for now. But, I believe that anytime Doxygen finds a method name, it creates a link. And, in other places I've been using "Refer to HandleMessage for receiving events from JavaScript." But, I need to standardize. I'll put it on the list. On 2011/08/04 17:00:14, dmichael wrote: > What was wrong with this? I thought it was useful. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h File ppapi/cpp/instance.h (right): http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode9 ppapi/cpp/instance.h:9: /// Defines the C++ wrapper for a module instance. On 2011/08/04 17:00:14, dmichael wrote: > can we just say 'an instance' instead of 'a module instance'? > or just > Defines the Instance class? > > The Instance class itself should have class documentation. Maybe you should see > what you can steal from PPB_Instance, PP_Instance, PPP_Instance, or the SDK > examples. Done. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode73 ppapi/cpp/instance.h:73: /// function will be called immediately after the instance object is created. On 2011/08/04 17:00:14, dmichael wrote: > created->constructed > I know it's not as nice sounding, but it has specific meaning in this case. Done. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode96 ppapi/cpp/instance.h:96: /// DidChangeView() is called when the position, the size, of the clip On 2011/08/04 17:00:14, dmichael wrote: > 'of'->'or' Done. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode114 ppapi/cpp/instance.h:114: /// the top left of the module's coordinate system (not the page). If the On 2011/08/04 17:00:14, dmichael wrote: > module->instance Done. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode114 ppapi/cpp/instance.h:114: /// the top left of the module's coordinate system (not the page). If the On 2011/08/04 17:00:14, dmichael wrote: > module->instance Done. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode115 ppapi/cpp/instance.h:115: /// module is invisible, <code>clip</code> will be (0, 0, 0, 0). On 2011/08/04 17:00:14, dmichael wrote: > module->instance Done. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode188 ppapi/cpp/instance.h:188: /// HandleDocumentLoad() is called after initialize for a full-frame On 2011/08/04 17:00:14, dmichael wrote: > initialize->Init() ? Done. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode188 ppapi/cpp/instance.h:188: /// HandleDocumentLoad() is called after initialize for a full-frame On 2011/08/04 17:00:14, dmichael wrote: > initialize->Init() ? Done. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode189 ppapi/cpp/instance.h:189: /// module that was instantiated based on the MIME type of a DOMWindow On 2011/08/04 17:00:14, dmichael wrote: > module->instance Done. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode292 ppapi/cpp/instance.h:292: /// RequestFilteringInputEvents() for this class of input. On 2011/08/04 17:00:14, dmichael wrote: > Oops, this part is no longer true. Could you remove this paragraph while you're > here? Done. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode310 ppapi/cpp/instance.h:310: /// delivered and not bubbled to the page. This means that even if you On 2011/08/04 17:00:14, dmichael wrote: > could you leave some kind of emphasis on 'not'? If you don't like <i>, maybe > something else? It's very important that the readers' eyes don't overlook that > 'not'. Done. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode310 ppapi/cpp/instance.h:310: /// delivered and not bubbled to the page. This means that even if you On 2011/08/04 17:00:14, dmichael wrote: > could you leave some kind of emphasis on 'not'? If you don't like <i>, maybe > something else? It's very important that the readers' eyes don't overlook that > 'not'. Done. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode312 ppapi/cpp/instance.h:312: /// a the message. On 2011/08/04 17:00:14, dmichael wrote: > remove 'a' > (Didn't like 'a crack at'? :-p) Done. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode312 ppapi/cpp/instance.h:312: /// a the message. On 2011/08/04 17:00:14, dmichael wrote: > remove 'a' > (Didn't like 'a crack at'? :-p) Done. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode314 ppapi/cpp/instance.h:314: /// <strong>Example:</code> On 2011/08/04 17:00:14, dmichael wrote: > I think you meant </strong> Done. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode316 ppapi/cpp/instance.h:316: /// @code Let me look into that. Its @code in many other places, so I'll put it on a global list. On 2011/08/04 17:00:14, dmichael wrote: > I would rather use <code> everywhere instead of @code http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode321 ppapi/cpp/instance.h:321: /// @endcode I'll take "a crack at this" later. ;) On 2011/08/04 17:00:14, dmichael wrote: > </code> http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode334 ppapi/cpp/instance.h:334: /// events, these must use RequestFilteringInputEvents(). On 2011/08/04 17:00:14, dmichael wrote: > The last sentence is no longer true; please delete it. Done. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode334 ppapi/cpp/instance.h:334: /// events, these must use RequestFilteringInputEvents(). On 2011/08/04 17:00:14, dmichael wrote: > The last sentence is no longer true; please delete it. Done. http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode356 ppapi/cpp/instance.h:356: /// @code Later man. On 2011/08/04 17:00:14, dmichael wrote: > <code> http://codereview.chromium.org/7565017/diff/1/ppapi/cpp/instance.h#newcode362 ppapi/cpp/instance.h:362: /// @endcode Later man. On 2011/08/04 17:00:14, dmichael wrote: > </code>
LGTM |
