Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6286018:
Updated descriptions to several functions, structs, enums (Closed)
|
Created:
9 years, 10 months ago by jond Modified:
9 years, 7 months ago CC:
chromium-reviews, piman+watch_chromium.org, josiew_google.com, jhartman _google.com, awatson1 Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionPatch Set 1 #
Total comments: 17
Patch Set 2 : '' #
Total comments: 8
Patch Set 3 : '' #Messages
Total messages: 8 (0 generated)
This is my first try at cleaning up comments and adding more information to the Pepper C API. Feedback is very important at this point so I can start to establish a standard for which our APIs are documented.
http://codereview.chromium.org/6286018/diff/1/ppapi/c/pp_var.h File ppapi/c/pp_var.h (right): http://codereview.chromium.org/6286018/diff/1/ppapi/c/pp_var.h#newcode47 ppapi/c/pp_var.h:47: * PP_VAR is a struct that represents a variant data type and can contain any PP_VAR does not represent a variant data type; it is a variant data type. It can't contain any data type, just the ones that are enumerated by PP_VarType. http://codereview.chromium.org/6286018/diff/1/ppapi/c/pp_var.h#newcode55 ppapi/c/pp_var.h:55: * numbers. The JavaScript library will try to optimize operations by using Javascript library -> Javascript operations ie "Javascript operations will try to optimize" The phrase "Javascript library," tends to mean javascript code modules (e.g. node.js) http://codereview.chromium.org/6286018/diff/1/ppapi/c/ppp.h File ppapi/c/ppp.h (right): http://codereview.chromium.org/6286018/diff/1/ppapi/c/ppp.h#newcode80 ppapi/c/ppp.h:80: * or our module gaining or loosing focus on a page, these functions will s/loosing/losing
http://codereview.chromium.org/6286018/diff/1/ppapi/c/pp_var.h File ppapi/c/pp_var.h (right): http://codereview.chromium.org/6286018/diff/1/ppapi/c/pp_var.h#newcode48 ppapi/c/pp_var.h:48: * value, such as a bool, int32, double, or string. This structure is for 'int32' -> int32_t 'or string' -> 'string, or object.' (I'm being a bit vague... we might be able to say 'string, JavaScript object, or a scriptable native object (see PPB_Class)'. Not sure which is less confusing. Also... it may not be good to say 'any value', since it is limited to a particular set of types. Maybe just get rid of 'any value, such as' so it just says: "...can contain a bool, int32_t, double, ..." http://codereview.chromium.org/6286018/diff/1/ppapi/c/pp_var.h#newcode59 ppapi/c/pp_var.h:59: * that always returns the type you expect (converting as necessary). I don't believe this is true. PPB_Var has a function for converting, but you have to do it manually. A numeric PP_Var coming from JavaScript could be int32_t or double, and the native code can't make assumptions. That's what the comment was trying to warn about. pp::Var (the C++ wrapper) may do conversions more automatically, but PP_Var does not and never will (I'm not sure how you'd do it in C). http://codereview.chromium.org/6286018/diff/1/ppapi/c/ppp.h File ppapi/c/ppp.h (right): http://codereview.chromium.org/6286018/diff/1/ppapi/c/ppp.h#newcode39 ppapi/c/ppp.h:39: * called by the browser when your module loads. Your code will implement this maybe change 'will' to 'must'? http://codereview.chromium.org/6286018/diff/1/ppapi/c/ppp.h#newcode47 ppapi/c/ppp.h:47: * @return PP_OK on success. Any other value on failure Two spaces after the period. I would also maybe put a period after each description (maybe in PP_Var too). http://codereview.chromium.org/6286018/diff/1/ppapi/c/ppp.h#newcode62 ppapi/c/ppp.h:62: * Your code will implement this function. will->must? http://codereview.chromium.org/6286018/diff/1/ppapi/c/ppp.h#newcode80 ppapi/c/ppp.h:80: * or our module gaining or loosing focus on a page, these functions will our->your, will->must http://codereview.chromium.org/6286018/diff/1/ppapi/c/ppp.h#newcode82 ppapi/c/ppp.h:82: * more information on these functions. Is it really accurate to focus on PPP_Instance so much? I thought this was usable for other interfaces...
http://codereview.chromium.org/6286018/diff/1/ppapi/c/pp_var.h File ppapi/c/pp_var.h (right): http://codereview.chromium.org/6286018/diff/1/ppapi/c/pp_var.h#newcode47 ppapi/c/pp_var.h:47: * PP_VAR is a struct that represents a variant data type and can contain any On 2011/02/01 19:19:00, Sang Ahn wrote: > PP_VAR does not represent a variant data type; it is a variant data type. It > can't contain any data type, just the ones that are enumerated by PP_VarType. Fixed. http://codereview.chromium.org/6286018/diff/1/ppapi/c/pp_var.h#newcode48 ppapi/c/pp_var.h:48: * value, such as a bool, int32, double, or string. This structure is for It says this now: "The PP_VAR struct is a variant data type and can contain any * value represented by the PP_VarType enum. " On 2011/02/01 22:17:22, dmichael wrote: > 'int32' -> int32_t > 'or string' -> 'string, or object.' > (I'm being a bit vague... we might be able to say 'string, JavaScript object, > or a scriptable native object (see PPB_Class)'. Not sure which is less > confusing. > > Also... it may not be good to say 'any value', since it is limited to a > particular set of types. Maybe just get rid of 'any value, such as' so it just > says: > "...can contain a bool, int32_t, double, ..." > http://codereview.chromium.org/6286018/diff/1/ppapi/c/pp_var.h#newcode55 ppapi/c/pp_var.h:55: * numbers. The JavaScript library will try to optimize operations by using Fixed. On 2011/02/01 19:19:00, Sang Ahn wrote: > Javascript library -> Javascript operations > ie > "Javascript operations will try to optimize" > > The phrase "Javascript library," tends to mean javascript code modules (e.g. > node.js) http://codereview.chromium.org/6286018/diff/1/ppapi/c/ppp.h File ppapi/c/ppp.h (right): http://codereview.chromium.org/6286018/diff/1/ppapi/c/ppp.h#newcode39 ppapi/c/ppp.h:39: * called by the browser when your module loads. Your code will implement this Fixed. On 2011/02/01 22:17:22, dmichael wrote: > maybe change 'will' to 'must'? http://codereview.chromium.org/6286018/diff/1/ppapi/c/ppp.h#newcode47 ppapi/c/ppp.h:47: * @return PP_OK on success. Any other value on failure Fixed. On 2011/02/01 22:17:22, dmichael wrote: > Two spaces after the period. I would also maybe put a period after each > description (maybe in PP_Var too). http://codereview.chromium.org/6286018/diff/1/ppapi/c/ppp.h#newcode62 ppapi/c/ppp.h:62: * Your code will implement this function. Fixed. On 2011/02/01 22:17:22, dmichael wrote: > will->must? http://codereview.chromium.org/6286018/diff/1/ppapi/c/ppp.h#newcode80 ppapi/c/ppp.h:80: * or our module gaining or loosing focus on a page, these functions will Fixed. On 2011/02/01 22:17:22, dmichael wrote: > our->your, will->must
Please do 'gcl upload' again so we can see your changes. They're not appearing on the web page yet. Thanks! On Wed, Feb 2, 2011 at 9:54 AM, <jond@google.com> wrote: > > http://codereview.chromium.org/6286018/diff/1/ppapi/c/pp_var.h > File ppapi/c/pp_var.h (right): > > http://codereview.chromium.org/6286018/diff/1/ppapi/c/pp_var.h#newcode47 > ppapi/c/pp_var.h:47: * PP_VAR is a struct that represents a variant data > type and can contain any > On 2011/02/01 19:19:00, Sang Ahn wrote: > >> PP_VAR does not represent a variant data type; it is a variant data >> > type. It > >> can't contain any data type, just the ones that are enumerated by >> > PP_VarType. > > Fixed. > > http://codereview.chromium.org/6286018/diff/1/ppapi/c/pp_var.h#newcode48 > ppapi/c/pp_var.h:48: * value, such as a bool, int32, double, or string. > This structure is for > It says this now: > > "The PP_VAR struct is a variant data type and can contain any > * value represented by the PP_VarType enum. " > > On 2011/02/01 22:17:22, dmichael wrote: > >> 'int32' -> int32_t >> 'or string' -> 'string, or object.' >> (I'm being a bit vague... we might be able to say 'string, JavaScript >> > object, > >> or a scriptable native object (see PPB_Class)'. Not sure which is >> > less > >> confusing. >> > > Also... it may not be good to say 'any value', since it is limited to >> > a > >> particular set of types. Maybe just get rid of 'any value, such as' >> > so it just > >> says: >> "...can contain a bool, int32_t, double, ..." >> > > > http://codereview.chromium.org/6286018/diff/1/ppapi/c/pp_var.h#newcode55 > ppapi/c/pp_var.h:55: * numbers. The JavaScript library will try to > optimize operations by using > Fixed. > > On 2011/02/01 19:19:00, Sang Ahn wrote: > >> Javascript library -> Javascript operations >> ie >> "Javascript operations will try to optimize" >> > > The phrase "Javascript library," tends to mean javascript code modules >> > (e.g. > >> node.js) >> > > http://codereview.chromium.org/6286018/diff/1/ppapi/c/ppp.h > File ppapi/c/ppp.h (right): > > http://codereview.chromium.org/6286018/diff/1/ppapi/c/ppp.h#newcode39 > ppapi/c/ppp.h:39: * called by the browser when your module loads. Your > code will implement this > Fixed. > > On 2011/02/01 22:17:22, dmichael wrote: > >> maybe change 'will' to 'must'? >> > > http://codereview.chromium.org/6286018/diff/1/ppapi/c/ppp.h#newcode47 > ppapi/c/ppp.h:47: * @return PP_OK on success. Any other value on failure > Fixed. > > On 2011/02/01 22:17:22, dmichael wrote: > >> Two spaces after the period. I would also maybe put a period after >> > each > >> description (maybe in PP_Var too). >> > > http://codereview.chromium.org/6286018/diff/1/ppapi/c/ppp.h#newcode62 > ppapi/c/ppp.h:62: * Your code will implement this function. > Fixed. > > On 2011/02/01 22:17:22, dmichael wrote: > >> will->must? >> > > http://codereview.chromium.org/6286018/diff/1/ppapi/c/ppp.h#newcode80 > ppapi/c/ppp.h:80: * or our module gaining or loosing focus on a page, > these functions will > Fixed. > On 2011/02/01 22:17:22, dmichael wrote: > >> our->your, will->must >> > > http://codereview.chromium.org/6286018/ >
http://codereview.chromium.org/6286018/diff/7/ppapi/c/pp_var.h File ppapi/c/pp_var.h (right): http://codereview.chromium.org/6286018/diff/7/ppapi/c/pp_var.h#newcode49 ppapi/c/pp_var.h:49: * value represented by the PP_VarType enum. This structure is for maybe instead of 'any value represented by the PP_VarType enum': 'any value of one of the types named in the PP_VarType' (or similar) http://codereview.chromium.org/6286018/diff/7/ppapi/c/pp_var.h#newcode58 ppapi/c/pp_var.h:58: * Your code should be capable of handle either int32_t or double for numeric handle->handling (my fault) http://codereview.chromium.org/6286018/diff/7/ppapi/c/ppp.h File ppapi/c/ppp.h (right): http://codereview.chromium.org/6286018/diff/7/ppapi/c/ppp.h#newcode20 ppapi/c/ppp.h:20: * This file defines three functions that your Native Client module must Is it okay to talk about Native Client here? PPAPI isn't specific to native client; it's also the way to do trusted Chrome plugins. Maybe just say 'module' to be appropriately vague.
http://codereview.chromium.org/6286018/diff/7/ppapi/c/pp_var.h File ppapi/c/pp_var.h (right): http://codereview.chromium.org/6286018/diff/7/ppapi/c/pp_var.h#newcode49 ppapi/c/pp_var.h:49: * value represented by the PP_VarType enum. This structure is for On 2011/02/02 20:12:59, dmichael wrote: > maybe instead of 'any value represented by the PP_VarType enum': > 'any value of one of the types named in the PP_VarType' (or similar) Done. http://codereview.chromium.org/6286018/diff/7/ppapi/c/pp_var.h#newcode58 ppapi/c/pp_var.h:58: * Your code should be capable of handle either int32_t or double for numeric On 2011/02/02 20:12:59, dmichael wrote: > handle->handling > (my fault) Done. http://codereview.chromium.org/6286018/diff/7/ppapi/c/pp_var.h#newcode58 ppapi/c/pp_var.h:58: * Your code should be capable of handle either int32_t or double for numeric On 2011/02/02 20:12:59, dmichael wrote: > handle->handling > (my fault) Done. http://codereview.chromium.org/6286018/diff/7/ppapi/c/ppp.h File ppapi/c/ppp.h (right): http://codereview.chromium.org/6286018/diff/7/ppapi/c/ppp.h#newcode20 ppapi/c/ppp.h:20: * This file defines three functions that your Native Client module must On 2011/02/02 20:12:59, dmichael wrote: > Is it okay to talk about Native Client here? PPAPI isn't specific to native > client; it's also the way to do trusted Chrome plugins. > > Maybe just say 'module' to be appropriately vague. Done. http://codereview.chromium.org/6286018/diff/7/ppapi/c/ppp.h#newcode20 ppapi/c/ppp.h:20: * This file defines three functions that your Native Client module must On 2011/02/02 20:12:59, dmichael wrote: > Is it okay to talk about Native Client here? PPAPI isn't specific to native > client; it's also the way to do trusted Chrome plugins. > > Maybe just say 'module' to be appropriately vague. Done.
LGTM, but I would recommend gclient sync and gcl try before you commit. |
