Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
539353004:
DevTools: [Documentation] Update parser for WikiText (Closed)
|
Created:
6 years, 3 months ago by iliia Modified:
6 years, 3 months ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionDevTools: [Documentation] Update parser for WikiText
BUG=391593
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181901
Patch Set 1 #Patch Set 2 : add method secondToken() #
Total comments: 52
Patch Set 3 : comments addressed #
Total comments: 20
Patch Set 4 : comments addressed #
Total comments: 35
Patch Set 5 : comments addressed #Patch Set 6 : add annotation to WikiParser._parse() #
Total comments: 54
Patch Set 7 : comments addressed #
Total comments: 12
Patch Set 8 : comments addressed #
Total comments: 25
Patch Set 9 : comments addressed #Patch Set 10 : comments addressed #Patch Set 11 : fix cases #Patch Set 12 : comments addressed #
Total comments: 1
Patch Set 13 : move unescapeHTML() #Patch Set 14 : add test for unescape symbols #
Messages
Total messages: 26 (4 generated)
iliia@google.com changed reviewers: + loislo@chromium.org, lushnikov@chromium.org, semeny@google.com, yurys@chromium.org
Update WikiParser with tokenizer.
https://codereview.chromium.org/539353004/diff/20001/LayoutTests/inspector/do... File LayoutTests/inspector/documentation/document-parser.html (right): https://codereview.chromium.org/539353004/diff/20001/LayoutTests/inspector/do... LayoutTests/inspector/documentation/document-parser.html:114: debugger; remove https://codereview.chromium.org/539353004/diff/20001/LayoutTests/inspector/do... LayoutTests/inspector/documentation/document-parser.html:135: InspectorTest.addResult(error); prepend with "Expected error: " https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationURLProvider.js (right): https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationURLProvider.js:120: new WebInspector.DocumentationURLProvider.DocumentationSource(Window.prototype, "javascript/","Global"), why this change again? https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (left): https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:240: break; I believe you still want "break" statement https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:25: view.element.removeChildren(); why do we need this? https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:68: this._createPageForError(error); let's fallback to empty page for now. We'll do a "file a bug" fallback if we have enough time https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:75: this.element.removeChildren(); why this change? https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/documentation/JSArticle.js (right): https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:35: this.name = name ? name.children()[0].children()[0].text() : null; That's a very risky code. Who guarantees there'll be always a child in name, which has a child, which has text? https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:36: this.dataType = dataType ? dataType.children()[0].children()[0].text() : null; ditto https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:50: this.language = language ? language.children()[0].children()[0].text() : null ; you should not address that deep structures without checks. this will be an exception otherwise. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:63: this.returnValueName = returnValueName ? returnValueName.children()[0].children()[0].text() : null; ditto https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:90: var remarks = wikiDocument["Remarks_Section"] ? wikiDocument["Remarks_Section"]["Remarks"] : null; article.remarks = ... https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:94: var summary = wikiDocument["Summary_Section"]; article.summary =... https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/documentation/WikiParser.js (right): https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:11: var text = wikiMarkupText.replace(/</g, "<") lets extact this into utilities.js and call it unescapeHTML ( see method escapeHTML@utilities.js) https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:28: Table: "Table", do we have any tests for table? https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:217: WebInspector.WikiParser.State = { looks like you don't need this https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:232: WebInspector.WikiParser.HtmlStates = { and this https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:243: WebInspector.WikiParser.ValueState = { and even this https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:279: var obj = {}; obj is not a good name here as it doesn't give any clue. If we parse field here, lets call it "field" https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:288: if (this._tokenizer.peekToken().type() == WebInspector.WikiParser.TokenType.ClosingBraces) { === https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:301: while (this._tokenizer.peekToken().type() === WebInspector.WikiParser.TokenType.OpeningBraces) { this is while, inside case, inside while, inside if.. this is way too complicated to be read. You should split the logic here into functions https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:325: if (token.type() != WebInspector.WikiParser.TokenType.ClosingBraces) !== https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:349: return "Wrong title"; throw exception https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:374: return "Wrong name"; nope, you want to throw here https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:398: case WebInspector.WikiParser.TokenType.EqualSignInBraces: looks like this branch might be omitted, as Default will cover this case anyway https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:441: this._tokenizer.nextToken(); why missing break here?
comments addressed https://codereview.chromium.org/539353004/diff/20001/LayoutTests/inspector/do... File LayoutTests/inspector/documentation/document-parser.html (right): https://codereview.chromium.org/539353004/diff/20001/LayoutTests/inspector/do... LayoutTests/inspector/documentation/document-parser.html:114: debugger; On 2014/09/08 14:08:29, lushnikov wrote: > remove Done. https://codereview.chromium.org/539353004/diff/20001/LayoutTests/inspector/do... LayoutTests/inspector/documentation/document-parser.html:135: InspectorTest.addResult(error); On 2014/09/08 14:08:29, lushnikov wrote: > prepend with "Expected error: " Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationURLProvider.js (right): https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationURLProvider.js:120: new WebInspector.DocumentationURLProvider.DocumentationSource(Window.prototype, "javascript/","Global"), On 2014/09/08 14:08:29, lushnikov wrote: > why this change again? Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (left): https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:240: break; On 2014/09/08 14:08:30, lushnikov wrote: > I believe you still want "break" statement Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:25: view.element.removeChildren(); We discuss it with offline. On 2014/09/08 14:08:29, lushnikov wrote: > why do we need this? https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:68: this._createPageForError(error); On 2014/09/08 14:08:29, lushnikov wrote: > let's fallback to empty page for now. We'll do a "file a bug" fallback if we > have enough time Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:75: this.element.removeChildren(); We discuss it offline. On 2014/09/08 14:08:29, lushnikov wrote: > why this change? https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/documentation/JSArticle.js (right): https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:35: this.name = name ? name.children()[0].children()[0].text() : null; It was one simple plain text before, but now it's inside block. On 2014/09/08 14:08:30, lushnikov wrote: > That's a very risky code. Who guarantees there'll be always a child in name, > which has a child, which has text? https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:36: this.dataType = dataType ? dataType.children()[0].children()[0].text() : null; On 2014/09/08 14:08:30, lushnikov wrote: > ditto Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:50: this.language = language ? language.children()[0].children()[0].text() : null ; On 2014/09/08 14:08:30, lushnikov wrote: > you should not address that deep structures without checks. this will be an > exception otherwise. Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:63: this.returnValueName = returnValueName ? returnValueName.children()[0].children()[0].text() : null; On 2014/09/08 14:08:30, lushnikov wrote: > ditto Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:90: var remarks = wikiDocument["Remarks_Section"] ? wikiDocument["Remarks_Section"]["Remarks"] : null; On 2014/09/08 14:08:30, lushnikov wrote: > article.remarks = ... Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:94: var summary = wikiDocument["Summary_Section"]; On 2014/09/08 14:08:30, lushnikov wrote: > article.summary =... Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/documentation/WikiParser.js (right): https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:28: Table: "Table", we don't support tables at the moment. On 2014/09/08 14:08:31, lushnikov wrote: > do we have any tests for table? https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:217: WebInspector.WikiParser.State = { On 2014/09/08 14:08:30, lushnikov wrote: > looks like you don't need this Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:232: WebInspector.WikiParser.HtmlStates = { On 2014/09/08 14:08:30, lushnikov wrote: > and this Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:243: WebInspector.WikiParser.ValueState = { On 2014/09/08 14:08:31, lushnikov wrote: > and even this Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:279: var obj = {}; On 2014/09/08 14:08:30, lushnikov wrote: > obj is not a good name here as it doesn't give any clue. If we parse field here, > lets call it "field" Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:288: if (this._tokenizer.peekToken().type() == WebInspector.WikiParser.TokenType.ClosingBraces) { On 2014/09/08 14:08:31, lushnikov wrote: > === Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:301: while (this._tokenizer.peekToken().type() === WebInspector.WikiParser.TokenType.OpeningBraces) { On 2014/09/08 14:08:30, lushnikov wrote: > this is while, inside case, inside while, inside if.. this is way too > complicated to be read. You should split the logic here into functions Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:325: if (token.type() != WebInspector.WikiParser.TokenType.ClosingBraces) On 2014/09/08 14:08:30, lushnikov wrote: > !== Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:349: return "Wrong title"; On 2014/09/08 14:08:30, lushnikov wrote: > throw exception Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:349: return "Wrong title"; On 2014/09/08 14:08:30, lushnikov wrote: > throw exception Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:374: return "Wrong name"; On 2014/09/08 14:08:31, lushnikov wrote: > nope, you want to throw here Done. https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:398: case WebInspector.WikiParser.TokenType.EqualSignInBraces: in this case we should add "=", but not "{{=}}". On 2014/09/08 14:08:30, lushnikov wrote: > looks like this branch might be omitted, as Default will cover this case anyway https://codereview.chromium.org/539353004/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:441: this._tokenizer.nextToken(); because we must do exactly the same as in case of ClosingBrackets, when we met VerticalLine, we must stop parsing string. On 2014/09/08 14:08:30, lushnikov wrote: > why missing break here?
https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:87: _createPageForError: function(message) I don't see where you are using it. https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/documentation/JSArticle.js (right): https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:37: if (name && Array.isArray(name.children()) && name.children().length > 0 && Array.isArray(name.children()[0].children()) && name.children()[0].children().length > 0) please wrap the expression for better readability https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:41: if (dataType && Array.isArray(dataType.children()) && dataType.children().length > 0 && Array.isArray(dataType.children()[0].children()) && dataType.children()[0].children().length > 0) ditto https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:44: if (optional && Array.isArray(optional.children()) && optional.children().length > 0 && Array.isArray(optional.children()[0].children()) && optional.children()[0].children().length > 0) ditto https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:60: if (language && Array.isArray(language.children()) && language.children().length > 0 && Array.isArray(language.children()[0].children()) && language.children()[0].children().length > 0) ditto https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:65: if (liveUrl && Array.isArray(liveUrl.children()) && liveUrl.children().length > 0 && Array.isArray(liveUrl.children()[0].children()) && liveUrl.children()[0].children().length > 0) ditto https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:79: if (returnValueName && Array.isArray(returnValueName.children()) && returnValueName.children().length > 0 && Array.isArray(returnValueName.children()[0].children()) && returnValueName.children()[0].children().length > 0) ditto https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:83: if (returnValueDescription && Array.isArray(returnValueDescription.children()) && returnValueDescription.children().length > 0 && Array.isArray(returnValueDescription.children()[0].children()) && returnValueDescription.children()[0].children().length > 0) ditto https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/documentation/WikiParser.js (left): https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:109: please return back this empty line
https://codereview.chromium.org/539353004/diff/40001/LayoutTests/inspector/do... File LayoutTests/inspector/documentation/document-parser.html (right): https://codereview.chromium.org/539353004/diff/40001/LayoutTests/inspector/do... LayoutTests/inspector/documentation/document-parser.html:14: "{{A|Code= var nod{{=}}document.createElement(\"li\");\ndocument.getElementById(\"oUL1\").insertBefore(nod, document.getElementById(\"oLIYellow\"));\nnod.textContet{{=}}\"Orange\"}}", please reduce the size of the line.
comments addressed https://codereview.chromium.org/539353004/diff/40001/LayoutTests/inspector/do... File LayoutTests/inspector/documentation/document-parser.html (right): https://codereview.chromium.org/539353004/diff/40001/LayoutTests/inspector/do... LayoutTests/inspector/documentation/document-parser.html:14: "{{A|Code= var nod{{=}}document.createElement(\"li\");\ndocument.getElementById(\"oUL1\").insertBefore(nod, document.getElementById(\"oLIYellow\"));\nnod.textContet{{=}}\"Orange\"}}", There is the same test in testExamples, so I will delete this string. On 2014/09/09 08:06:57, loislo wrote: > please reduce the size of the line. https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:87: _createPageForError: function(message) On 2014/09/09 08:05:04, loislo wrote: > I don't see where you are using it. Done. https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/documentation/JSArticle.js (right): https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:37: if (name && Array.isArray(name.children()) && name.children().length > 0 && Array.isArray(name.children()[0].children()) && name.children()[0].children().length > 0) On 2014/09/09 08:05:04, loislo wrote: > please wrap the expression for better readability Done. https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:41: if (dataType && Array.isArray(dataType.children()) && dataType.children().length > 0 && Array.isArray(dataType.children()[0].children()) && dataType.children()[0].children().length > 0) On 2014/09/09 08:05:04, loislo wrote: > ditto Done. https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:44: if (optional && Array.isArray(optional.children()) && optional.children().length > 0 && Array.isArray(optional.children()[0].children()) && optional.children()[0].children().length > 0) On 2014/09/09 08:05:04, loislo wrote: > ditto Done. https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:60: if (language && Array.isArray(language.children()) && language.children().length > 0 && Array.isArray(language.children()[0].children()) && language.children()[0].children().length > 0) On 2014/09/09 08:05:04, loislo wrote: > ditto Done. https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:65: if (liveUrl && Array.isArray(liveUrl.children()) && liveUrl.children().length > 0 && Array.isArray(liveUrl.children()[0].children()) && liveUrl.children()[0].children().length > 0) On 2014/09/09 08:05:04, loislo wrote: > ditto Done. https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:79: if (returnValueName && Array.isArray(returnValueName.children()) && returnValueName.children().length > 0 && Array.isArray(returnValueName.children()[0].children()) && returnValueName.children()[0].children().length > 0) On 2014/09/09 08:05:04, loislo wrote: > ditto Done. https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:83: if (returnValueDescription && Array.isArray(returnValueDescription.children()) && returnValueDescription.children().length > 0 && Array.isArray(returnValueDescription.children()[0].children()) && returnValueDescription.children()[0].children().length > 0) On 2014/09/09 08:05:04, loislo wrote: > ditto Done. https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/documentation/WikiParser.js (left): https://codereview.chromium.org/539353004/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:109: On 2014/09/09 08:05:05, loislo wrote: > please return back this empty line Done.
https://codereview.chromium.org/539353004/diff/60001/LayoutTests/inspector/do... File LayoutTests/inspector/documentation/document-parser-expected.txt (right): https://codereview.chromium.org/539353004/diff/60001/LayoutTests/inspector/do... LayoutTests/inspector/documentation/document-parser-expected.txt:248: _text : "[http://msdn.microsoft.com/en-us/library/ie/ms536365(v=vs.85).aspx cloneNode Method]" and this is not going to be rendered as a link.. hmm https://codereview.chromium.org/539353004/diff/60001/LayoutTests/inspector/do... LayoutTests/inspector/documentation/document-parser-expected.txt:286: _url : " , replacer" so it looks like we gonna parse this as a link? This is unfortunate. https://codereview.chromium.org/539353004/diff/60001/LayoutTests/inspector/do... LayoutTests/inspector/documentation/document-parser-expected.txt:316: } lets add a test for the table to make sure we don't throw https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:66: var article = WebInspector.JSArticle.parse(wikiMarkupText); lets avoid varaibel hoisting and declare it outside of the try{ } block https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:68: this._createEmptyPage(); article = null; console.error(error); https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... File Source/devtools/front_end/documentation/JSArticle.js (right): https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:39: this.name = name.children()[0].children()[0].text(); I would introduce a helper function "textContent", which behaves similarly to Node.prototype.textContent. I'd use it here https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:44: this.dataType = dataType.children()[0].children()[0].text(); and here https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:48: this.optional = optional.children()[0].children()[0].text().toUpperCase() === "YES"; and here https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... File Source/devtools/front_end/documentation/WikiParser.js (right): https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:11: var text = wikiMarkupText.replace(/</g, "<") lets extract this into utilities? https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:111: var begin = result.index + result[0].length - 1; where does this -1 come from? https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:243: * @return {?Object} it never returns null. Why nullable? also, it should be parametrized https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:263: while (this._tokenizer.hasMoreTokens()) { can we extract this whole while into a separate function? https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:276: break; remove https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:293: * @return {!Array.<?Object>} why nullable? https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:299: array.push(this._parseField()); this cyclic recursion looks odd. I believe we can avoid it https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:324: throw new Error("Can't parse Title, unexpected token " + token.value()); "Title could not be parsed. Unexpected token: " + token.value() https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:350: throw new Error("Can't parse name, unexpected token " + token.value()); "Name could not be parsed. Unexpected token: " + token.value()
comments addressed https://codereview.chromium.org/539353004/diff/60001/LayoutTests/inspector/do... File LayoutTests/inspector/documentation/document-parser-expected.txt (right): https://codereview.chromium.org/539353004/diff/60001/LayoutTests/inspector/do... LayoutTests/inspector/documentation/document-parser-expected.txt:248: _text : "[http://msdn.microsoft.com/en-us/library/ie/ms536365(v=vs.85).aspx cloneNode Method]" Yes, because it's a link without space in the beginning and without vertical line in the middle. On 2014/09/09 12:40:45, lushnikov wrote: > and this is not going to be rendered as a link.. hmm https://codereview.chromium.org/539353004/diff/60001/LayoutTests/inspector/do... LayoutTests/inspector/documentation/document-parser-expected.txt:286: _url : " , replacer" Yes, because there is a brackets with a space in the beginning. On 2014/09/09 12:40:45, lushnikov wrote: > so it looks like we gonna parse this as a link? This is unfortunate. https://codereview.chromium.org/539353004/diff/60001/LayoutTests/inspector/do... LayoutTests/inspector/documentation/document-parser-expected.txt:316: } On 2014/09/09 12:40:45, lushnikov wrote: > lets add a test for the table to make sure we don't throw Done. https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:66: var article = WebInspector.JSArticle.parse(wikiMarkupText); On 2014/09/09 12:40:46, lushnikov wrote: > lets avoid varaibel hoisting and declare it outside of the try{ } block Done. https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:68: this._createEmptyPage(); On 2014/09/09 12:40:46, lushnikov wrote: > article = null; > console.error(error); Done. https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... File Source/devtools/front_end/documentation/JSArticle.js (right): https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:39: this.name = name.children()[0].children()[0].text(); On 2014/09/09 12:40:46, lushnikov wrote: > I would introduce a helper function "textContent", which behaves similarly to > Node.prototype.textContent. I'd use it here Done. https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:44: this.dataType = dataType.children()[0].children()[0].text(); On 2014/09/09 12:40:46, lushnikov wrote: > and here Done. https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:48: this.optional = optional.children()[0].children()[0].text().toUpperCase() === "YES"; On 2014/09/09 12:40:46, lushnikov wrote: > and here Done. https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... File Source/devtools/front_end/documentation/WikiParser.js (right): https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:11: var text = wikiMarkupText.replace(/</g, "<") On 2014/09/09 12:40:46, lushnikov wrote: > lets extract this into utilities? Done. https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:111: var begin = result.index + result[0].length - 1; it's a space after new line in the beginning. On 2014/09/09 12:40:46, lushnikov wrote: > where does this -1 come from? https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:243: * @return {?Object} Then it will be Object.<string, string | Array.<string | !WebInspector.WikiParser.ArticleElement>> On 2014/09/09 12:40:46, lushnikov wrote: > it never returns null. Why nullable? also, it should be parametrized https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:263: while (this._tokenizer.hasMoreTokens()) { Yes, but I'm out of names for these functions :( On 2014/09/09 12:40:46, lushnikov wrote: > can we extract this whole while into a separate function? https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:263: while (this._tokenizer.hasMoreTokens()) { On 2014/09/09 12:40:46, lushnikov wrote: > can we extract this whole while into a separate function? Done. https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:276: break; why don't we need this break? On 2014/09/09 12:40:46, lushnikov wrote: > remove https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:293: * @return {!Array.<?Object>} On 2014/09/09 12:40:46, lushnikov wrote: > why nullable? Done. https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:299: array.push(this._parseField()); why? we must read all of internal blocks here without exiting switch. On 2014/09/09 12:40:46, lushnikov wrote: > this cyclic recursion looks odd. I believe we can avoid it https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:324: throw new Error("Can't parse Title, unexpected token " + token.value()); On 2014/09/09 12:40:46, lushnikov wrote: > "Title could not be parsed. Unexpected token: " + token.value() Done. https://codereview.chromium.org/539353004/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/WikiParser.js:350: throw new Error("Can't parse name, unexpected token " + token.value()); On 2014/09/09 12:40:46, lushnikov wrote: > "Name could not be parsed. Unexpected token: " + token.value() Done.
Patchset #6 (id:100001) has been deleted
Add annotation
apavlov@chromium.org changed reviewers: + apavlov@chromium.org
https://codereview.chromium.org/539353004/diff/120001/LayoutTests/inspector/d... File LayoutTests/inspector/documentation/document-parser-expected.txt (right): https://codereview.chromium.org/539353004/diff/120001/LayoutTests/inspector/d... LayoutTests/inspector/documentation/document-parser-expected.txt:395: _url : " , replacer" This corresponds to an optional argument, not a link, right? https://codereview.chromium.org/539353004/diff/120001/LayoutTests/inspector/d... LayoutTests/inspector/documentation/document-parser-expected.txt:401: _url : " , space" ditto https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:69: this._createEmptyPage(); This and the following line can be dropped: if (!article) {...} below will _createEmptyPage the second time otherwise. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:71: console.error(error); Should this be error.message? https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:269: * @return {?Element} Any reason for this being nullable? https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:300: console.error("Unknown ArticleElement type " + article.type()); Is this intentionally missing a break; ? Or should this go last in the switch? What should be returned in this case? https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:306: if (article instanceof WebInspector.WikiParser.Block || article instanceof WebInspector.WikiParser.Inline) { Now we can check article.type() https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/documentation/JSArticle.js (right): https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/JSArticle.js:37: this.optional = WebInspector.JSArticle.textContent(optional) ? WebInspector.JSArticle.textContent(optional).toUpperCase() === "YES" : false; This is computing the text content twice in a row. Please pre-compute to avoid the overhead. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/JSArticle.js:69: * @return {null|string} {?string} https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/JSArticle.js:73: if (block && Array.isArray(block.children()) && block.children().length > 0 Let's introduce WI.WikiParser.Block.prototype.hasChildren(): return !!this._children && !!this._children.length; https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/JSArticle.js:75: return block.children()[0].children()[0].text(); Is this the universally correct way to compute the block text content? https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/documentation/WikiParser.js (right): https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:23: /** @type {?WebInspector.WikiParser.Values} */ blank lines before jsdocs https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:36: /** @type {?WebInspector.WikiParser.FieldValue} */ ditto https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:42: /** @typedef {?Object.<string, !WebInspector.WikiParser.FieldValue>} */ ditto https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:251: _secondTokenType: function() why not just save and restore the state of the current tokenizer instead of cloning it? It's the easiest way to peek a token. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:323: var field = new WebInspector.WikiParser.Field; new ...Field(); https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:374: break; "return title;" for consistency? https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:413: /** blank lines between inner function and surrounding code https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:416: function wrapToArticleElement() To -> Into https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:418: var plainText = new WebInspector.WikiParser.PlainText(code, false); Can these "false" arguments be made optional in the constructors? https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:433: code += '|'; double quotes https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:595: return new WebInspector.WikiParser.Inline(WebInspector.WikiParser.ArticleElement.Type.Inline, children); Article element types should never get passed into the constructor of concrete elements. Rather, their constructors should pass the type into the superconstructor. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:760: text : function() no whitespace before colon
comments addressed https://codereview.chromium.org/539353004/diff/120001/LayoutTests/inspector/d... File LayoutTests/inspector/documentation/document-parser-expected.txt (right): https://codereview.chromium.org/539353004/diff/120001/LayoutTests/inspector/d... LayoutTests/inspector/documentation/document-parser-expected.txt:395: _url : " , replacer" Yes, but we don't know how to fix it. On 2014/09/10 14:09:23, apavlov (OOO until Sept.19) wrote: > This corresponds to an optional argument, not a link, right? https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:69: this._createEmptyPage(); On 2014/09/10 14:09:23, apavlov (OOO until Sept.19) wrote: > This and the following line can be dropped: > if (!article) {...} below will _createEmptyPage the second time otherwise. Done. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:71: console.error(error); On 2014/09/10 14:09:23, apavlov (OOO until Sept.19) wrote: > Should this be error.message? Done. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:269: * @return {?Element} element could be undefined if something's going wrong. On 2014/09/10 14:09:23, apavlov (OOO until Sept.19) wrote: > Any reason for this being nullable? https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:300: console.error("Unknown ArticleElement type " + article.type()); On 2014/09/10 14:09:23, apavlov (OOO until Sept.19) wrote: > Is this intentionally missing a break; ? Or should this go last in the switch? > What should be returned in this case? Done. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:306: if (article instanceof WebInspector.WikiParser.Block || article instanceof WebInspector.WikiParser.Inline) { On 2014/09/10 14:09:23, apavlov (OOO until Sept.19) wrote: > Now we can check article.type() Done. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/documentation/JSArticle.js (right): https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/JSArticle.js:37: this.optional = WebInspector.JSArticle.textContent(optional) ? WebInspector.JSArticle.textContent(optional).toUpperCase() === "YES" : false; On 2014/09/10 14:09:23, apavlov (OOO until Sept.19) wrote: > This is computing the text content twice in a row. Please pre-compute to avoid > the overhead. Done. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/JSArticle.js:69: * @return {null|string} On 2014/09/10 14:09:23, apavlov (OOO until Sept.19) wrote: > {?string} Done. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/JSArticle.js:73: if (block && Array.isArray(block.children()) && block.children().length > 0 On 2014/09/10 14:09:23, apavlov (OOO until Sept.19) wrote: > Let's introduce WI.WikiParser.Block.prototype.hasChildren(): > > return !!this._children && !!this._children.length; Done. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/JSArticle.js:75: return block.children()[0].children()[0].text(); Yes, earlier there was only one simple string. But now due to parser's changes it's one simple string inside blocks. On 2014/09/10 14:09:23, apavlov (OOO until Sept.19) wrote: > Is this the universally correct way to compute the block text content? https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/documentation/WikiParser.js (right): https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:23: /** @type {?WebInspector.WikiParser.Values} */ On 2014/09/10 14:09:24, apavlov (OOO until Sept.19) wrote: > blank lines before jsdocs Done. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:36: /** @type {?WebInspector.WikiParser.FieldValue} */ On 2014/09/10 14:09:23, apavlov (OOO until Sept.19) wrote: > ditto Done. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:42: /** @typedef {?Object.<string, !WebInspector.WikiParser.FieldValue>} */ On 2014/09/10 14:09:24, apavlov (OOO until Sept.19) wrote: > ditto Done. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:251: _secondTokenType: function() Clone is a saving copy of the state of current tokenizer. On 2014/09/10 14:09:23, apavlov (OOO until Sept.19) wrote: > why not just save and restore the state of the current tokenizer instead of > cloning it? It's the easiest way to peek a token. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:323: var field = new WebInspector.WikiParser.Field; On 2014/09/10 14:09:23, apavlov (OOO until Sept.19) wrote: > new ...Field(); Done. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:374: break; We must return title only then we met | or }}, so we need to continue and to read the next token. On 2014/09/10 14:09:24, apavlov (OOO until Sept.19) wrote: > "return title;" for consistency? https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:413: /** On 2014/09/10 14:09:24, apavlov (OOO until Sept.19) wrote: > blank lines between inner function and surrounding code Done. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:416: function wrapToArticleElement() On 2014/09/10 14:09:24, apavlov (OOO until Sept.19) wrote: > To -> Into Done. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:418: var plainText = new WebInspector.WikiParser.PlainText(code, false); On 2014/09/10 14:09:24, apavlov (OOO until Sept.19) wrote: > Can these "false" arguments be made optional in the constructors? Done. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:433: code += '|'; On 2014/09/10 14:09:23, apavlov (OOO until Sept.19) wrote: > double quotes Done. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:595: return new WebInspector.WikiParser.Inline(WebInspector.WikiParser.ArticleElement.Type.Inline, children); So if some of Inlines haven't any special fields or methods, I think it's not a good idea to create new classes for them. On 2014/09/10 14:09:24, apavlov (OOO until Sept.19) wrote: > Article element types should never get passed into the constructor of concrete > elements. Rather, their constructors should pass the type into the > superconstructor. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:760: text : function() On 2014/09/10 14:09:24, apavlov (OOO until Sept.19) wrote: > no whitespace before colon Done.
https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:71: console.error(error); On 2014/09/11 09:26:30, iliia wrote: > On 2014/09/10 14:09:23, apavlov (OOO until Sept.19) wrote: > > Should this be error.message? > > Done. Not done - still, the "error" object is used instead of error.message https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/documentation/WikiParser.js (right): https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:251: _secondTokenType: function() On 2014/09/11 09:26:31, iliia wrote: > Clone is a saving copy of the state of current tokenizer. > On 2014/09/10 14:09:23, apavlov (OOO until Sept.19) wrote: > > why not just save and restore the state of the current tokenizer instead of > > cloning it? It's the easiest way to peek a token. > I know that cloning implicitly allows to retain the current instance state intact, but is a lot more heavyweight (a new instance creation + destruction through GC). pushState/popState or plain saving of the state into local variables is much faster. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:374: break; On 2014/09/11 09:26:31, iliia wrote: > We must return title only then we met | or }}, so we need to continue and to > read the next token. > On 2014/09/10 14:09:24, apavlov (OOO until Sept.19) wrote: > > "return title;" for consistency? > Oops, misread the code, thanks. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:595: return new WebInspector.WikiParser.Inline(WebInspector.WikiParser.ArticleElement.Type.Inline, children); On 2014/09/11 09:26:30, iliia wrote: > So if some of Inlines haven't any special fields or methods, I think it's not a > good idea to create new classes for them. > On 2014/09/10 14:09:24, apavlov (OOO until Sept.19) wrote: > > Article element types should never get passed into the constructor of concrete > > elements. Rather, their constructors should pass the type into the > > superconstructor. > Can an instance of WI.WikiParser.Inline have a type different than "Inline"? https://codereview.chromium.org/539353004/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/539353004/diff/140001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:180: parameterType.textContent = parameters[i].dataType + (parameters[i].optional ? "=" : "");; extra trailing ; https://codereview.chromium.org/539353004/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/documentation/WikiParser.js (right): https://codereview.chromium.org/539353004/diff/140001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:43: /** @typedef {(?WebInspector.WikiParser.ArticleElement|!Array.<!WebInspector.WikiParser.Section>)} */ Has this and the following union type been agreed upon with Andrey? Last time I was present at a related discussion, we agreed that there should be no unions. https://codereview.chromium.org/539353004/diff/140001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:277: obj[section.title] = section.singleValue; obj[section.title] = section.singleValue || section.values; https://codereview.chromium.org/539353004/diff/140001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:429: while (this._tokenizer.hasMoreTokens()) { a blank line above for the readability would be nice https://codereview.chromium.org/539353004/diff/140001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:759: this._isHighlighted = highlight ? highlight : false; highlight || false https://codereview.chromium.org/539353004/diff/140001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:792: this._hasBullet = hasBullet ? hasBullet : false; ditto
comments addressed https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:71: console.error(error); On 2014/09/11 10:40:26, apavlov (OOO until Sept.19) wrote: > On 2014/09/11 09:26:30, iliia wrote: > > On 2014/09/10 14:09:23, apavlov (OOO until Sept.19) wrote: > > > Should this be error.message? > > > > Done. > > Not done - still, the "error" object is used instead of error.message Done. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/documentation/WikiParser.js (right): https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:595: return new WebInspector.WikiParser.Inline(WebInspector.WikiParser.ArticleElement.Type.Inline, children); Yes, for example, Link. On 2014/09/11 10:40:26, apavlov (OOO until Sept.19) wrote: > On 2014/09/11 09:26:30, iliia wrote: > > So if some of Inlines haven't any special fields or methods, I think it's not > a > > good idea to create new classes for them. > > On 2014/09/10 14:09:24, apavlov (OOO until Sept.19) wrote: > > > Article element types should never get passed into the constructor of > concrete > > > elements. Rather, their constructors should pass the type into the > > > superconstructor. > > > > Can an instance of WI.WikiParser.Inline have a type different than "Inline"? https://codereview.chromium.org/539353004/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/539353004/diff/140001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:180: parameterType.textContent = parameters[i].dataType + (parameters[i].optional ? "=" : "");; On 2014/09/11 10:40:26, apavlov (OOO until Sept.19) wrote: > extra trailing ; Done. https://codereview.chromium.org/539353004/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/documentation/WikiParser.js (right): https://codereview.chromium.org/539353004/diff/140001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:43: /** @typedef {(?WebInspector.WikiParser.ArticleElement|!Array.<!WebInspector.WikiParser.Section>)} */ Yes, it was discussed with him. We don't know how to avoid this union. On 2014/09/11 10:40:27, apavlov (OOO until Sept.19) wrote: > Has this and the following union type been agreed upon with Andrey? Last time I > was present at a related discussion, we agreed that there should be no unions. https://codereview.chromium.org/539353004/diff/140001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:277: obj[section.title] = section.singleValue; On 2014/09/11 10:40:27, apavlov (OOO until Sept.19) wrote: > obj[section.title] = section.singleValue || section.values; Done. https://codereview.chromium.org/539353004/diff/140001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:429: while (this._tokenizer.hasMoreTokens()) { On 2014/09/11 10:40:27, apavlov (OOO until Sept.19) wrote: > a blank line above for the readability would be nice Done. https://codereview.chromium.org/539353004/diff/140001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:759: this._isHighlighted = highlight ? highlight : false; On 2014/09/11 10:40:27, apavlov (OOO until Sept.19) wrote: > highlight || false Done. https://codereview.chromium.org/539353004/diff/140001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:792: this._hasBullet = hasBullet ? hasBullet : false; On 2014/09/11 10:40:27, apavlov (OOO until Sept.19) wrote: > ditto Done.
https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/documentation/WikiParser.js (right): https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:595: return new WebInspector.WikiParser.Inline(WebInspector.WikiParser.ArticleElement.Type.Inline, children); On 2014/09/11 13:26:28, iliia wrote: > Yes, for example, Link. > On 2014/09/11 10:40:26, apavlov (OOO until Sept.19) wrote: > > On 2014/09/11 09:26:30, iliia wrote: > > > So if some of Inlines haven't any special fields or methods, I think it's > not > > a > > > good idea to create new classes for them. > > > On 2014/09/10 14:09:24, apavlov (OOO until Sept.19) wrote: > > > > Article element types should never get passed into the constructor of > > concrete > > > > elements. Rather, their constructors should pass the type into the > > > > superconstructor. > > > > > > > Can an instance of WI.WikiParser.Inline have a type different than "Inline"? > OK, I have chatted with Andrey about it. Leaving this to his discretion. https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:704: * @return {string} str remove "str" https://codereview.chromium.org/539353004/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:706: _deleteFrontLineEnds: function(str) _trimLeadingNewLines
https://codereview.chromium.org/539353004/diff/160001/LayoutTests/inspector/d... File LayoutTests/inspector/documentation/document-parser-expected.txt (right): https://codereview.chromium.org/539353004/diff/160001/LayoutTests/inspector/d... LayoutTests/inspector/documentation/document-parser-expected.txt:192: |Description= [[css/color|CSS color value]]}} why added space? https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... File Source/devtools/front_end/common/utilities.js (right): https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/common/utilities.js:175: .replace(/>/g, ">") lets indent this with 4 spaces instead of aligning per-dots https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:301: console.error("Unknown ArticleElement type " + article.type()); don't you want to return null here as well? https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... File Source/devtools/front_end/documentation/JSArticle.js (right): https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/JSArticle.js:74: if (block && block.hasChildren() && block.children()[0].hasChildren()) lets make this a real textContent which returns concatenated text of all children, or rename into something like unfoldStringValue to avoid confusion. https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... File Source/devtools/front_end/documentation/WikiParser.js (right): https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:140: var begin = result.index + result[0].length - 3; where does this magic comes from? https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:205: WebInspector.WikiParser.openingCodeTag = /^<\s*code\s*>/; as far as I recall no spaces are allowed after "<" and tag name https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:206: WebInspector.WikiParser.closingCodeTag = /^<\s*\/\s*code\s*>/; ditto https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:336: break; break is not needed here; https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:338: if (field.name === "Code") lets at least compare upperstrings to reduce real-life flakiness. https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:362: _parseTitle: function() parseSectionTitle https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:386: _parseName: function() parseFieldName https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:453: _parseString: function() It would be good if we come with some other name for this as it's not clear from the naming that it deals with a whole new world of markup, not just plain strings. parseMarkupText? https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:482: this._tokenizer.nextToken(); I believe you'd like to have a break here.
comments addressed https://codereview.chromium.org/539353004/diff/160001/LayoutTests/inspector/d... File LayoutTests/inspector/documentation/document-parser-expected.txt (right): https://codereview.chromium.org/539353004/diff/160001/LayoutTests/inspector/d... LayoutTests/inspector/documentation/document-parser-expected.txt:192: |Description= [[css/color|CSS color value]]}} We assume that link has a space in the beginning. On 2014/09/12 06:39:28, lushnikov wrote: > why added space? https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... File Source/devtools/front_end/common/utilities.js (right): https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/common/utilities.js:175: .replace(/>/g, ">") On 2014/09/12 06:39:28, lushnikov wrote: > lets indent this with 4 spaces instead of aligning per-dots Done. https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:301: console.error("Unknown ArticleElement type " + article.type()); On 2014/09/12 06:39:29, lushnikov wrote: > don't you want to return null here as well? Done. https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... File Source/devtools/front_end/documentation/JSArticle.js (right): https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/JSArticle.js:74: if (block && block.hasChildren() && block.children()[0].hasChildren()) On 2014/09/12 06:39:29, lushnikov wrote: > lets make this a real textContent which returns concatenated text of all > children, or rename into something like unfoldStringValue to avoid confusion. Done. https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... File Source/devtools/front_end/documentation/WikiParser.js (right): https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:140: var begin = result.index + result[0].length - 3; We want to save all matching symbols. It's \n, then space and then one not space. On 2014/09/12 06:39:29, lushnikov wrote: > where does this magic comes from? https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:205: WebInspector.WikiParser.openingCodeTag = /^<\s*code\s*>/; When we discussed it (~a month ago), you told me to ignore spaces before and after word "code" :) On 2014/09/12 06:39:29, lushnikov wrote: > as far as I recall no spaces are allowed after "<" and tag name https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:336: break; On 2014/09/12 06:39:29, lushnikov wrote: > break is not needed here; Done. https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:338: if (field.name === "Code") On 2014/09/12 06:39:29, lushnikov wrote: > lets at least compare upperstrings to reduce real-life flakiness. Done. https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:362: _parseTitle: function() On 2014/09/12 06:39:29, lushnikov wrote: > parseSectionTitle Done. https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:386: _parseName: function() On 2014/09/12 06:39:29, lushnikov wrote: > parseFieldName Done. https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:453: _parseString: function() On 2014/09/12 06:39:29, lushnikov wrote: > It would be good if we come with some other name for this as it's not clear from > the naming that it deals with a whole new world of markup, not just plain > strings. parseMarkupText? Done. https://codereview.chromium.org/539353004/diff/160001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:482: this._tokenizer.nextToken(); No. We must do exactly the same as we do in case ClosingBrackets. On 2014/09/12 06:39:29, lushnikov wrote: > I believe you'd like to have a break here.
https://codereview.chromium.org/539353004/diff/240001/Source/devtools/front_e... File Source/devtools/front_end/documentation/WikiParser.js (right): https://codereview.chromium.org/539353004/diff/240001/Source/devtools/front_e... Source/devtools/front_end/documentation/WikiParser.js:11: var text = wikiMarkupText.unescapeHTML(); we shouldn't unescape prior to parsing. This kills the sole purpose of escaped characters.
LGTM
The CQ bit was checked by loislo@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/539353004/280001
Message was sent while issue was closed.
Committed patchset #14 (id:280001) as 181901 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
