Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7108044:
C++ documentation for URL Loading classes (Closed)
|
Created:
9 years, 6 months ago by jond Modified:
9 years, 4 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionC++ documentation for URL Loading classes
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96030
Patch Set 1 #Patch Set 2 : '' #
Total comments: 77
Patch Set 3 : '' #
Total comments: 22
Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 6
Messages
Total messages: 19 (0 generated)
With the introduction of IDL, I think you should not be making these changes in the ppapi/api directory and then running scripts from ppapi/generators to update headers. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h File ppapi/cpp/url_loader.h (right): http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:13: /// This file defines the API to download URLs. s/to download/for loading/ (to match C description) http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:21: /// URLLoader provides an API to download URLs. ditto http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:42: /// return factory_.NewCallback(&MyHandler::DidCompleteIO); You lost NewOptionalCallback in a sync. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:85: /// Default constructor. This constructor creates an is_null() default constructor for creating ... [to match image_data.h and graphics2d.h] http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:100: /// return value whose reference count we need to increment. This is wrong. Only resources have reference counts, not instances. This should be: A constructor used to allocate a new URLLoader in the browser. The resulting object will be is_null if the allocation failed. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:105: /// A constructor for copying an <code>URLLoader</code>. The copy constructor (common C++ terminology) http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:115: /// @param[in] request_info A </code>PP_Resource</code> corresponding to a URLRequestInfo, not PP_Resource http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:120: /// will only run if Open() returns <code>PP_OK_COMPLETIONPENDING</code>. forgot @return http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:136: /// This function returns the current upload progress (which is meaningful is only meaningful? http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:165: /// @return True if the download progress is available, False if it is true/false (no True/False in C++) do these need <code></code>? http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:173: /// @return A <code>PP_Resource</code> corresponding to the URLResponseInfo, not PP_Resource http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:174: /// <code>URLResponseInfo</code> if successful, 0 if the loader is not a the return value is an object, so it cannot be 0, but an is_null object http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h File ppapi/cpp/url_request_info.h (right): http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:13: /// This file defines the API to create and manipulate URL requests. API for creating ... (to match C interface and description below) http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:26: /// A constructor used when an <code>Instance</code> is provided as a return This is wrong. See URLLoader constructor comment. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:32: /// A constructor for copying an <code>URLRequestInfo</code>. The copy constructor for URLRequestInfo. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:40: /// @param[in] property A <code>URLRequestProperty</code> identifying the PP_URLRequestProperty http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:59: /// the request body. A content-length request header will be automatically inconsistent: previous comment says Content-Length http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:62: /// @param[in] file_ref A <code>PP_Resource</code> containing the file FileRef_Dev, not PP_Resource http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:86: /// be included. If -1, then the sub-range to upload extends to the end of the file. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:102: /// (corresponding to a string of type <code>PP_VARTYPE_STRING</code>) I don't think the info in () is necessary. First of all, it is a C reference in a comment for a function that is designed to abstract out the C interface. Plus this is already covered by the header that defines these properties. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:107: /// @return True if successful, false if any of the parameters are invalid. these is only one parameter (same for other functions) http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:142: /// to a bool of type <code>PP_VARTYPE_BOOL</code>). The default of the bool "default of the bool" sounds odd how about: the default of the property is the default value is (same for all other functions please) http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:146: /// @param[in] enable A <code>Var</code> containing the property value. bool, not Var (same for all other) http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:160: /// @param[in] enable A <code>Var</code> containing the property value. bool, not Var http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.h File ppapi/cpp/url_response_info.h (right): http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:24: /// Not sure what to put here Copy from image_data.h http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:27: /// A constructor used when a PP_Resource is provided as a return value not really Copy from image_data.h http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:33: /// A constructor for copying an URLResponseInfo. The copy constructor http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:36: // PPB_URLResponseInfo methods: Did you leave this here by accident? http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:42: /// @return A PP_Var containing the response property value if successful, Var, not PP_Var (please apply to other functions as well) http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:43: /// PP_VARTYPE_VOID if an input parameter is invalid. is_undefined Var, not PP_VARTYPE_VOID (please apply to other functions as well) http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:46: /// This function returns a FileRef You seem to be using <code> in some places, but not others. Is that on purpose? Is there some kind of pattern I am not recognizing? http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:53: /// @return A PP_Resource corresponding to a FileRef if successful, 0 if FileRef_Dev, not PP_Resource http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:53: /// @return A PP_Resource corresponding to a FileRef if successful, 0 if is_null object, not 0 http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:58: /// This function gets the PP_URLRESPONSEPROPERTY_URL (corresponding to a instead of putting "corresponding to a type..." in (), I would put it in the description of the return value: @return An is_string Var ...., is_undefined Var .... (for all other functions too)
http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h File ppapi/cpp/url_loader.h (right): http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:13: /// This file defines the API to download URLs. On 2011/08/01 10:35:51, polina wrote: > s/to download/for loading/ > (to match C description) Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:13: /// This file defines the API to download URLs. On 2011/08/01 10:35:51, polina wrote: > s/to download/for loading/ > (to match C description) Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:21: /// URLLoader provides an API to download URLs. On 2011/08/01 10:35:51, polina wrote: > ditto Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:42: /// return factory_.NewCallback(&MyHandler::DidCompleteIO); On 2011/08/01 10:35:51, polina wrote: > You lost NewOptionalCallback in a sync. Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:85: /// Default constructor. This constructor creates an is_null() On 2011/08/01 10:35:51, polina wrote: > default constructor for creating ... > [to match image_data.h and graphics2d.h] Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:100: /// return value whose reference count we need to increment. On 2011/08/01 10:35:51, polina wrote: > This is wrong. Only resources have reference counts, not instances. > > This should be: > A constructor used to allocate a new URLLoader in the browser. The resulting > object will be is_null if the allocation failed. Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:105: /// A constructor for copying an <code>URLLoader</code>. On 2011/08/01 10:35:51, polina wrote: > The copy constructor > (common C++ terminology) Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:105: /// A constructor for copying an <code>URLLoader</code>. On 2011/08/01 10:35:51, polina wrote: > The copy constructor > (common C++ terminology) Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:115: /// @param[in] request_info A </code>PP_Resource</code> corresponding to a On 2011/08/01 10:35:51, polina wrote: > URLRequestInfo, not PP_Resource Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:120: /// will only run if Open() returns <code>PP_OK_COMPLETIONPENDING</code>. On 2011/08/01 10:35:51, polina wrote: > forgot @return Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:136: /// This function returns the current upload progress (which is meaningful On 2011/08/01 10:35:51, polina wrote: > is only meaningful? Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:165: /// @return True if the download progress is available, False if it is On 2011/08/01 10:35:51, polina wrote: > true/false > (no True/False in C++) > > do these need <code></code>? Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:165: /// @return True if the download progress is available, False if it is On 2011/08/01 10:35:51, polina wrote: > true/false > (no True/False in C++) > > do these need <code></code>? Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:173: /// @return A <code>PP_Resource</code> corresponding to the On 2011/08/01 10:35:51, polina wrote: > URLResponseInfo, not PP_Resource Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_loader.h#newco... ppapi/cpp/url_loader.h:174: /// <code>URLResponseInfo</code> if successful, 0 if the loader is not a On 2011/08/01 10:35:51, polina wrote: > the return value is an object, so it cannot be 0, but an is_null object Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h File ppapi/cpp/url_request_info.h (right): http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:13: /// This file defines the API to create and manipulate URL requests. On 2011/08/01 10:35:51, polina wrote: > API for creating ... (to match C interface and description below) Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:26: /// A constructor used when an <code>Instance</code> is provided as a return On 2011/08/01 10:35:51, polina wrote: > This is wrong. See URLLoader constructor comment. Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:32: /// A constructor for copying an <code>URLRequestInfo</code>. On 2011/08/01 10:35:51, polina wrote: > The copy constructor for URLRequestInfo. Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:40: /// @param[in] property A <code>URLRequestProperty</code> identifying the On 2011/08/01 10:35:51, polina wrote: > PP_URLRequestProperty Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:59: /// the request body. A content-length request header will be automatically On 2011/08/01 10:35:51, polina wrote: > inconsistent: previous comment says Content-Length Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:62: /// @param[in] file_ref A <code>PP_Resource</code> containing the file On 2011/08/01 10:35:51, polina wrote: > FileRef_Dev, not PP_Resource Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:62: /// @param[in] file_ref A <code>PP_Resource</code> containing the file On 2011/08/01 10:35:51, polina wrote: > FileRef_Dev, not PP_Resource Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:86: /// be included. On 2011/08/01 10:35:51, polina wrote: > If -1, then the sub-range to upload extends to the end of the file. Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:86: /// be included. On 2011/08/01 10:35:51, polina wrote: > If -1, then the sub-range to upload extends to the end of the file. Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:102: /// (corresponding to a string of type <code>PP_VARTYPE_STRING</code>) On 2011/08/01 10:35:51, polina wrote: > I don't think the info in () is necessary. First of all, it is a C reference in > a comment for a function that is designed to abstract out the C interface. Plus > this is already covered by the header that defines these properties. Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:102: /// (corresponding to a string of type <code>PP_VARTYPE_STRING</code>) On 2011/08/01 10:35:51, polina wrote: > I don't think the info in () is necessary. First of all, it is a C reference in > a comment for a function that is designed to abstract out the C interface. Plus > this is already covered by the header that defines these properties. Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:107: /// @return True if successful, false if any of the parameters are invalid. On 2011/08/01 10:35:51, polina wrote: > these is only one parameter > (same for other functions) Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:142: /// to a bool of type <code>PP_VARTYPE_BOOL</code>). The default of the bool On 2011/08/01 10:35:51, polina wrote: > "default of the bool" sounds odd > how about: > the default of the property is > the default value is > (same for all other functions please) Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:146: /// @param[in] enable A <code>Var</code> containing the property value. On 2011/08/01 10:35:51, polina wrote: > bool, not Var > (same for all other) Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:160: /// @param[in] enable A <code>Var</code> containing the property value. On 2011/08/01 10:35:51, polina wrote: > bool, not Var Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.h File ppapi/cpp/url_response_info.h (right): http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:24: /// Not sure what to put here On 2011/08/01 10:35:51, polina wrote: > Copy from image_data.h Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:27: /// A constructor used when a PP_Resource is provided as a return value On 2011/08/01 10:35:51, polina wrote: > not really > Copy from image_data.h Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:33: /// A constructor for copying an URLResponseInfo. On 2011/08/01 10:35:51, polina wrote: > The copy constructor Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:36: // PPB_URLResponseInfo methods: On 2011/08/01 10:35:51, polina wrote: > Did you leave this here by accident? Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:42: /// @return A PP_Var containing the response property value if successful, On 2011/08/01 10:35:51, polina wrote: > Var, not PP_Var > (please apply to other functions as well) > Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:43: /// PP_VARTYPE_VOID if an input parameter is invalid. On 2011/08/01 10:35:51, polina wrote: > is_undefined Var, not PP_VARTYPE_VOID > (please apply to other functions as well) Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:43: /// PP_VARTYPE_VOID if an input parameter is invalid. On 2011/08/01 10:35:51, polina wrote: > is_undefined Var, not PP_VARTYPE_VOID > (please apply to other functions as well) Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:46: /// This function returns a FileRef On 2011/08/01 10:35:51, polina wrote: > You seem to be using <code> in some places, but not others. Is that on purpose? > Is there some kind of pattern I am not recognizing? Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:53: /// @return A PP_Resource corresponding to a FileRef if successful, 0 if On 2011/08/01 10:35:51, polina wrote: > FileRef_Dev, not PP_Resource Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:53: /// @return A PP_Resource corresponding to a FileRef if successful, 0 if On 2011/08/01 10:35:51, polina wrote: > is_null object, not 0 Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:53: /// @return A PP_Resource corresponding to a FileRef if successful, 0 if On 2011/08/01 10:35:51, polina wrote: > FileRef_Dev, not PP_Resource Done. http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_response_info.... ppapi/cpp/url_response_info.h:58: /// This function gets the PP_URLRESPONSEPROPERTY_URL (corresponding to a On 2011/08/01 10:35:51, polina wrote: > instead of putting "corresponding to a type..." in (), I would put it in the > description of the return value: > > @return An is_string Var ...., is_undefined Var .... > > (for all other functions too) Done.
New files uploaded.
http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h File ppapi/cpp/url_request_info.h (right): http://codereview.chromium.org/7108044/diff/5001/ppapi/cpp/url_request_info.h... ppapi/cpp/url_request_info.h:102: /// (corresponding to a string of type <code>PP_VARTYPE_STRING</code>) On 2011/08/01 10:35:51, polina wrote: > I don't think the info in () is necessary. First of all, it is a C reference in > a comment for a function that is designed to abstract out the C interface. Plus > this is already covered by the header that defines these properties. If you agree that this should be removed, please do this for every function. Moreover, it would be nice if you used the same trick you followed in the other file where the is_string type reference is added to the arg description. E.g. @param ... An <code>is_string Var</code> containing.... http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_loader.h File ppapi/cpp/url_loader.h (right): http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_loader.h#newc... ppapi/cpp/url_loader.h:105: /// The copy constructor. The copy constructor for <code>URLLoader</code>? http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_loader.h#newc... ppapi/cpp/url_loader.h:115: /// @param[in] request_info A </code>URL_RequestInfo</code> corresponding to a URLRequestInfo (no _) http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_request_info.h File ppapi/cpp/url_request_info.h (right): http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_request_info.... ppapi/cpp/url_request_info.h:22: /// Default constructor. This constructor creates an you changed the other file to say "constructor for creating" (I don't really care what wording you choose, I just think it should be consistent across interfaces/files.) http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_request_info.... ppapi/cpp/url_request_info.h:63: /// @param[in] file_ref A <code>FileRef_Dev</code> containing the file Actually this has been moved out of Dev, so there should be no _Dev here and in the function signature. Have you synced? http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_request_info.... ppapi/cpp/url_request_info.h:87: /// be included. If -1, then the sub-range to upload extends to the end of If the value is -1? http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_response_info.h File ppapi/cpp/url_response_info.h (right): http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_response_info... ppapi/cpp/url_response_info.h:21: /// Default constructor. This constructor creates an <code>is_null</code> same comment about consistency as in the other file http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_response_info... ppapi/cpp/url_response_info.h:35: /// The copy constructor. for <code>URLResponseInfo</code>? http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_response_info... ppapi/cpp/url_response_info.h:54: /// @return A <code>FileRef_Dev</code> corresponding to a no _Dev here and in the function signature? (sync issue?)
http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_loader.h File ppapi/cpp/url_loader.h (right): http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_loader.h#newc... ppapi/cpp/url_loader.h:105: /// The copy constructor. On 2011/08/03 00:28:33, polina wrote: > The copy constructor for <code>URLLoader</code>? Done. http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_loader.h#newc... ppapi/cpp/url_loader.h:115: /// @param[in] request_info A </code>URL_RequestInfo</code> corresponding to a On 2011/08/03 00:28:33, polina wrote: > URLRequestInfo (no _) Done. http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_request_info.h File ppapi/cpp/url_request_info.h (right): http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_request_info.... ppapi/cpp/url_request_info.h:22: /// Default constructor. This constructor creates an I'll note this and look at it for a global change. On 2011/08/03 00:28:33, polina wrote: > you changed the other file to say "constructor for creating" > > (I don't really care what wording you choose, I just think it should be > consistent across interfaces/files.) http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_request_info.... ppapi/cpp/url_request_info.h:63: /// @param[in] file_ref A <code>FileRef_Dev</code> containing the file I sync'd but got to conflict - so apparently it kept my old version. I just removed the files and resync'd and got the newer files. So, now I should be up to date. On 2011/08/03 00:28:33, polina wrote: > Actually this has been moved out of Dev, so there should be no _Dev here and in > the function signature. Have you synced? http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_request_info.... ppapi/cpp/url_request_info.h:87: /// be included. If -1, then the sub-range to upload extends to the end of On 2011/08/03 00:28:33, polina wrote: > If the value is -1? Done.
http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_request_info.h File ppapi/cpp/url_request_info.h (right): http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_request_info.... ppapi/cpp/url_request_info.h:63: /// @param[in] file_ref A <code>FileRef_Dev</code> containing the file On 2011/08/04 15:50:33, jond wrote: > I sync'd but got to conflict - so apparently it kept my old version. I just > removed the files and resync'd and got the newer files. So, now I should be up > to date. > > On 2011/08/03 00:28:33, polina wrote: > > Actually this has been moved out of Dev, so there should be no _Dev here and > in > > the function signature. Have you synced? > Yes, the _Dev's are gone from code. But they still appear in your comments, that are not affect by the merge because you just added them. You need to take out _Dev manually. http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_request_info.... ppapi/cpp/url_request_info.h:87: /// be included. If -1, then the sub-range to upload extends to the end of On 2011/08/04 15:50:33, jond wrote: > On 2011/08/03 00:28:33, polina wrote: > > If the value is -1? > > Done. Looks like you remove this sentence completely. Was this the intention? My comment was to clarify the wording going from "If -1" to "If value is -1". http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_response_info.h File ppapi/cpp/url_response_info.h (right): http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_response_info... ppapi/cpp/url_response_info.h:54: /// @return A <code>FileRef_Dev</code> corresponding to a On 2011/08/03 00:28:33, polina wrote: > no _Dev here and in the function signature? > (sync issue?) Still erroneous _Dev in your comment.
http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_request_info.h File ppapi/cpp/url_request_info.h (right): http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_request_info.... ppapi/cpp/url_request_info.h:63: /// @param[in] file_ref A <code>FileRef_Dev</code> containing the file On 2011/08/04 22:01:04, polina wrote: > On 2011/08/04 15:50:33, jond wrote: > > I sync'd but got to conflict - so apparently it kept my old version. I just > > removed the files and resync'd and got the newer files. So, now I should be up > > to date. > > > > On 2011/08/03 00:28:33, polina wrote: > > > Actually this has been moved out of Dev, so there should be no _Dev here and > > in > > > the function signature. Have you synced? > > > > Yes, the _Dev's are gone from code. But they still appear in your comments, that > are not affect by the merge because you just added them. You need to take out > _Dev manually. Done. http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_request_info.... ppapi/cpp/url_request_info.h:87: /// be included. If -1, then the sub-range to upload extends to the end of On 2011/08/04 22:01:04, polina wrote: > On 2011/08/04 15:50:33, jond wrote: > > On 2011/08/03 00:28:33, polina wrote: > > > If the value is -1? > > > > Done. > > Looks like you remove this sentence completely. Was this the intention? My > comment was to clarify the wording going from "If -1" to "If value is -1". Done. http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_response_info.h File ppapi/cpp/url_response_info.h (right): http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_response_info... ppapi/cpp/url_response_info.h:21: /// Default constructor. This constructor creates an <code>is_null</code> Right. I will address these all at the same time across all files. On 2011/08/03 00:28:33, polina wrote: > same comment about consistency as in the other file http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_response_info... ppapi/cpp/url_response_info.h:35: /// The copy constructor. On 2011/08/03 00:28:33, polina wrote: > for <code>URLResponseInfo</code>? Done. http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_response_info... ppapi/cpp/url_response_info.h:35: /// The copy constructor. On 2011/08/03 00:28:33, polina wrote: > for <code>URLResponseInfo</code>? Done. http://codereview.chromium.org/7108044/diff/13001/ppapi/cpp/url_response_info... ppapi/cpp/url_response_info.h:54: /// @return A <code>FileRef_Dev</code> corresponding to a On 2011/08/03 00:28:33, polina wrote: > no _Dev here and in the function signature? > (sync issue?) Done.
LGTM
No LGTM from valid reviewers yet.
rubber-stamp LGTM
http://codereview.chromium.org/7108044/diff/27001/ppapi/cpp/url_loader.h File ppapi/cpp/url_loader.h (right): http://codereview.chromium.org/7108044/diff/27001/ppapi/cpp/url_loader.h#newc... ppapi/cpp/url_loader.h:26: /// class MyHandler { This code example should not have been added back. The code doesn't compile and some of the callback handling is subtly and dangerously incorrect. This is why I made a separate example that's actually compiled in the build, so we can ensure it actually works. Can you please revert this part of this change? In the future, can you please be extra careful when updating things that have changed since starting working on the documentation? There have been several cases of comment fixes getting reverted because a comment from an older revision was rewritten and pasted over the updated version. It should be a huge red flag when you get a merge conflict, and we always need to understand the conflicts rather than overwriting the changes.
http://codereview.chromium.org/7108044/diff/27001/ppapi/cpp/url_loader.h File ppapi/cpp/url_loader.h (right): http://codereview.chromium.org/7108044/diff/27001/ppapi/cpp/url_loader.h#newc... ppapi/cpp/url_loader.h:26: /// class MyHandler { I actually don't know how this example got in here if it wasn't there previously. And, there doesn't seem to be a url_loader.cc at the location on the left. Do you simply want me to remove the example? On 2011/08/23 23:29:01, brettw wrote: > This code example should not have been added back. The code doesn't compile and > some of the callback handling is subtly and dangerously incorrect. This is why I > made a separate example that's actually compiled in the build, so we can ensure > it actually works. > > Can you please revert this part of this change? > > In the future, can you please be extra careful when updating things that have > changed since starting working on the documentation? There have been several > cases of comment fixes getting reverted because a comment from an older revision > was rewritten and pasted over the updated version. It should be a huge red flag > when you get a merge conflict, and we always need to understand the conflicts > rather than overwriting the changes.
http://codereview.chromium.org/7108044/diff/27001/ppapi/cpp/url_loader.h File ppapi/cpp/url_loader.h (right): http://codereview.chromium.org/7108044/diff/27001/ppapi/cpp/url_loader.h#newc... ppapi/cpp/url_loader.h:26: /// class MyHandler { On 2011/08/24 15:33:59, jond wrote: > I actually don't know how this example got in here if it wasn't there > previously. And, there doesn't seem to be a url_loader.cc at the location on the > left. Do you simply want me to remove the example? > > On 2011/08/23 23:29:01, brettw wrote: > > This code example should not have been added back. The code doesn't compile > and > > some of the callback handling is subtly and dangerously incorrect. This is why > I > > made a separate example that's actually compiled in the build, so we can > ensure > > it actually works. > > > > Can you please revert this part of this change? > > > > In the future, can you please be extra careful when updating things that have > > changed since starting working on the documentation? There have been several > > cases of comment fixes getting reverted because a comment from an older > revision > > was rewritten and pasted over the updated version. It should be a huge red > flag > > when you get a merge conflict, and we always need to understand the conflicts > > rather than overwriting the changes. > Jon, if you view the diff for patch sets 1 or 2, they will be compared against the older revision that still had this example. Remember I even had you fix some other merge-related omission in my first set of comments? Anyway, it looks like after that you synced to a newer revision that no longer had the example. But since your local copy still did, it got erroneously put back in a merge conflict. After a sync and before a commit, it is always a good idea to look at the most recent diff, to see how the merge conflicts got resolved, both automatically and manually. So at this point, you should just remove this example from line 22 to line 81 and replace it with the comment pointing to ppapi/examples/url_loader/url_loader.cc on line 20 of the LHS.
http://codereview.chromium.org/7108044/diff/27001/ppapi/cpp/url_loader.h File ppapi/cpp/url_loader.h (right): http://codereview.chromium.org/7108044/diff/27001/ppapi/cpp/url_loader.h#newc... ppapi/cpp/url_loader.h:26: /// class MyHandler { On 2011/08/24 19:52:01, polina wrote: > On 2011/08/24 15:33:59, jond wrote: > > I actually don't know how this example got in here if it wasn't there > > previously. And, there doesn't seem to be a url_loader.cc at the location on > the > > left. Do you simply want me to remove the example? > > > > On 2011/08/23 23:29:01, brettw wrote: > > > This code example should not have been added back. The code doesn't compile > > and > > > some of the callback handling is subtly and dangerously incorrect. This is > why > > I > > > made a separate example that's actually compiled in the build, so we can > > ensure > > > it actually works. > > > > > > Can you please revert this part of this change? > > > > > > In the future, can you please be extra careful when updating things that > have > > > changed since starting working on the documentation? There have been several > > > cases of comment fixes getting reverted because a comment from an older > > revision > > > was rewritten and pasted over the updated version. It should be a huge red > > flag > > > when you get a merge conflict, and we always need to understand the > conflicts > > > rather than overwriting the changes. > > > > Jon, if you view the diff for patch sets 1 or 2, they will be compared against > the older revision that still had this example. Remember I even had you fix some > other merge-related omission in my first set of comments? Anyway, it looks like > after that you synced to a newer revision that no longer had the example. But > since your local copy still did, it got erroneously put back in a merge > conflict. After a sync and before a commit, it is always a good idea to look at > the most recent diff, to see how the merge conflicts got resolved, both > automatically and manually. > > So at this point, you should just remove this example from line 22 to line 81 > and replace it with the comment pointing to > ppapi/examples/url_loader/url_loader.cc on line 20 of the LHS. > And I just verified that the file is there: http://src.chromium.org/viewvc/chrome/trunk/src/ppapi/examples/url_loader/ You must have looked in the example/ dir instead of examples/ with an S.
http://codereview.chromium.org/7108044/diff/27001/ppapi/cpp/url_loader.h File ppapi/cpp/url_loader.h (right): http://codereview.chromium.org/7108044/diff/27001/ppapi/cpp/url_loader.h#newc... ppapi/cpp/url_loader.h:26: /// class MyHandler { On 2011/08/24 19:55:53, polina wrote: > On 2011/08/24 19:52:01, polina wrote: > > On 2011/08/24 15:33:59, jond wrote: > > > I actually don't know how this example got in here if it wasn't there > > > previously. And, there doesn't seem to be a url_loader.cc at the location on > > the > > > left. Do you simply want me to remove the example? > > > > > > On 2011/08/23 23:29:01, brettw wrote: > > > > This code example should not have been added back. The code doesn't > compile > > > and > > > > some of the callback handling is subtly and dangerously incorrect. This is > > why > > > I > > > > made a separate example that's actually compiled in the build, so we can > > > ensure > > > > it actually works. > > > > > > > > Can you please revert this part of this change? > > > > > > > > In the future, can you please be extra careful when updating things that > > have > > > > changed since starting working on the documentation? There have been > several > > > > cases of comment fixes getting reverted because a comment from an older > > > revision > > > > was rewritten and pasted over the updated version. It should be a huge red > > > flag > > > > when you get a merge conflict, and we always need to understand the > > conflicts > > > > rather than overwriting the changes. > > > > > > > Jon, if you view the diff for patch sets 1 or 2, they will be compared against > > the older revision that still had this example. Remember I even had you fix > some > > other merge-related omission in my first set of comments? Anyway, it looks > like > > after that you synced to a newer revision that no longer had the example. But > > since your local copy still did, it got erroneously put back in a merge > > conflict. After a sync and before a commit, it is always a good idea to look > at > > the most recent diff, to see how the merge conflicts got resolved, both > > automatically and manually. > > > > So at this point, you should just remove this example from line 22 to line 81 > > and replace it with the comment pointing to > > ppapi/examples/url_loader/url_loader.cc on line 20 of the LHS. > > > > And I just verified that the file is there: > > http://src.chromium.org/viewvc/chrome/trunk/src/ppapi/examples/url_loader/ > > You must have looked in the example/ dir instead of examples/ with an S. > Sorry, I rushed with my "helpful" comment. The directory is there, but the .cc file is not.
http://codereview.chromium.org/7108044/diff/27001/ppapi/cpp/url_loader.h File ppapi/cpp/url_loader.h (right): http://codereview.chromium.org/7108044/diff/27001/ppapi/cpp/url_loader.h#newc... ppapi/cpp/url_loader.h:26: /// class MyHandler { On 2011/08/24 20:06:14, polina wrote: > On 2011/08/24 19:55:53, polina wrote: > > On 2011/08/24 19:52:01, polina wrote: > > > On 2011/08/24 15:33:59, jond wrote: > > > > I actually don't know how this example got in here if it wasn't there > > > > previously. And, there doesn't seem to be a url_loader.cc at the location > on > > > the > > > > left. Do you simply want me to remove the example? > > > > > > > > On 2011/08/23 23:29:01, brettw wrote: > > > > > This code example should not have been added back. The code doesn't > > compile > > > > and > > > > > some of the callback handling is subtly and dangerously incorrect. This > is > > > why > > > > I > > > > > made a separate example that's actually compiled in the build, so we can > > > > ensure > > > > > it actually works. > > > > > > > > > > Can you please revert this part of this change? > > > > > > > > > > In the future, can you please be extra careful when updating things that > > > have > > > > > changed since starting working on the documentation? There have been > > several > > > > > cases of comment fixes getting reverted because a comment from an older > > > > revision > > > > > was rewritten and pasted over the updated version. It should be a huge > red > > > > flag > > > > > when you get a merge conflict, and we always need to understand the > > > conflicts > > > > > rather than overwriting the changes. > > > > > > > > > > Jon, if you view the diff for patch sets 1 or 2, they will be compared > against > > > the older revision that still had this example. Remember I even had you fix > > some > > > other merge-related omission in my first set of comments? Anyway, it looks > > like > > > after that you synced to a newer revision that no longer had the example. > But > > > since your local copy still did, it got erroneously put back in a merge > > > conflict. After a sync and before a commit, it is always a good idea to look > > at > > > the most recent diff, to see how the merge conflicts got resolved, both > > > automatically and manually. > > > > > > So at this point, you should just remove this example from line 22 to line > 81 > > > and replace it with the comment pointing to > > > ppapi/examples/url_loader/url_loader.cc on line 20 of the LHS. > > > > > > > And I just verified that the file is there: > > > > http://src.chromium.org/viewvc/chrome/trunk/src/ppapi/examples/url_loader/ > > > > You must have looked in the example/ dir instead of examples/ with an S. > > > > Sorry, I rushed with my "helpful" comment. The directory is there, but the .cc > file is not. Confirmed with Brett. Please point to streaming.cc. Thank you! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
