Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9815025:
PPAPI: Clean up documentation for InstanceHandle and PASS_REF (Closed)
|
Created:
8 years, 9 months ago by jond Modified:
8 years, 8 months ago Reviewers:
dmichael (off chromium) CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionPPAPI: Clean up documentation for InstanceHandle and PASS_REF
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=131594
Patch Set 1 #
Total comments: 10
Patch Set 2 : #
Messages
Total messages: 15 (0 generated)
http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/instance_handle.h File ppapi/cpp/instance_handle.h (right): http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/instance_handle.h#new... ppapi/cpp/instance_handle.h:22: /// thread of the module. This means that it may get destroyed at any time how about if we say "main pepper thread." http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/instance_handle.h#new... ppapi/cpp/instance_handle.h:23: /// based on something that happens on the web page. So, it's never good to So->Therefore ? "never good" doesn't give the right sense here. Maybe "it is not safe to" http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/instance_handle.h#new... ppapi/cpp/instance_handle.h:30: /// 2. <code>PP_Instance</code> is a good identifier to use for this case. is->would be ? (If it was a good identifier for this case, we would use it.) The sense is more <code>PP_Instance</code> is another way to refer to a unique instance, but we can not use it because it is just a typedef for an integer... reword as you see fit. http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/instance_handle.h#new... ppapi/cpp/instance_handle.h:64: /// The pp_instance() function returns the internal instance handle. "instance handle" -> "PP_Instance" (not wrong, but confusing since we're in InstanceHandle, but that's not what you're returning.) http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/pass_ref.h File ppapi/cpp/pass_ref.h (right): http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/pass_ref.h#newcode9 ppapi/cpp/pass_ref.h:9: /// This file defines an annotation for ocnstructors and other functions that ocnstructors->constructors
I just noted this in another CL... I've been lax. Please provide a better subject and description for your CL. On Fri, Mar 23, 2012 at 3:05 PM, <dmichael@chromium.org> wrote: > > http://codereview.chromium.**org/9815025/diff/1/ppapi/cpp/** > instance_handle.h<http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/instance_handle.h> > File ppapi/cpp/instance_handle.h (right): > > http://codereview.chromium.**org/9815025/diff/1/ppapi/cpp/** > instance_handle.h#newcode22<http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/instance_handle.h#newcode22> > ppapi/cpp/instance_handle.h:**22: /// thread of the module. This means > > that it may get destroyed at any time > how about if we say "main pepper thread." > > http://codereview.chromium.**org/9815025/diff/1/ppapi/cpp/** > instance_handle.h#newcode23<http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/instance_handle.h#newcode23> > ppapi/cpp/instance_handle.h:**23: /// based on something that happens on > > the web page. So, it's never good to > So->Therefore > ? > "never good" doesn't give the right sense here. Maybe "it is not safe > to" > > http://codereview.chromium.**org/9815025/diff/1/ppapi/cpp/** > instance_handle.h#newcode30<http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/instance_handle.h#newcode30> > ppapi/cpp/instance_handle.h:**30: /// 2. <code>PP_Instance</code> is a > > good identifier to use for this case. > is->would be ? > > (If it was a good identifier for this case, we would use it.) > > The sense is more <code>PP_Instance</code> is another way to refer to a > unique instance, but we can not use it because it is just a typedef for > an integer... > > reword as you see fit. > > http://codereview.chromium.**org/9815025/diff/1/ppapi/cpp/** > instance_handle.h#newcode64<http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/instance_handle.h#newcode64> > ppapi/cpp/instance_handle.h:**64: /// The pp_instance() function returns > the internal instance handle. > "instance handle" -> "PP_Instance" > (not wrong, but confusing since we're in InstanceHandle, but that's not > what you're returning.) > > http://codereview.chromium.**org/9815025/diff/1/ppapi/cpp/**pass_ref.h<http:/... > File ppapi/cpp/pass_ref.h (right): > > http://codereview.chromium.**org/9815025/diff/1/ppapi/cpp/** > pass_ref.h#newcode9<http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/pass_ref.h#newcode9> > ppapi/cpp/pass_ref.h:9: /// This file defines an annotation for > > ocnstructors and other functions that > ocnstructors->constructors > > http://codereview.chromium.**org/9815025/<http://codereview.chromium.org/9815... >
http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/instance_handle.h File ppapi/cpp/instance_handle.h (right): http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/instance_handle.h#new... ppapi/cpp/instance_handle.h:22: /// thread of the module. This means that it may get destroyed at any time On 2012/03/23 21:05:00, dmichael wrote: > how about if we say "main pepper thread." Done. http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/instance_handle.h#new... ppapi/cpp/instance_handle.h:23: /// based on something that happens on the web page. So, it's never good to Agreed. I just didn't want to change the technical meaning without someone more knowledgable looking at it. On 2012/03/23 21:05:00, dmichael wrote: > So->Therefore > ? > "never good" doesn't give the right sense here. Maybe "it is not safe to" http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/instance_handle.h#new... ppapi/cpp/instance_handle.h:30: /// 2. <code>PP_Instance</code> is a good identifier to use for this case. Changing to "would be" seems okay to me. I made another small modification. On 2012/03/23 21:05:00, dmichael wrote: > is->would be ? > > (If it was a good identifier for this case, we would use it.) > > The sense is more <code>PP_Instance</code> is another way to refer to a unique > instance, but we can not use it because it is just a typedef for an integer... > > reword as you see fit. http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/instance_handle.h#new... ppapi/cpp/instance_handle.h:64: /// The pp_instance() function returns the internal instance handle. On 2012/03/23 21:05:00, dmichael wrote: > "instance handle" -> "PP_Instance" > (not wrong, but confusing since we're in InstanceHandle, but that's not what > you're returning.) Done. http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/pass_ref.h File ppapi/cpp/pass_ref.h (right): http://codereview.chromium.org/9815025/diff/1/ppapi/cpp/pass_ref.h#newcode9 ppapi/cpp/pass_ref.h:9: /// This file defines an annotation for ocnstructors and other functions that On 2012/03/23 21:05:00, dmichael wrote: > ocnstructors->constructors Done.
lgtm
Oops... again, please update the subject and description. It's very vague right now. Cleaned up documentation for what subsystem (i.e., PPAPI)? For what source files?
On Mon, Mar 26, 2012 at 1:37 PM, <jond@google.com> wrote: > On 2012/03/26 18:02:53, dmichael wrote: > >> Oops... again, please update the subject and description. It's very vague >> > right > >> now. Cleaned up documentation for what subsystem (i.e., PPAPI)? For what >> > source > >> files? >> > > I guess I was assuming that the source files, being in the CL, are > evident. Are > they not? > Listing the names of the files is overkill, but you should say something about what part of ppapi it is. If somebody is looking through the SVN commit log, it's nice to have a summary. E.g., look here: http://src.chromium.org/viewvc/chrome/trunk/src/ppapi/api/ all you see are the commit comments (the summary of the CL) Similarly, at: http://chromium-build.appspot.com/p/chromium/console you get the full description, but not a list of files. When browsing source or looking at the status of the tree, "Cleaned up documentation." is not very helpful. I like to prefix ppapi CLs with "PPAPI:", to give a brief bit of context. In your case, you might say: PPAPI: Clean up documentation for InstanceHandle and PASS_REF Since this is a pretty short CL, that would probably suffice for the description as well. Though you should usually add: """ BUG= TEST= """ to the description (it's okay to leave them empty) > http://codereview.chromium.**org/9815025/<http://codereview.chromium.org/9815... >
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jond@google.com/9815025/6001
Try job failure for 9815025-6001 (retry) on win_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jond@google.com/9815025/6001
Try job failure for 9815025-6001 (retry) (retry) on win_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jond@google.com/9815025/6001
Change committed as 131594 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
