Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9813011:
Websocket documentation change (Closed)
|
Created:
8 years, 9 months ago by jond Modified:
8 years, 9 months ago CC:
chromium-reviews, piman+watch_chromium.org, darin-cc_chromium.org, yzshen+watch_chromium.org, ihf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionCleaned up documentation (standard documentation rewrite).
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129236
Patch Set 1 #
Total comments: 20
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Messages
Total messages: 13 (0 generated)
OK, I see. If you used git, '% git cl issue <old issue number>' would work for this kind of situation. It allows you to upload to old issue page from your new workspace. Anyway, submit history holds the description of code review page. So please describe CL summary in description. You may want to add some information for reviewer by 'Publish+Mail comments', or just by Messages form. >Description >Websocket documentation change. I applied the patches from the previous files (I had to install a new OS that deleted my CLs) and reuploaded. It created a new CL in code review. Can you review this one?
http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h File ppapi/cpp/websocket.h (right): http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode95 ppapi/cpp/websocket.h:95: /// <code>PP_ERROR_NOACCESS</code> corresponds to InvalidAccessError in the It's my original fault, but I need 'a' before InvalidAccessError here, like others? http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode135 ppapi/cpp/websocket.h:135: /// <code>PP_ERROR_FAILED</code> corresponds to the JavaScript the -> a ? http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode140 ppapi/cpp/websocket.h:140: /// JavaScript SyntaxError in the WebSocket API specification. ditto http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode179 ppapi/cpp/websocket.h:179: /// connection is established, the var's data is an empty string.Returns a <code>Var</code>'s is better? This term 'Var' is case-sensitive. http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode181 ppapi/cpp/websocket.h:181: /// Currently the var's data for valid resources are always an empty string. ditto http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode188 ppapi/cpp/websocket.h:188: /// connection is established, the var contains the empty string. Returns a ditto http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode203 ppapi/cpp/websocket.h:203: /// connection is established, the var contains the empty string. Returns a ditto
http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h File ppapi/cpp/websocket.h (right): http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode95 ppapi/cpp/websocket.h:95: /// <code>PP_ERROR_NOACCESS</code> corresponds to InvalidAccessError in the On 2012/03/21 17:36:40, toyoshim wrote: > It's my original fault, but I need 'a' before InvalidAccessError here, like > others? Done. http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode135 ppapi/cpp/websocket.h:135: /// <code>PP_ERROR_FAILED</code> corresponds to the JavaScript On 2012/03/21 17:36:40, toyoshim wrote: > the -> a ? Done. http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode140 ppapi/cpp/websocket.h:140: /// JavaScript SyntaxError in the WebSocket API specification. On 2012/03/21 17:36:40, toyoshim wrote: > ditto Done. http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode179 ppapi/cpp/websocket.h:179: /// connection is established, the var's data is an empty string.Returns a On 2012/03/21 17:36:40, toyoshim wrote: > <code>Var</code>'s is better? > This term 'Var' is case-sensitive. Done. http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode181 ppapi/cpp/websocket.h:181: /// Currently the var's data for valid resources are always an empty string. On 2012/03/21 17:36:40, toyoshim wrote: > ditto Done. http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode188 ppapi/cpp/websocket.h:188: /// connection is established, the var contains the empty string. Returns a On 2012/03/21 17:36:40, toyoshim wrote: > ditto Done. http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode203 ppapi/cpp/websocket.h:203: /// connection is established, the var contains the empty string. Returns a On 2012/03/21 17:36:40, toyoshim wrote: > ditto Done.
http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h File ppapi/cpp/websocket.h (right): http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode95 ppapi/cpp/websocket.h:95: /// <code>PP_ERROR_NOACCESS</code> corresponds to InvalidAccessError in the On 2012/03/23 19:31:52, jond wrote: > On 2012/03/21 17:36:40, toyoshim wrote: > > It's my original fault, but I need 'a' before InvalidAccessError here, like > > others? > > Done. I'm pretty sure toyoshim meant you need "an" before "InvalidAccessError". I don't think it's necessary here, but I don't have a strong opinion. http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode135 ppapi/cpp/websocket.h:135: /// <code>PP_ERROR_FAILED</code> corresponds to the JavaScript On 2012/03/23 19:31:52, jond wrote: > On 2012/03/21 17:36:40, toyoshim wrote: > > the -> a ? > > Done. toyoshim was suggesting you change "the" to "a". My first choice is just to omit the article there. I just would prefer we're consistent... if you omit it here, omit it on 95 for "InvalidAccessError". If you use the indefinite article here, use it there, etc. http://codereview.chromium.org/9813011/diff/4001/ppapi/api/ppb_websocket.idl File ppapi/api/ppb_websocket.idl (right): http://codereview.chromium.org/9813011/diff/4001/ppapi/api/ppb_websocket.idl#... ppapi/api/ppb_websocket.idl:271: * corresponds to InvalidAccessError in the WebSocket API specification. If you do add the indefinite article before the errors, shouldn't you also do it in the C interface? I mostly care that we're consistent. (Though I have a slight preference for just leaving articles out in front of the error codes). http://codereview.chromium.org/9813011/diff/4001/ppapi/cpp/websocket.h File ppapi/cpp/websocket.h (right): http://codereview.chromium.org/9813011/diff/4001/ppapi/cpp/websocket.h#newcode56 ppapi/cpp/websocket.h:56: /// the WebSocket API specification. A <code>PP_ERROR_BADARGUMENT</code> I don't think we want an article here (or in the other places you added in this patch). If it said "A value of ...", that would feel correct, but we can also just leave it out (as it was). (I was trying to find grammatical guidance online, but failed.) What do you think? On the other hand, I just don't care that much... it makes sense either way. You can check with another docs person if you want.
http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h File ppapi/cpp/websocket.h (right): http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode95 ppapi/cpp/websocket.h:95: /// <code>PP_ERROR_NOACCESS</code> corresponds to InvalidAccessError in the I'm going to leave it off. On 2012/03/23 20:48:15, dmichael wrote: > On 2012/03/23 19:31:52, jond wrote: > > On 2012/03/21 17:36:40, toyoshim wrote: > > > It's my original fault, but I need 'a' before InvalidAccessError here, like > > > others? > > > > Done. > I'm pretty sure toyoshim meant you need "an" before "InvalidAccessError". I > don't think it's necessary here, but I don't have a strong opinion. http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode135 ppapi/cpp/websocket.h:135: /// <code>PP_ERROR_FAILED</code> corresponds to the JavaScript On 2012/03/23 20:48:15, dmichael wrote: > On 2012/03/23 19:31:52, jond wrote: > > On 2012/03/21 17:36:40, toyoshim wrote: > > > the -> a ? > > > > Done. > toyoshim was suggesting you change "the" to "a". My first choice is just to omit > the article there. I just would prefer we're consistent... if you omit it here, > omit it on 95 for "InvalidAccessError". If you use the indefinite article here, > use it there, etc. Done. http://codereview.chromium.org/9813011/diff/4001/ppapi/api/ppb_websocket.idl File ppapi/api/ppb_websocket.idl (right): http://codereview.chromium.org/9813011/diff/4001/ppapi/api/ppb_websocket.idl#... ppapi/api/ppb_websocket.idl:271: * corresponds to InvalidAccessError in the WebSocket API specification. Yeah, I actually like leave it off. I'm going to do that. On 2012/03/23 20:48:15, dmichael wrote: > If you do add the indefinite article before the errors, shouldn't you also do it > in the C interface? I mostly care that we're consistent. (Though I have a slight > preference for just leaving articles out in front of the error codes). http://codereview.chromium.org/9813011/diff/4001/ppapi/cpp/websocket.h File ppapi/cpp/websocket.h (right): http://codereview.chromium.org/9813011/diff/4001/ppapi/cpp/websocket.h#newcode56 ppapi/cpp/websocket.h:56: /// the WebSocket API specification. A <code>PP_ERROR_BADARGUMENT</code> Actually, looking at it again, I think I should leave it off. On 2012/03/23 20:48:15, dmichael wrote: > I don't think we want an article here (or in the other places you added in this > patch). If it said "A value of ...", that would feel correct, but we can also > just leave it out (as it was). > > (I was trying to find grammatical guidance online, but failed.) > > What do you think? > > On the other hand, I just don't care that much... it makes sense either way. You > can check with another docs person if you want.
lgtm
lgtm with two nits. http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h File ppapi/cpp/websocket.h (right): http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode95 ppapi/cpp/websocket.h:95: /// <code>PP_ERROR_NOACCESS</code> corresponds to InvalidAccessError in the Agreed. Actually, the article is too difficult to me because my mother tongue doesn't have it. So I just check its consistency. I follow your decision. http://codereview.chromium.org/9813011/diff/6003/ppapi/api/ppb_websocket.idl File ppapi/api/ppb_websocket.idl (right): http://codereview.chromium.org/9813011/diff/6003/ppapi/api/ppb_websocket.idl#... ppapi/api/ppb_websocket.idl:271: * corresponds to a InvalidAccessError in the WebSocket API specification. 'an'? http://codereview.chromium.org/9813011/diff/6003/ppapi/cpp/websocket.h File ppapi/cpp/websocket.h (right): http://codereview.chromium.org/9813011/diff/6003/ppapi/cpp/websocket.h#newcode95 ppapi/cpp/websocket.h:95: /// <code>PP_ERROR_NOACCESS</code> corresponds to a InvalidAccessError in the 'an'?
Sorry, please update description before landing. 'Description' must follow format like this http://dev.chromium.org/developers/contributing-code#TOC-GCL
http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h File ppapi/cpp/websocket.h (right): http://codereview.chromium.org/9813011/diff/1/ppapi/cpp/websocket.h#newcode95 ppapi/cpp/websocket.h:95: /// <code>PP_ERROR_NOACCESS</code> corresponds to InvalidAccessError in the On 2012/03/26 08:14:05, toyoshim wrote: > Agreed. > Actually, the article is too difficult to me because my mother tongue doesn't > have it. So I just check its consistency. I follow your decision. Done. http://codereview.chromium.org/9813011/diff/6003/ppapi/api/ppb_websocket.idl File ppapi/api/ppb_websocket.idl (right): http://codereview.chromium.org/9813011/diff/6003/ppapi/api/ppb_websocket.idl#... ppapi/api/ppb_websocket.idl:271: * corresponds to a InvalidAccessError in the WebSocket API specification. On 2012/03/26 08:14:06, toyoshim wrote: > 'an'? Done. http://codereview.chromium.org/9813011/diff/6003/ppapi/cpp/websocket.h File ppapi/cpp/websocket.h (right): http://codereview.chromium.org/9813011/diff/6003/ppapi/cpp/websocket.h#newcode95 ppapi/cpp/websocket.h:95: /// <code>PP_ERROR_NOACCESS</code> corresponds to a InvalidAccessError in the On 2012/03/26 08:14:06, toyoshim wrote: > 'an'? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jond@google.com/9813011/6003
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jond@google.com/9813011/16001
Change committed as 129236 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
