Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2257963002:
DevTools: Better view of web sockets binary frames' data
|
Created:
4 years, 4 months ago by talkain Modified:
3 years, 5 months ago CC:
paulirish, chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Better view of web sockets binary frames' data
Debugging web sockets that send and receive binary frames is hard,
mostly because the information that was presented about binary frames is
"Binary Frame (Opcode 2, mask)", regardless of the frame's content.
This patch exports the frame's data in a python-like way, printing the
printable characters and presenting all other bytes using the "\xXX" format.
BUG=
Signed-off-by: Tal Kain <tal@kain.net>;
Patch Set 1 #
Total comments: 4
Patch Set 2 : DevTools: Better view of web sockets binary frames' data #
Total comments: 10
Patch Set 3 : DevTools: Better view of web sockets binary frames' data #
Total comments: 6
Patch Set 4 : DevTools: Better view of web sockets binary frames' data #
Total comments: 6
Messages
Total messages: 23 (5 generated)
Description was changed from ========== DevTools: Better view of web sockets binary frames' data Debugging web sockets that send and receive binary frames is hard, mostly because the information that was presented about binary frames is "Binary Frame (Opcode 2, mask)", regardless of the frame's content. This patch exports the frame's data in a python-like way, printing the printable characters and presenting all other bytes using the "\xXX" format. BUG= Signed-off-by: Tal Kain <tal@kain.net> ========== to ========== DevTools: Better view of web sockets binary frames' data Debugging web sockets that send and receive binary frames is hard, mostly because the information that was presented about binary frames is "Binary Frame (Opcode 2, mask)", regardless of the frame's content. This patch exports the frame's data in a python-like way, printing the printable characters and presenting all other bytes using the "\xXX" format. BUG= Signed-off-by: Tal Kain <tal@kain.net> ==========
tal@kain.net changed reviewers: + pfeldman@chromium.org, sergeyv@chromium.org
dgozman@chromium.org changed reviewers: + allada@chromium.org, dgozman@chromium.org - pfeldman@chromium.org, sergeyv@chromium.org
Blaise, could you please take a look? https://codereview.chromium.org/2257963002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js (right): https://codereview.chromium.org/2257963002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:216: for (var i = 0; i < frame.text.length; i++) { Let's limit the number of characters with 5000 from the start and 5000 from the end?
I see this is your first patch... Please do not get discouraged by our code review process, we really do appreciate your contribution, but Chrome has a very high code standard and code review is quite extensive. Thanks a lot! https://codereview.chromium.org/2257963002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js (right): https://codereview.chromium.org/2257963002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:213: if (frame.opCode === WebInspector.ResourceWebSocketFrameView.OpCodes.BinaryFrame) { Lets flip this around and early return instead of nesting (but please make sure that ping, pong, continuation and close frames all work properly), eg: if (this._isTextFrame) { this._dataText = WebInspector.ResourceWebSocketFrameView.opCodeDescription(frame.opCode, frame.mask); return; } // Handle the binary frame data here. https://codereview.chromium.org/2257963002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:216: for (var i = 0; i < frame.text.length; i++) { Can we instead do a regular expression + function to handle this? (but also do what dgozman@ suggested and omit the middle characters when it's over 5000 and replace with "…" (\u2026)). regex example: this._dataText = frame.text.replace(/[^\x20-\x7E\n\r]/g, escapeNonPrintableCharacters); function escapeNonPrintableCharacters(match) { var charCode = match.charCodeAt(0); if (charCode <= 0xF) return "\\x0" + charCode.toString(16); return "\\x" + charCode.toString(16); } https://codereview.chromium.org/2257963002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:218: // Checks if the char code is printable or not We generally do not put comments in our code unless it's extremely hard to read the code or there's a gotcha.
On 2016/08/22 16:50:32, Blaise wrote: > I see this is your first patch... Please do not get discouraged by our code > review process, we really do appreciate your contribution, but Chrome has a very > high code standard and code review is quite extensive. > > Thanks a lot! Hey Blaise, First I would like to thank you for the quick response. I have to say this is probably the most politest code review I had till now, thank you for that. > > https://codereview.chromium.org/2257963002/diff/1/third_party/WebKit/Source/d... > File > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js > (right): > > https://codereview.chromium.org/2257963002/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:213: > if (frame.opCode === > WebInspector.ResourceWebSocketFrameView.OpCodes.BinaryFrame) { > Lets flip this around and early return instead of nesting (but please make sure > that ping, pong, continuation and close frames all work properly), eg: > > if (this._isTextFrame) { > this._dataText = > WebInspector.ResourceWebSocketFrameView.opCodeDescription(frame.opCode, > frame.mask); > return; > } > > // Handle the binary frame data here. No problem, will change it. > > https://codereview.chromium.org/2257963002/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:216: > for (var i = 0; i < frame.text.length; i++) { > Can we instead do a regular expression + function to handle this? (but also do At first I thought regex can be a good solution, but I wasn't sure performance-wise that using a regex for every binary frame will be a good idea. I preferred the "simple" solution, but if you insist on using regex I will change it. > what dgozman@ suggested and omit the middle characters when it's over 5000 and > replace with "…" (\u2026)). Sounds legit, but I suggest tagging it in a way that the user will be able to differentiate between "..." and the actual data of "...". For example: adding some prefix, liked "omitted" or something similar to that. What do you think? > > regex example: > > this._dataText = frame.text.replace(/[^\x20-\x7E\n\r]/g, > escapeNonPrintableCharacters); > > function escapeNonPrintableCharacters(match) > { > var charCode = match.charCodeAt(0); > if (charCode <= 0xF) > return "\\x0" + charCode.toString(16); > return "\\x" + charCode.toString(16); > } > > https://codereview.chromium.org/2257963002/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:218: > // Checks if the char code is printable or not > We generally do not put comments in our code unless it's extremely hard to read > the code or there's a gotcha. No problem, will remove it. Thanks!
On 2016/08/22 23:11:12, talkain wrote: > On 2016/08/22 16:50:32, Blaise wrote: > > I see this is your first patch... Please do not get discouraged by our code > > review process, we really do appreciate your contribution, but Chrome has a > very > > high code standard and code review is quite extensive. > > > > Thanks a lot! > > Hey Blaise, > First I would like to thank you for the quick response. > I have to say this is probably the most politest code review I had till now, > thank you for that. > > > > > > https://codereview.chromium.org/2257963002/diff/1/third_party/WebKit/Source/d... > > File > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js > > (right): > > > > > https://codereview.chromium.org/2257963002/diff/1/third_party/WebKit/Source/d... > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:213: > > if (frame.opCode === > > WebInspector.ResourceWebSocketFrameView.OpCodes.BinaryFrame) { > > Lets flip this around and early return instead of nesting (but please make > sure > > that ping, pong, continuation and close frames all work properly), eg: > > > > if (this._isTextFrame) { > > this._dataText = > > WebInspector.ResourceWebSocketFrameView.opCodeDescription(frame.opCode, > > frame.mask); > > return; > > } > > > > // Handle the binary frame data here. > > No problem, will change it. > > > > > > https://codereview.chromium.org/2257963002/diff/1/third_party/WebKit/Source/d... > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:216: > > for (var i = 0; i < frame.text.length; i++) { > > Can we instead do a regular expression + function to handle this? (but also do > > At first I thought regex can be a good solution, but I wasn't sure > performance-wise that using a regex for every > binary frame will be a good idea. > I preferred the "simple" solution, but if you insist on using regex I will > change it. > > > what dgozman@ suggested and omit the middle characters when it's over 5000 and > > replace with "…" (\u2026)). > > Sounds legit, but I suggest tagging it in a way that the user will be able to > differentiate between "..." and the actual data of "...". > For example: adding some prefix, liked "omitted" or something similar to that. > What do you think? > > > > > regex example: > > > > this._dataText = frame.text.replace(/[^\x20-\x7E\n\r]/g, > > escapeNonPrintableCharacters); > > > > function escapeNonPrintableCharacters(match) > > { > > var charCode = match.charCodeAt(0); > > if (charCode <= 0xF) > > return "\\x0" + charCode.toString(16); > > return "\\x" + charCode.toString(16); > > } > > > > > https://codereview.chromium.org/2257963002/diff/1/third_party/WebKit/Source/d... > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:218: > > // Checks if the char code is printable or not > > We generally do not put comments in our code unless it's extremely hard to > read > > the code or there's a gotcha. > > No problem, will remove it. > > Thanks! I spoke to paulirish@ about it and we agreed that because of some other views being constructed right now, lets not display anything over 5000. So instead of doing the "..." thing just put a "cannot display data" error view.
Hi Blaise and dgozman,
As you requested, I changed the patch according to your review.
Note that I wasn't able to change the code to return instead of the nesting
without making a code duplication (since the line that comes after the change is
relevant to all frame's types).
WebInspector.SortableDataGridNode.call(this, {data: this._dataText, length:
length, time: timeNode});
Moreover, to make the code a little bit clearer I refactored the change and
exported a function for the binary frame handling process.
Thanks in advance,
Tal
Thanks! https://codereview.chromium.org/2257963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js (right): https://codereview.chromium.org/2257963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:214: if (frame.opCode === WebInspector.ResourceWebSocketFrameView.OpCodes.BinaryFrame) { nit: Our code style is, if an if or for statement has only one item in it we do not wrap them in {} however we do wrap if there are ever more than one line in them. Because the opCodeDescription is fast, I think having it generate the _dataText always for binary frames is fine. https://codereview.chromium.org/2257963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:228: escapeNonPrintableCharacters: function(match) { lets embed this into handleBinaryFrameView. Google follows the YAGNI, so we try not to expose much to keep other people that come and use the code from using it then having it get changed and then have it break areas that decided to start using it. https://codereview.chromium.org/2257963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:230: if (charCode <= 0xF) Lets also use a ternary here. eg: return "\\x" + (charCode <= 0xF ? "0" : "") + charCode.toString(16); https://codereview.chromium.org/2257963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:240: handleBinaryFrameView: function(frame) { lets make this private (append "_") and rename to something like "_escapeBinaryFrameData()". I am almost tempted to stay we should inline this, but I'll let the final reviewer decide that. https://codereview.chromium.org/2257963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:242: this._dataText = "cannot display data"; All user visible strings are wrapped in WebInspector.UIString(). In this case we should just return because this._dataText will already be set and tell the user it's a binary frame.
https://codereview.chromium.org/2257963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js (right): https://codereview.chromium.org/2257963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:214: if (frame.opCode === WebInspector.ResourceWebSocketFrameView.OpCodes.BinaryFrame) { On 2016/08/29 17:38:02, Blaise wrote: > nit: Our code style is, if an if or for statement has only one item in it we do > not wrap them in {} however we do wrap if there are ever more than one line in > them. Because the opCodeDescription is fast, I think having it generate the > _dataText always for binary frames is fine. Acknowledged. https://codereview.chromium.org/2257963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:228: escapeNonPrintableCharacters: function(match) { On 2016/08/29 17:38:02, Blaise wrote: > lets embed this into handleBinaryFrameView. > > Google follows the YAGNI, so we try not to expose much to keep other people that > come and use the code from using it then having it get changed and then have it > break areas that decided to start using it. Acknowledged. https://codereview.chromium.org/2257963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:230: if (charCode <= 0xF) On 2016/08/29 17:38:02, Blaise wrote: > Lets also use a ternary here. > > eg: return "\\x" + (charCode <= 0xF ? "0" : "") + charCode.toString(16); Acknowledged. https://codereview.chromium.org/2257963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:240: handleBinaryFrameView: function(frame) { On 2016/08/29 17:38:02, Blaise wrote: > lets make this private (append "_") and rename to something like > "_escapeBinaryFrameData()". > > I am almost tempted to stay we should inline this, but I'll let the final > reviewer decide that. Acknowledged. https://codereview.chromium.org/2257963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:242: this._dataText = "cannot display data"; On 2016/08/29 17:38:02, Blaise wrote: > All user visible strings are wrapped in WebInspector.UIString(). > > In this case we should just return because this._dataText will already be set > and tell the user it's a binary frame. Acknowledged.
Overall looks good! https://codereview.chromium.org/2257963002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js (right): https://codereview.chromium.org/2257963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:224: * Handles binary frames by escaping the non printable characters. We do not add comments/documents to functions. Our philosophy is that function names should be named in a way that is self explanatory. https://codereview.chromium.org/2257963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:230: this._dataText = "cannot display data"; Lets remove this, _dataText should already be set from line 213. https://codereview.chromium.org/2257963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:235: (function(match) { We no longer use anonymous functions unless it's a single line ES6 arrow function. We name the function inside the method then pass the name of the function into the callback. Example on how we do it: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...
https://codereview.chromium.org/2257963002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js (right): https://codereview.chromium.org/2257963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:224: * Handles binary frames by escaping the non printable characters. On 2016/08/30 16:40:30, Blaise wrote: > We do not add comments/documents to functions. Our philosophy is that function > names should be named in a way that is self explanatory. Acknowledged. https://codereview.chromium.org/2257963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:230: this._dataText = "cannot display data"; On 2016/08/30 16:40:29, Blaise wrote: > Lets remove this, _dataText should already be set from line 213. Acknowledged. https://codereview.chromium.org/2257963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:235: (function(match) { On 2016/08/30 16:40:29, Blaise wrote: > We no longer use anonymous functions unless it's a single line ES6 arrow > function. We name the function inside the method then pass the name of the > function into the callback. > > Example on how we do it: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... Acknowledged.
allada@chromium.org changed reviewers: + chowse@chromium.org
Looking good! Can we add a screenshot link (we usually just use imgur) to see how it'd look? Bringing in +chowse to review the UI change. Thanks! https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js (right): https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:213: this._dataText = WebInspector.ResourceWebSocketFrameView.opCodeDescription(frame.opCode, frame.mask); Since we are not using this._dataText as our source for the _escapeBinaryFrameData function, I think we should do this as an else of the binary's if statement, so we don't need to call/set it for no reason. https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:223: _escapeBinaryFrameData: function(frame) { I'm sorry I should have clarified a little more... We do add JSDoc, but we don't usually ad comments/notes. In this case it should look like this: /** * @param {!WebInspector.NetworkRequest.WebSocketFrame} frame */ https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:223: _escapeBinaryFrameData: function(frame) { On second thought, lets inline this function. It's only used in this one case and not large enough to merit it's own function. https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:224: if (frame.text.length > 5000) { Single line if statements we do not add curly brackets around it. ie: if (frame.text.length > 5000) return; https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:228: function escapeChar(match) { We need to add docs: /** * @param {string} match * @return {string} */ https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:233: this._dataText = frame.text.replace(/[^\x20-\x7E\n\r]/g, escapeChar); Can we put this above the above function? We often take advantage of class hoisting. (and it puts all the actual function code together)
Thank you. Sure,here are two samples of the change: http://i.imgur.com/fDb73kT.png http://i.imgur.com/YTujo48.png One is a binary frames only example and the other one is an example with mixed text frames. - Tal On 2016/10/20 00:06:48, Blaise wrote: > Looking good! Can we add a screenshot link (we usually just use imgur) to see > how it'd look? > > Bringing in +chowse to review the UI change. > > Thanks! > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js > (right): > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:213: > this._dataText = > WebInspector.ResourceWebSocketFrameView.opCodeDescription(frame.opCode, > frame.mask); > Since we are not using this._dataText as our source for the > _escapeBinaryFrameData function, I think we should do this as an else of the > binary's if statement, so we don't need to call/set it for no reason. > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:223: > _escapeBinaryFrameData: function(frame) { > I'm sorry I should have clarified a little more... We do add JSDoc, but we don't > usually ad comments/notes. > > In this case it should look like this: > /** > * @param {!WebInspector.NetworkRequest.WebSocketFrame} frame > */ > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:223: > _escapeBinaryFrameData: function(frame) { > On second thought, lets inline this function. It's only used in this one case > and not large enough to merit it's own function. > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:224: > if (frame.text.length > 5000) { > Single line if statements we do not add curly brackets around it. > > ie: > if (frame.text.length > 5000) > return; > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:228: > function escapeChar(match) { > We need to add docs: > /** > * @param {string} match > * @return {string} > */ > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:233: > this._dataText = frame.text.replace(/[^\x20-\x7E\n\r]/g, escapeChar); > Can we put this above the above function? We often take advantage of class > hoisting. (and it puts all the actual function code together)
Thanks for the screenshots. Tal/Blaise: Could you provide a screenshot of how the WebSocket appeared before this change? Is the change limited to using the \xHH hex codes in place of binary characters (SGTM)? Are the yellow/green colors new (seem a bit loud)? On 2016/10/20 08:43:09, talkain wrote: > Thank you. > > Sure,here are two samples of the change: > http://i.imgur.com/fDb73kT.png > http://i.imgur.com/YTujo48.png > > One is a binary frames only example and the other one is an example with mixed > text frames. > > - Tal > > On 2016/10/20 00:06:48, Blaise wrote: > > Looking good! Can we add a screenshot link (we usually just use imgur) to see > > how it'd look? > > > > Bringing in +chowse to review the UI change. > > > > Thanks! > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > File > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js > > (right): > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:213: > > this._dataText = > > WebInspector.ResourceWebSocketFrameView.opCodeDescription(frame.opCode, > > frame.mask); > > Since we are not using this._dataText as our source for the > > _escapeBinaryFrameData function, I think we should do this as an else of the > > binary's if statement, so we don't need to call/set it for no reason. > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:223: > > _escapeBinaryFrameData: function(frame) { > > I'm sorry I should have clarified a little more... We do add JSDoc, but we > don't > > usually ad comments/notes. > > > > In this case it should look like this: > > /** > > * @param {!WebInspector.NetworkRequest.WebSocketFrame} frame > > */ > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:223: > > _escapeBinaryFrameData: function(frame) { > > On second thought, lets inline this function. It's only used in this one case > > and not large enough to merit it's own function. > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:224: > > if (frame.text.length > 5000) { > > Single line if statements we do not add curly brackets around it. > > > > ie: > > if (frame.text.length > 5000) > > return; > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:228: > > function escapeChar(match) { > > We need to add docs: > > /** > > * @param {string} match > > * @return {string} > > */ > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:233: > > this._dataText = frame.text.replace(/[^\x20-\x7E\n\r]/g, escapeChar); > > Can we put this above the above function? We often take advantage of class > > hoisting. (and it puts all the actual function code together)
Hi Chowse, Here is a screenshot of the inspector before the change: http://i.imgur.com/oDr2bgR.png The colors aren't new, just the binary frames' content. On 2016/10/20 18:56:22, chowse wrote: > Thanks for the screenshots. Tal/Blaise: Could you provide a screenshot of how > the WebSocket appeared before this change? Is the change limited to using the > \xHH hex codes in place of binary characters (SGTM)? Are the yellow/green colors > new (seem a bit loud)? > > On 2016/10/20 08:43:09, talkain wrote: > > Thank you. > > > > Sure,here are two samples of the change: > > http://i.imgur.com/fDb73kT.png > > http://i.imgur.com/YTujo48.png > > > > One is a binary frames only example and the other one is an example with mixed > > text frames. > > > > - Tal > > > > On 2016/10/20 00:06:48, Blaise wrote: > > > Looking good! Can we add a screenshot link (we usually just use imgur) to > see > > > how it'd look? > > > > > > Bringing in +chowse to review the UI change. > > > > > > Thanks! > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > File > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js > > > (right): > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:213: > > > this._dataText = > > > WebInspector.ResourceWebSocketFrameView.opCodeDescription(frame.opCode, > > > frame.mask); > > > Since we are not using this._dataText as our source for the > > > _escapeBinaryFrameData function, I think we should do this as an else of the > > > binary's if statement, so we don't need to call/set it for no reason. > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:223: > > > _escapeBinaryFrameData: function(frame) { > > > I'm sorry I should have clarified a little more... We do add JSDoc, but we > > don't > > > usually ad comments/notes. > > > > > > In this case it should look like this: > > > /** > > > * @param {!WebInspector.NetworkRequest.WebSocketFrame} frame > > > */ > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:223: > > > _escapeBinaryFrameData: function(frame) { > > > On second thought, lets inline this function. It's only used in this one > case > > > and not large enough to merit it's own function. > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:224: > > > if (frame.text.length > 5000) { > > > Single line if statements we do not add curly brackets around it. > > > > > > ie: > > > if (frame.text.length > 5000) > > > return; > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:228: > > > function escapeChar(match) { > > > We need to add docs: > > > /** > > > * @param {string} match > > > * @return {string} > > > */ > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:233: > > > this._dataText = frame.text.replace(/[^\x20-\x7E\n\r]/g, escapeChar); > > > Can we put this above the above function? We often take advantage of class > > > hoisting. (and it puts all the actual function code together)
Got it. Using hex codes for binary data seems fine to me. On 2016/10/21 07:03:43, talkain wrote: > Hi Chowse, > Here is a screenshot of the inspector before the change: > http://i.imgur.com/oDr2bgR.png > The colors aren't new, just the binary frames' content. > > On 2016/10/20 18:56:22, chowse wrote: > > Thanks for the screenshots. Tal/Blaise: Could you provide a screenshot of how > > the WebSocket appeared before this change? Is the change limited to using the > > \xHH hex codes in place of binary characters (SGTM)? Are the yellow/green > colors > > new (seem a bit loud)? > > > > On 2016/10/20 08:43:09, talkain wrote: > > > Thank you. > > > > > > Sure,here are two samples of the change: > > > http://i.imgur.com/fDb73kT.png > > > http://i.imgur.com/YTujo48.png > > > > > > One is a binary frames only example and the other one is an example with > mixed > > > text frames. > > > > > > - Tal > > > > > > On 2016/10/20 00:06:48, Blaise wrote: > > > > Looking good! Can we add a screenshot link (we usually just use imgur) to > > see > > > > how it'd look? > > > > > > > > Bringing in +chowse to review the UI change. > > > > > > > > Thanks! > > > > > > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > File > > > > > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:213: > > > > this._dataText = > > > > WebInspector.ResourceWebSocketFrameView.opCodeDescription(frame.opCode, > > > > frame.mask); > > > > Since we are not using this._dataText as our source for the > > > > _escapeBinaryFrameData function, I think we should do this as an else of > the > > > > binary's if statement, so we don't need to call/set it for no reason. > > > > > > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:223: > > > > _escapeBinaryFrameData: function(frame) { > > > > I'm sorry I should have clarified a little more... We do add JSDoc, but we > > > don't > > > > usually ad comments/notes. > > > > > > > > In this case it should look like this: > > > > /** > > > > * @param {!WebInspector.NetworkRequest.WebSocketFrame} frame > > > > */ > > > > > > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:223: > > > > _escapeBinaryFrameData: function(frame) { > > > > On second thought, lets inline this function. It's only used in this one > > case > > > > and not large enough to merit it's own function. > > > > > > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:224: > > > > if (frame.text.length > 5000) { > > > > Single line if statements we do not add curly brackets around it. > > > > > > > > ie: > > > > if (frame.text.length > 5000) > > > > return; > > > > > > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:228: > > > > function escapeChar(match) { > > > > We need to add docs: > > > > /** > > > > * @param {string} match > > > > * @return {string} > > > > */ > > > > > > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:233: > > > > this._dataText = frame.text.replace(/[^\x20-\x7E\n\r]/g, escapeChar); > > > > Can we put this above the above function? We often take advantage of class > > > > hoisting. (and it puts all the actual function code together)
Great! Can we please merge it? :) On 2016/10/27 21:07:46, chowse wrote: > Got it. Using hex codes for binary data seems fine to me. > > On 2016/10/21 07:03:43, talkain wrote: > > Hi Chowse, > > Here is a screenshot of the inspector before the change: > > http://i.imgur.com/oDr2bgR.png > > The colors aren't new, just the binary frames' content. > > > > On 2016/10/20 18:56:22, chowse wrote: > > > Thanks for the screenshots. Tal/Blaise: Could you provide a screenshot of > how > > > the WebSocket appeared before this change? Is the change limited to using > the > > > \xHH hex codes in place of binary characters (SGTM)? Are the yellow/green > > colors > > > new (seem a bit loud)? > > > > > > On 2016/10/20 08:43:09, talkain wrote: > > > > Thank you. > > > > > > > > Sure,here are two samples of the change: > > > > http://i.imgur.com/fDb73kT.png > > > > http://i.imgur.com/YTujo48.png > > > > > > > > One is a binary frames only example and the other one is an example with > > mixed > > > > text frames. > > > > > > > > - Tal > > > > > > > > On 2016/10/20 00:06:48, Blaise wrote: > > > > > Looking good! Can we add a screenshot link (we usually just use imgur) > to > > > see > > > > > how it'd look? > > > > > > > > > > Bringing in +chowse to review the UI change. > > > > > > > > > > Thanks! > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > File > > > > > > > > > > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > > > > > > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:213: > > > > > this._dataText = > > > > > WebInspector.ResourceWebSocketFrameView.opCodeDescription(frame.opCode, > > > > > frame.mask); > > > > > Since we are not using this._dataText as our source for the > > > > > _escapeBinaryFrameData function, I think we should do this as an else of > > the > > > > > binary's if statement, so we don't need to call/set it for no reason. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > > > > > > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:223: > > > > > _escapeBinaryFrameData: function(frame) { > > > > > I'm sorry I should have clarified a little more... We do add JSDoc, but > we > > > > don't > > > > > usually ad comments/notes. > > > > > > > > > > In this case it should look like this: > > > > > /** > > > > > * @param {!WebInspector.NetworkRequest.WebSocketFrame} frame > > > > > */ > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > > > > > > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:223: > > > > > _escapeBinaryFrameData: function(frame) { > > > > > On second thought, lets inline this function. It's only used in this one > > > case > > > > > and not large enough to merit it's own function. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > > > > > > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:224: > > > > > if (frame.text.length > 5000) { > > > > > Single line if statements we do not add curly brackets around it. > > > > > > > > > > ie: > > > > > if (frame.text.length > 5000) > > > > > return; > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > > > > > > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:228: > > > > > function escapeChar(match) { > > > > > We need to add docs: > > > > > /** > > > > > * @param {string} match > > > > > * @return {string} > > > > > */ > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2257963002/diff/60001/third_party/WebKit/Sour... > > > > > > > > > > > > > > > third_party/WebKit/Source/devtools/front_end/network/ResourceWebSocketFrameView.js:233: > > > > > this._dataText = frame.text.replace(/[^\x20-\x7E\n\r]/g, escapeChar); > > > > > Can we put this above the above function? We often take advantage of > class > > > > > hoisting. (and it puts all the actual function code together)
We need my comments in the last patch to be addressed before we can move forward. On 2016/10/29 09:14:54, talkain wrote: > Great! Can we please merge it? :)
On 2016/10/31 17:32:41, Blaise wrote: > We need my comments in the last patch to be addressed before we can move > forward. > On 2016/10/29 09:14:54, talkain wrote: > > Great! Can we please merge it? :) What's the status of this patch?
dgozman@chromium.org changed reviewers: - dgozman@chromium.org |
