Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7621019:
Missed some items to document here. New 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. |
DescriptionMissed some items to document here. New documentation.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96987
Patch Set 1 #Patch Set 2 : '' #
Total comments: 27
Patch Set 3 : '' #
Total comments: 2
Patch Set 4 : '' #Patch Set 5 : '' #Messages
Total messages: 8 (0 generated)
http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h File ppapi/cpp/module.h (right): http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode49 ppapi/cpp/module.h:49: /// @return True if success, otherwise false. True->true if->on ? (or success->successful)? http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode52 ppapi/cpp/module.h:52: /// The <code>pp_module</code> function returns the internal module handle. Why are you using 'The <code>function_name</code> function' here, but 'function_name()' above? Please make them consistent. I think you've been using the other style elsewhere, so: /// pp_module() returns... http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode57 ppapi/cpp/module.h:57: /// The <code>get_browser_interface</code> the internal get_browser_interface /// get_browser_interface() returns the internal get_browser_interface pointer. ? http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode67 ppapi/cpp/module.h:67: /// The <code>core</code> function returns the core interface for doing basic core() returns... http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode83 ppapi/cpp/module.h:83: /// GetBrowserInterface() returns an interface in the browser. I think the original wording is confusing. Maybe: GetBrowserInterface() returns interfaces which the browser implements (i.e., PPB interfaces). Or similar. http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode112 ppapi/cpp/module.h:112: /// @param[in,out] vtable The vtable returned for <code>interface_name</code>. 'returned'->'to return' http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode116 ppapi/cpp/module.h:116: // InternalInit() sets the browser interface and calls the regular init init -> Init() http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode125 ppapi/cpp/module.h:125: /// @return True if successful, otherwise false. True->true http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode129 ppapi/cpp/module.h:129: /// The <code>current_instance</code> allows iteration over the current /// current_instances() allows...
http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h File ppapi/cpp/module.h (right): http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode49 ppapi/cpp/module.h:49: /// @return True if success, otherwise false. On 2011/08/11 22:20:50, dmichael wrote: > True->true > if->on ? (or success->successful)? Done. http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode49 ppapi/cpp/module.h:49: /// @return True if success, otherwise false. On 2011/08/11 22:20:50, dmichael wrote: > True->true > if->on ? (or success->successful)? Done. http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode52 ppapi/cpp/module.h:52: /// The <code>pp_module</code> function returns the internal module handle. The function name here is lowercase. So, I thought it looked bad to start a sentence pp_module()... That's why I say "The <code>pp_module</code> function... On 2011/08/11 22:20:50, dmichael wrote: > Why are you using 'The <code>function_name</code> function' here, but > 'function_name()' above? > > Please make them consistent. I think you've been using the other style > elsewhere, so: > /// pp_module() returns... http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode57 ppapi/cpp/module.h:57: /// The <code>get_browser_interface</code> the internal get_browser_interface On 2011/08/11 22:20:50, dmichael wrote: > /// get_browser_interface() returns the internal get_browser_interface pointer. > > ? Done. http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode67 ppapi/cpp/module.h:67: /// The <code>core</code> function returns the core interface for doing basic See above. I will talk to Josie about this, but for now I'm going to start sentences with capitals (even if its a function name). On 2011/08/11 22:20:50, dmichael wrote: > core() returns... http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode83 ppapi/cpp/module.h:83: /// GetBrowserInterface() returns an interface in the browser. On 2011/08/11 22:20:50, dmichael wrote: > I think the original wording is confusing. Maybe: > GetBrowserInterface() returns interfaces which the browser implements (i.e., PPB > interfaces). > > Or similar. Done. http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode112 ppapi/cpp/module.h:112: /// @param[in,out] vtable The vtable returned for <code>interface_name</code>. On 2011/08/11 22:20:50, dmichael wrote: > 'returned'->'to return' Done. http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode116 ppapi/cpp/module.h:116: // InternalInit() sets the browser interface and calls the regular init On 2011/08/11 22:20:50, dmichael wrote: > init -> Init() Done. http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode125 ppapi/cpp/module.h:125: /// @return True if successful, otherwise false. Again, I was hoping to start sentences with a capital. But, if you think its confusing, I'll revisit. On 2011/08/11 22:20:50, dmichael wrote: > True->true http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode129 ppapi/cpp/module.h:129: /// The <code>current_instance</code> allows iteration over the current On 2011/08/11 22:20:50, dmichael wrote: > /// current_instances() allows... Done. http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode129 ppapi/cpp/module.h:129: /// The <code>current_instance</code> allows iteration over the current On 2011/08/11 22:20:50, dmichael wrote: > /// current_instances() allows... Done.
http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h File ppapi/cpp/module.h (right): http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode52 ppapi/cpp/module.h:52: /// The <code>pp_module</code> function returns the internal module handle. On 2011/08/12 16:58:18, jond wrote: > The function name here is lowercase. So, I thought it looked bad to start a > sentence pp_module()... That's why I say "The <code>pp_module</code> function... Okay, that's fine. But it should still be pp_module() instead of <code>pp_module</code>, shouldn't it? Same elsewhere. > > On 2011/08/11 22:20:50, dmichael wrote: > > Why are you using 'The <code>function_name</code> function' here, but > > 'function_name()' above? > > > > Please make them consistent. I think you've been using the other style > > elsewhere, so: > > /// pp_module() returns... > http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode125 ppapi/cpp/module.h:125: /// @return True if successful, otherwise false. On 2011/08/12 16:58:18, jond wrote: > Again, I was hoping to start sentences with a capital. But, if you think its > confusing, I'll revisit. On 2011/08/11 22:20:50, dmichael wrote: > > True->true > When case is important, my opinion is it's much better to ignore the capitalization rule and get the code right. 'True' has no meaning in C or C++, but 'true' is a keyword. Since we have PP_TRUE in some places, and some developers might be new to C/C++, it could be confusing. That's my 2 cents. http://codereview.chromium.org/7621019/diff/7001/ppapi/cpp/module.h#newcode84 ppapi/cpp/module.h:84: /// 'such as' isn't right; all browser-implemented interfaces are PPB interfaces (hence 'i.e.').
http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h File ppapi/cpp/module.h (right): http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode52 ppapi/cpp/module.h:52: /// The <code>pp_module</code> function returns the internal module handle. Actually saying pp_module() function is wrong. That's the pp_module function function. And, to say The pp_module() is strange. So, that's why its pp_module function. On 2011/08/12 19:13:55, dmichael wrote: > On 2011/08/12 16:58:18, jond wrote: > > The function name here is lowercase. So, I thought it looked bad to start a > > sentence pp_module()... That's why I say "The <code>pp_module</code> > function... > Okay, that's fine. But it should still be pp_module() instead of > <code>pp_module</code>, shouldn't it? Same elsewhere. > > > > On 2011/08/11 22:20:50, dmichael wrote: > > > Why are you using 'The <code>function_name</code> function' here, but > > > 'function_name()' above? > > > > > > Please make them consistent. I think you've been using the other style > > > elsewhere, so: > > > /// pp_module() returns... > > > http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode125 ppapi/cpp/module.h:125: /// @return True if successful, otherwise false. Okay, I'll change that one. On 2011/08/12 19:13:55, dmichael wrote: > On 2011/08/12 16:58:18, jond wrote: > > Again, I was hoping to start sentences with a capital. But, if you think its > > confusing, I'll revisit. On 2011/08/11 22:20:50, dmichael wrote: > > > True->true > > > When case is important, my opinion is it's much better to ignore the > capitalization rule and get the code right. 'True' has no meaning in C or C++, > but 'true' is a keyword. Since we have PP_TRUE in some places, and some > developers might be new to C/C++, it could be confusing. That's my 2 cents. http://codereview.chromium.org/7621019/diff/7001/ppapi/cpp/module.h#newcode84 ppapi/cpp/module.h:84: /// On 2011/08/12 19:13:56, dmichael wrote: > 'such as' isn't right; all browser-implemented interfaces are PPB interfaces > (hence 'i.e.'). > Done.
http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h File ppapi/cpp/module.h (right): http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode52 ppapi/cpp/module.h:52: /// The <code>pp_module</code> function returns the internal module handle. On 2011/08/12 19:29:24, jond wrote: > Actually saying pp_module() function is wrong. That's the pp_module function > function. Why? I don't read it 'function function'. All I'm saying is it should be consistent. We should always write function names in 1 way, so that you can tell at a glance that it's a function name. I'd rather you either use 'The pp_module() function returns' or just write 'pp_module() returns' (even though it means starting a sentence with lower-case. This is code documentation; consistency and correctness should trump grammar in these cases, IMHO. > And, to say The pp_module() is strange. So, that's why its pp_module > function. > > On 2011/08/12 19:13:55, dmichael wrote: > > On 2011/08/12 16:58:18, jond wrote: > > > The function name here is lowercase. So, I thought it looked bad to start a > > > sentence pp_module()... That's why I say "The <code>pp_module</code> > > function... > > Okay, that's fine. But it should still be pp_module() instead of > > <code>pp_module</code>, shouldn't it? Same elsewhere. > > > > > > On 2011/08/11 22:20:50, dmichael wrote: > > > > Why are you using 'The <code>function_name</code> function' here, but > > > > 'function_name()' above? > > > > > > > > Please make them consistent. I think you've been using the other style > > > > elsewhere, so: > > > > /// pp_module() returns... > > > > > > http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode125 ppapi/cpp/module.h:125: /// @return True if successful, otherwise false. On 2011/08/12 19:29:24, jond wrote: > Okay, I'll change that one. > > On 2011/08/12 19:13:55, dmichael wrote: > > On 2011/08/12 16:58:18, jond wrote: > > > Again, I was hoping to start sentences with a capital. But, if you think its > > > confusing, I'll revisit. On 2011/08/11 22:20:50, dmichael wrote: > > > > True->true > > > > > When case is important, my opinion is it's much better to ignore the > > capitalization rule and get the code right. 'True' has no meaning in C or C++, > > but 'true' is a keyword. Since we have PP_TRUE in some places, and some > > developers might be new to C/C++, it could be confusing. That's my 2 cents. > Thanks!
http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h File ppapi/cpp/module.h (right): http://codereview.chromium.org/7621019/diff/3001/ppapi/cpp/module.h#newcode52 ppapi/cpp/module.h:52: /// The <code>pp_module</code> function returns the internal module handle. I agree. I'll have to change this in several spots. How about "The get_browser_interface() function" I'm not going to stress on this one. - Jon On 2011/08/12 19:42:29, dmichael wrote: > On 2011/08/12 19:29:24, jond wrote: > > Actually saying pp_module() function is wrong. That's the pp_module function > > function. > Why? I don't read it 'function function'. All I'm saying is it should be > consistent. We should always write function names in 1 way, so that you can tell > at a glance that it's a function name. I'd rather you either use 'The > pp_module() function returns' or just write 'pp_module() returns' (even though > it means starting a sentence with lower-case. This is code documentation; > consistency and correctness should trump grammar in these cases, IMHO. > > And, to say The pp_module() is strange. So, that's why its pp_module > > function. > > > > On 2011/08/12 19:13:55, dmichael wrote: > > > On 2011/08/12 16:58:18, jond wrote: > > > > The function name here is lowercase. So, I thought it looked bad to start > a > > > > sentence pp_module()... That's why I say "The <code>pp_module</code> > > > function... > > > Okay, that's fine. But it should still be pp_module() instead of > > > <code>pp_module</code>, shouldn't it? Same elsewhere. > > > > > > > > On 2011/08/11 22:20:50, dmichael wrote: > > > > > Why are you using 'The <code>function_name</code> function' here, but > > > > > 'function_name()' above? > > > > > > > > > > Please make them consistent. I think you've been using the other style > > > > > elsewhere, so: > > > > > /// pp_module() returns... > > > > > > > > > >
LGTM |
