Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
514343002:
XMPP implementation in JavaScript. (Closed)
|
Created:
6 years, 3 months ago by Sergey Ulanov Modified:
6 years, 3 months ago CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionXMPP implementation in JavaScript.
This adds XMPP implementation that will be used for signaling in the
webapp instead of WCS. It depends on TLS support added in the TCP API
in Chrome 38.
BUG=274652
Committed: https://crrev.com/ea32100195dad534bee56d779efde4c085766acc
Cr-Commit-Position: refs/heads/master@{#293068}
Patch Set 1 #
Total comments: 32
Patch Set 2 : #
Total comments: 50
Patch Set 3 : #Messages
Total messages: 12 (1 generated)
sergeyu@chromium.org changed reviewers: + jamiewalch@chromium.org, kelvinp@chromium.org
This new code is not hooked up yet. I'm planning to do it in a separate CL (I have integration working already, but would like to refactor it a bit before submitting for review)
https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/base.js File remoting/webapp/base.js (right): https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:390: * Converts UTF-8 string to ArrayBuffer. Nit: blank line between JSDocs and params annotation. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:403: * Decodes UTF-8 string from ArrayBuffer. same here https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_con... File remoting/webapp/xmpp_connection.js (right): https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_con... remoting/webapp/xmpp_connection.js:26: this.server_ = ''; @private. Same for other private members and functions https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_con... remoting/webapp/xmpp_connection.js:45: * HANDSHAKE -> CONNECTED (connected successfully). Nit: HANDSHAKE -> CONNECTED (Authenticated successfully).? https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_con... remoting/webapp/xmpp_connection.js:93: new remoting.XmppLoginHandler(xmppServer, username, authToken, Instead of binding private functions and handing them off to XmppLoginHandler as callbacks, may be it is cleaner to pass a reference to XmppLoginHandler and make those functions public. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_con... remoting/webapp/xmpp_connection.js:169: remoting.XmppConnection.prototype.tryRead_ = function() { tryRead will cause onRead to call, which will call tryRead again. Is there anyway to guarantee that it will terminate? https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_con... remoting/webapp/xmpp_connection.js:227: base.debug.assert(writeInfo.bytesWritten <= data.byteLength); It is strange that we assert that bytesWritten <= data.byteLength, but then handle the cases when it is not. If it is possible for bytesWritten to be larger than data.byteLength, the assert should be removed. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_con... remoting/webapp/xmpp_connection.js:237: remoting.XmppConnection.prototype.startTls_ = function() { This function is marked as private but it is not called within this class but only called by XMPPLoginHandler. Can we mark it as public? https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... File remoting/webapp/xmpp_login_handler.js (right): https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... remoting/webapp/xmpp_login_handler.js:89: this.streamParser_.appendData(data); Have you consider moving streamParser to XMPPConnection so that XMPPConnection can always hand a complete stanza to the login handler and potentially other handlers in the future? In this way, future handlers won't have to create their own streamParser. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... File remoting/webapp/xmpp_stream_parser.js (right): https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... remoting/webapp/xmpp_stream_parser.js:53: if (this.depth_ <= 1) { base.debug.assert(this.depth_ > 0); https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... remoting/webapp/xmpp_stream_parser.js:149: this.onStanzaCallback_(parsed.childNodes[0]); childNodes also contains all nodes, including text nodes consisting entirely of whitespace (in theory it should not happen as you are removing empty spaces in the code), would it be safer to just use firstElementChild? Same for parseTag_.
My review is still in-progress, but since I'm out tomorrow I wanted to send what I had so far. https://codereview.chromium.org/514343002/diff/1/remoting/webapp/unittests/ba... File remoting/webapp/unittests/base_unittest.js (right): https://codereview.chromium.org/514343002/diff/1/remoting/webapp/unittests/ba... remoting/webapp/unittests/base_unittest.js:250: [0xE6, 0x8C, 0x82, 0xD0, 0x83, 209, 132]); Why not hex for the last two? Also, it might be worth splitting the array to show how the encoding separates; perhaps a double-space or a new line between the characters? https://codereview.chromium.org/514343002/diff/1/remoting/webapp/unittests/ba... remoting/webapp/unittests/base_unittest.js:253: QUnit.deepEqual(toJsArray(base.encodeUtf8("😃")), I always felt we needed more smiley faces in our source code :) https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... File remoting/webapp/xmpp_connection.js (right): https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:11: * Object used to connect to XMPP server. I think it's implicit that it's an object. How about just "A connection to an XMPP server"? https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:26: this.server_ = ''; These should be declared as @private. I'm also a bit surprised they don't need any type information. Has this been run through jscompile? https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:33: this.starttlsPending_ = false; s/tls/Tls/ for consistency with the method name. https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:38: } Semi-colons after functions declared by assignment, please (emacs auto-indent gets confused otherwise). https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:67: 'remoting.XmppConnection.prototype.connect() can only be called once.'); The log message will include the file name, so I think the class is redundant (and if you prune everything before 'connect()' it will fit on one line). Similarly for the other log messages. Also, consider throwing an exception here (or using assert) otherwise the caller has no indication that something went wrong. You could call onError, but that doesn't feel like the right thing to do since it interferes with the previous call to connect(). https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:88: console.error( As above, the caller should have some indication that their request was ignored. Or maybe just queue the message (ie, have sendMessage do the same thing as sendInternal). That would solve the slightly awkward use of both sendInternal_ and doSend_, both of which scream "I'd like to call this method send(), but that's already taken". https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:112: remoting.XmppConnection.prototype.dispose = function() { This might be better named disconnect() for symmetry with connect(), and moved adjacent to it. Alternatively, if you want to follow the base.Disposable pattern, you should annotate this class with @implements {base.Disposable}. https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:117: /** @param {chrome.socket.CreateInfo} createInfo */ @private for this and other methods ending in an underscore. https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:119: // Check if connection was terminated. I think this can only happen if the caller calls dispose (disconnect if you follow my naming advice). If that's the case, this might be clearer as "Check if the connection attempt was cancelled", since "terminated" implies it might have been closed by the remote end. https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:139: return; Braces for single-line ifs for consistency, please. Although don't you need to destroy and unassign this.socket? https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:159: } Why an assert for readPending, but a conditional for the more complex case? Since this is a private method, is it reasonable to assert that it should not be called other than in HANDSHAKE or CONNECTED states, and never with startTlsPending? https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:176: this.loginHandler_.onDataReceived(readInfo.data); The name of this class implies that it just handles the initial login handshake, but it appears that it also handles passing stanzas through to the calling code, is that correct? If so, then I think the naming could be improved. Perhaps call this class XmppConnector and the other something like XmppProtocol. Or the XmppLoginHandler could be refactored to only handle the login part of the protocol, and signal back to this class when the connection is authenticated and ready to exchange client stanzas (which I believe it already does via onHandshakeDone). https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:203: console.error('TCP write failed with error ' + writeInfo.bytesWritten); You should call onError here so that this.error is set correctly. https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:211: base.debug.assert(writeInfo.bytesWritten <= data.byteLength); What is this assert guarding against? You're already guarding against the API returning a value that's too large because you use >= below, and if it returns some other bogus value, you can't detect it anyway. I would be inclined to remove the assert. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... File remoting/webapp/xmpp_login_handler.js (right): https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... remoting/webapp/xmpp_login_handler.js:13: * to send outgoing messages and Incomplete comment. I think an overview of the protocol exchange would be useful here (or just a link if it's documented on-line somewhere). https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... remoting/webapp/xmpp_login_handler.js:18: * @param {function(string):void} sendMessageCallback Callback call to send s/call/to call/ https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... remoting/webapp/xmpp_login_handler.js:21: * the underlying socket. Why does this need to be a callback? There's only only implementation, and it seems like this class would be simpler if it handled the details itself. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... remoting/webapp/xmpp_login_handler.js:38: this.server_ = server; @private https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... remoting/webapp/xmpp_login_handler.js:133: '<auth xmlns="urn:ietf:params:xml:ns:xmpp-sasl" ' + When constructing XML hierarchies from string literals, I think it's better to indent as if it was an XML file, ie, something like: '<auth xmlns="urn:ietf:params:xml:ns:xmpp-sasl" ' + 'mechanism="X-OAUTH2" auth:service="oauth2" ' + ... 'xmlns:auth="http://www.google.com/talk/protocol/auth">' + cookie + '</auth>' It's not strictly style-guide conformant, but I think it adheres to the spirit and is more readable. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... remoting/webapp/xmpp_login_handler.js:162: 'Server doesn\'t support bind after authentication.'); You can use double-quotes if the string has an apostrophe. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... remoting/webapp/xmpp_login_handler.js:168: if (!jidElement) This is indented badly, and very confusing without the braces for the single-statement conditional. I don't think it's what you intended either, since you're checking jidElement twice. I think maybe you just want to get rid of this line. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... File remoting/webapp/xmpp_stream_parser.js (right): https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... remoting/webapp/xmpp_stream_parser.js:13: * onStartCallback is called once for the stream header and after that There is no onStartCallback. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... remoting/webapp/xmpp_stream_parser.js:25: /** @type {Array.<ArrayBuffer>} */ Please add descriptions of these members. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... remoting/webapp/xmpp_stream_parser.js:58: this.extractToken_(1); This doesn't look right. It seems to be testing whether or not the first character of the first buffer is a space, and if so extracting everything up to the first character of the *last* buffer. If the assumption is that there is only one buffer in this case, you should assert that. Could this whole tryAgain loop be replaced with a loop that strips leading whitespace, or is it important to error out if there's more than one leading space? https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... remoting/webapp/xmpp_stream_parser.js:62: base.decodeUtf8(this.data_[0])); Your earlier comment stated that this might be incomplete UTF-8. We should add a unit test for decodeUtf8 to ensure that it does something sensible in this case (decoding only complete characters, for example). https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... remoting/webapp/xmpp_stream_parser.js:67: var view = new DataView(this.data_[this.data_.length - 1]); It's not obvious what this second loop is doing. I think a comment would be helpful. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... remoting/webapp/xmpp_stream_parser.js:166: remoting.XmppStreamParser.prototype.extractToken_ = function(endPos) { What is a "token" in this context? This seems to be extracting bytes up to a certain limit and then returning the corresponding string representation; there's no tokenization going on, so I think the name is misleading. I suggest calling it something like extractStringUpTo. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... remoting/webapp/xmpp_stream_parser.js:186: var dataLeft = this.data_[this.data_.length - 1].slice(endPos); I suggest calling this dataRemaining, since it's arguably at the right-most end of the array, not the left :)
https://codereview.chromium.org/514343002/diff/1/remoting/webapp/unittests/ba... File remoting/webapp/unittests/base_unittest.js (right): https://codereview.chromium.org/514343002/diff/1/remoting/webapp/unittests/ba... remoting/webapp/unittests/base_unittest.js:250: [0xE6, 0x8C, 0x82, 0xD0, 0x83, 209, 132]); On 2014/08/29 02:14:08, Jamie wrote: > Why not hex for the last two? Also, it might be worth splitting the array to > show how the encoding separates; perhaps a double-space or a new line between > the characters? Done. https://codereview.chromium.org/514343002/diff/1/remoting/webapp/unittests/ba... remoting/webapp/unittests/base_unittest.js:253: QUnit.deepEqual(toJsArray(base.encodeUtf8("😃")), On 2014/08/29 02:14:08, Jamie wrote: > I always felt we needed more smiley faces in our source code :) Acknowledged. https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... File remoting/webapp/xmpp_connection.js (right): https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:11: * Object used to connect to XMPP server. On 2014/08/29 02:14:08, Jamie wrote: > I think it's implicit that it's an object. How about just "A connection to an > XMPP server"? Done. https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:26: this.server_ = ''; On 2014/08/29 02:14:08, Jamie wrote: > These should be declared as @private. I'm also a bit surprised they don't need > any type information. Has this been run through jscompile? Yes. jscompiler can infer type from assignment here. https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:33: this.starttlsPending_ = false; On 2014/08/29 02:14:08, Jamie wrote: > s/tls/Tls/ for consistency with the method name. Done. https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:38: } On 2014/08/29 02:14:09, Jamie wrote: > Semi-colons after functions declared by assignment, please (emacs auto-indent > gets confused otherwise). Done. Can emacs be integrated with clang-format for formatting? I think it can, see https://code.google.com/p/chromium/wiki/ClangFormat . clang-format works fine without semicolon here. https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:67: 'remoting.XmppConnection.prototype.connect() can only be called once.'); On 2014/08/29 02:14:08, Jamie wrote: > The log message will include the file name, so I think the class is redundant > (and if you prune everything before 'connect()' it will fit on one line). > Similarly for the other log messages. > > Also, consider throwing an exception here (or using assert) otherwise the caller > has no indication that something went wrong. You could call onError, but that > doesn't feel like the right thing to do since it interferes with the previous > call to connect(). Replaced with assert() https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:88: console.error( On 2014/08/29 02:14:08, Jamie wrote: > As above, the caller should have some indication that their request was ignored. > Or maybe just queue the message (ie, have sendMessage do the same thing as > sendInternal). That would solve the slightly awkward use of both sendInternal_ > and doSend_, both of which scream "I'd like to call this method send(), but > that's already taken". Done. https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:112: remoting.XmppConnection.prototype.dispose = function() { On 2014/08/29 02:14:09, Jamie wrote: > This might be better named disconnect() for symmetry with connect(), and moved > adjacent to it. Alternatively, if you want to follow the base.Disposable > pattern, you should annotate this class with @implements {base.Disposable}. Yes, the intent here is to follow the Disposable pattern. Added @implements. https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:117: /** @param {chrome.socket.CreateInfo} createInfo */ On 2014/08/29 02:14:08, Jamie wrote: > @private for this and other methods ending in an underscore. Done. https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:119: // Check if connection was terminated. On 2014/08/29 02:14:08, Jamie wrote: > I think this can only happen if the caller calls dispose (disconnect if you > follow my naming advice). If that's the case, this might be clearer as "Check if > the connection attempt was cancelled", since "terminated" implies it might have > been closed by the remote end. Replaced with "Check if connection was destroyed" https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:139: return; On 2014/08/29 02:14:09, Jamie wrote: > Braces for single-line ifs for consistency, please. Although don't you need to > destroy and unassign this.socket? No. It should have been destroyed when dispose() was called. https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:159: } On 2014/08/29 02:14:08, Jamie wrote: > Why an assert for readPending, but a conditional for the more complex case? > Since this is a private method, is it reasonable to assert that it should not be > called other than in HANDSHAKE or CONNECTED states, and never with > startTlsPending? Done. https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:176: this.loginHandler_.onDataReceived(readInfo.data); On 2014/08/29 02:14:09, Jamie wrote: > The name of this class implies that it just handles the initial login handshake, > but it appears that it also handles passing stanzas through to the calling code, > is that correct? If so, then I think the naming could be improved. Perhaps call > this class XmppConnector and the other something like XmppProtocol. Or the > XmppLoginHandler could be refactored to only handle the login part of the > protocol, and signal back to this class when the connection is authenticated and > ready to exchange client stanzas (which I believe it already does via > onHandshakeDone). Refactored this code so that LoginHandler is used only during HANDSHAKE face and then it passes the stream parser in the HandshakeDone callback to the XmppConnection https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:203: console.error('TCP write failed with error ' + writeInfo.bytesWritten); On 2014/08/29 02:14:08, Jamie wrote: > You should call onError here so that this.error is set correctly. Done. https://codereview.chromium.org/514343002/diff/1/remoting/webapp/xmpp_connect... remoting/webapp/xmpp_connection.js:211: base.debug.assert(writeInfo.bytesWritten <= data.byteLength); On 2014/08/29 02:14:09, Jamie wrote: > What is this assert guarding against? You're already guarding against the API > returning a value that's too large because you use >= below, and if it returns > some other bogus value, you can't detect it anyway. I would be inclined to > remove the assert. Its to detect bugs that cause head of sendQueue_ to be changed while write() is pending. I'm not trying to detect bugs in the API implementation. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/base.js File remoting/webapp/base.js (right): https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:390: * Converts UTF-8 string to ArrayBuffer. On 2014/08/29 01:37:09, kelvinp wrote: > Nit: blank line between JSDocs and params annotation. Done. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:403: * Decodes UTF-8 string from ArrayBuffer. On 2014/08/29 01:37:09, kelvinp wrote: > same here Done. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_con... File remoting/webapp/xmpp_connection.js (right): https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_con... remoting/webapp/xmpp_connection.js:26: this.server_ = ''; On 2014/08/29 01:37:09, kelvinp wrote: > @private. Same for other private members and functions Done. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_con... remoting/webapp/xmpp_connection.js:45: * HANDSHAKE -> CONNECTED (connected successfully). On 2014/08/29 01:37:09, kelvinp wrote: > Nit: HANDSHAKE -> CONNECTED (Authenticated successfully).? Done. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_con... remoting/webapp/xmpp_connection.js:93: new remoting.XmppLoginHandler(xmppServer, username, authToken, On 2014/08/29 01:37:10, kelvinp wrote: > Instead of binding private functions and handing them off to XmppLoginHandler as > callbacks, may be it is cleaner to pass a reference to XmppLoginHandler and make > those functions public. This would create circular dependency between XmppConnection and XmppLoginHandler, which is wrong and would make it harder to test XmppLoginHandler. It would also require exposing internal details of XmppConnection as public. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_con... remoting/webapp/xmpp_connection.js:169: remoting.XmppConnection.prototype.tryRead_ = function() { On 2014/08/29 01:37:10, kelvinp wrote: > tryRead will cause onRead to call, which will call tryRead again. Is there > anyway to guarantee that it will terminate? chrome.socket.read() never calls the callback synchronously. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_con... remoting/webapp/xmpp_connection.js:227: base.debug.assert(writeInfo.bytesWritten <= data.byteLength); On 2014/08/29 01:37:10, kelvinp wrote: > It is strange that we assert that bytesWritten <= data.byteLength, but then > handle the cases when it is not. > If it is possible for bytesWritten to be larger than data.byteLength, the assert > should be removed. replaced >= with == in the line below. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_con... remoting/webapp/xmpp_connection.js:237: remoting.XmppConnection.prototype.startTls_ = function() { On 2014/08/29 01:37:10, kelvinp wrote: > This function is marked as private but it is not called within this class but > only called by XMPPLoginHandler. Can we mark it as public? No. It's internal detail of XmppConnection() and is not supposed to be called by the client of this class. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... File remoting/webapp/xmpp_login_handler.js (right): https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... remoting/webapp/xmpp_login_handler.js:13: * to send outgoing messages and On 2014/08/29 02:14:09, Jamie wrote: > Incomplete comment. > > I think an overview of the protocol exchange would be useful here (or just a > link if it's documented on-line somewhere). Added reference to RFC and State description lists what happens in each state. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... remoting/webapp/xmpp_login_handler.js:18: * @param {function(string):void} sendMessageCallback Callback call to send On 2014/08/29 02:14:09, Jamie wrote: > s/call/to call/ Done. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... remoting/webapp/xmpp_login_handler.js:21: * the underlying socket. On 2014/08/29 02:14:09, Jamie wrote: > Why does this need to be a callback? There's only only implementation, and it > seems like this class would be simpler if it handled the details itself. This class doesn't know anything about the socket. If it was to call chrome.socket.secure() then it would depend on the socket API, which means that - It would need to know socketId. - It would be harder to replace chrome.socket with chrome.sockets.tcp or any other socket API. - It would be harder to test it due to the extra dependencies. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... remoting/webapp/xmpp_login_handler.js:38: this.server_ = server; On 2014/08/29 02:14:09, Jamie wrote: > @private Done. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... remoting/webapp/xmpp_login_handler.js:89: this.streamParser_.appendData(data); On 2014/08/29 01:37:10, kelvinp wrote: > Have you consider moving streamParser to XMPPConnection so that XMPPConnection > can always hand a complete stanza to the login handler and potentially other > handlers in the future? In this way, future handlers won't have to create their > own streamParser. The issue here is that streamParser needs to be reset (by creating a new instance) several times during handshake (e.g. after starting TLS), for that reason LoginHandler needs to own it during handshake. I've refactored interaction between LoginHandler and XmppConnection to pass ownership of the parser to XmppConnection once handshake is finished. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... remoting/webapp/xmpp_login_handler.js:133: '<auth xmlns="urn:ietf:params:xml:ns:xmpp-sasl" ' + On 2014/08/29 02:14:09, Jamie wrote: > When constructing XML hierarchies from string literals, I think it's better to > indent as if it was an XML file, ie, something like: > > '<auth xmlns="urn:ietf:params:xml:ns:xmpp-sasl" ' + > 'mechanism="X-OAUTH2" auth:service="oauth2" ' + > ... > 'xmlns:auth="http://www.google.com/talk/protocol/auth">' + > cookie + > '</auth>' > > It's not strictly style-guide conformant, but I think it adheres to the spirit > and is more readable. Done. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... remoting/webapp/xmpp_login_handler.js:162: 'Server doesn\'t support bind after authentication.'); On 2014/08/29 02:14:09, Jamie wrote: > You can use double-quotes if the string has an apostrophe. Done. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_log... remoting/webapp/xmpp_login_handler.js:168: if (!jidElement) Actually this |if| doesn't even need to be here. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... File remoting/webapp/xmpp_stream_parser.js (right): https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... remoting/webapp/xmpp_stream_parser.js:13: * onStartCallback is called once for the stream header and after that On 2014/08/29 02:14:09, Jamie wrote: > There is no onStartCallback. Done. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... remoting/webapp/xmpp_stream_parser.js:25: /** @type {Array.<ArrayBuffer>} */ On 2014/08/29 02:14:09, Jamie wrote: > Please add descriptions of these members. Done. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... remoting/webapp/xmpp_stream_parser.js:53: if (this.depth_ <= 1) { On 2014/08/29 01:37:10, kelvinp wrote: > base.debug.assert(this.depth_ > 0); depth_ can be 0 here. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... remoting/webapp/xmpp_stream_parser.js:58: this.extractToken_(1); On 2014/08/29 02:14:09, Jamie wrote: > This doesn't look right. It seems to be testing whether or not the first > character of the first buffer is a space, and if so extracting everything up to > the first character of the *last* buffer. If the assumption is that there is > only one buffer in this case, you should assert that. That's for catching this. I've simplified this code now to use a single ArrayBuffer for pending data. That was useless optimization. > > Could this whole tryAgain loop be replaced with a loop that strips leading > whitespace, or is it important to error out if there's more than one leading > space? The while loop is necessary to handle the case when we receive a new packet with multiple XMPP tokens. The inner loop finds end of the first tag and extracts it from |data_|. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... remoting/webapp/xmpp_stream_parser.js:62: base.decodeUtf8(this.data_[0])); On 2014/08/29 02:14:10, Jamie wrote: > Your earlier comment stated that this might be incomplete UTF-8. We should add a > unit test for decodeUtf8 to ensure that it does something sensible in this case > (decoding only complete characters, for example). Good point. decodeUtf8() throws in that case. I changed this code to handle this case properly. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... remoting/webapp/xmpp_stream_parser.js:67: var view = new DataView(this.data_[this.data_.length - 1]); On 2014/08/29 02:14:10, Jamie wrote: > It's not obvious what this second loop is doing. I think a comment would be > helpful. Done. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... remoting/webapp/xmpp_stream_parser.js:149: this.onStanzaCallback_(parsed.childNodes[0]); On 2014/08/29 01:37:10, kelvinp wrote: > childNodes also contains all nodes, including text nodes consisting entirely of > whitespace (in theory it should not happen as you are removing empty spaces in > the code), would it be safer to just use firstElementChild? Same for parseTag_. Done. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... remoting/webapp/xmpp_stream_parser.js:166: remoting.XmppStreamParser.prototype.extractToken_ = function(endPos) { On 2014/08/29 02:14:09, Jamie wrote: > What is a "token" in this context? This seems to be extracting bytes up to a > certain limit and then returning the corresponding string representation; > there's no tokenization going on, so I think the name is misleading. I suggest > calling it something like extractStringUpTo. Renamed extractStringFromBuffer_. https://codereview.chromium.org/514343002/diff/20001/remoting/webapp/xmpp_str... remoting/webapp/xmpp_stream_parser.js:186: var dataLeft = this.data_[this.data_.length - 1].slice(endPos); On 2014/08/29 02:14:10, Jamie wrote: > I suggest calling this dataRemaining, since it's arguably at the right-most end > of the array, not the left :) Done, though some people would say that arrays start on the right and end on the left :)
Thank you for making the changes. Looks mostly good. Still would like to talk to you in person to understand why the parser cannot be moved into the connection class. It seems like the interface would be cleaner if there is a higher level XMPPSocket that operates in terms of stanza instead of raw bytes from the socket. Why does the parser needs to be reset upon TLS starting?
lgtm
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/514343002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_gpu_retina_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...) mac_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as ae155e0340c94a0642f26d069e2d9555f4ae7a6a
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ea32100195dad534bee56d779efde4c085766acc Cr-Commit-Position: refs/heads/master@{#293068} |
