Issue 252880043: Use javascript to parse html & generate js content (Closed)
|
Created:
8 years, 9 months ago by nednguyen Modified:
8 years, 9 months ago CC:
tracing-review_chromium.org Base URL:
https://github.com/google/trace-viewer.git@master Visibility:
Public. |
DescriptionUse javascript to parse html & generate js content
BUG=https://github.com/google/trace-viewer/issues/1030, https://github.com/google/trace-viewer/issues/1034
R=dsinclair@chromium.org
Committed: https://chromium.googlesource.com/external/trace-viewer/+/b734ae94ff81ed6de98648db8f7357b9129d3939
Patch Set 1 : #
Total comments: 27
Patch Set 2 : Address Dan's comments #
Total comments: 3
Patch Set 3 : Address Dan's comment #
Total comments: 43
Patch Set 4 : Address Dan's comments #
Total comments: 8
Patch Set 5 : Address Dan's comments #
Total comments: 4
Patch Set 6 : Drop number type check in createJsChunk #Patch Set 7 : Make all the generateJsChunks... return array #
MessagesTotal messages: 21
I still need to add bunch of tests & clean up the implementation a bit. Please be gentle as I am illiterate in js :P. This amazingly speeds up the run_d8_tests from 22.4s to 1.4s.
Sign in to reply to this message.
Added unnittest, PTAL https://codereview.appspot.com/252880043/diff/110001/tracing/build/d8_runner_... File tracing/build/d8_runner_unittest.py (right): https://codereview.appspot.com/252880043/diff/110001/tracing/build/d8_runner_... tracing/build/d8_runner_unittest.py:322: def testGenerateJsForD8RunnerImportWithEscapedScriptTag(self): Nat: is this the test case that fails HTMLParser?
Sign in to reply to this message.
As a side note, you can do BUG=#1030, #1034 and github should link the bugs correctly when committed.
Sign in to reply to this message.
On 2015/07/06 16:50:51, dsinclair wrote: > As a side note, you can do > > BUG=#1030, #1034 > > and github should link the bugs correctly when committed. Hi Dan, I choose to put the whole URL because it's convenient to just grab the link and get to the github bug :P
Sign in to reply to this message.
https://codereview.appspot.com/252880043/diff/110001/tracing/build/d8_bootstr... File tracing/build/d8_bootstrap.js (right): https://codereview.appspot.com/252880043/diff/110001/tracing/build/d8_bootstr... tracing/build/d8_bootstrap.js:158: global.path_to_parse5 = '<%parse5_js_path%>'; I'd call this path_to_js_parser. If we happen to change parse5 to something else in the future we don't need to update the variable name. https://codereview.appspot.com/252880043/diff/110001/tracing/build/d8_bootstr... tracing/build/d8_bootstrap.js:245: ': File does not exist'); Wouldn't it be better to output the actual error? Are there not other reasons for read to fail then just file doesn't exist? https://codereview.appspot.com/252880043/diff/110001/tracing/build/d8_runner.py File tracing/build/d8_runner.py (right): https://codereview.appspot.com/252880043/diff/110001/tracing/build/d8_runner.... tracing/build/d8_runner.py:35: _PARSE5_JS_DIR = os.path.abspath( nit: _PARSER_JS_DIR https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... File tracing/build/html_to_js_generator.js (right): https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:19: // parse5.js. I'm not sure I understand what this means? Why do we need to hoist the |global| to parse5? And, do you mean it isn't available to the load()'d file? https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:33: this.content = content; Internal members should have a trailing _. So, this.content_, etc. https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:52: throw new Error('Cannot generate js content for ' + chunks[i].content); Can you add a bit more into this error? Something along the lines of 'Expected to add x blank lines but got y') https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:71: if (!node.__location) { Where does __location come from? Is it a parse5 thing? Is it part of the public api or is it internal? https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:78: if (!node.__location) { nit: No {}'s on single line if statements. https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:110: if (is_import_link) Nit: Braces on multiline if statements. https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:152: } else if (node.nodeName === 'script') { Don't need the else's since each if returns.
Sign in to reply to this message.
On 2015/07/06 16:51:45, nednguyen wrote: > On 2015/07/06 16:50:51, dsinclair wrote: > > As a side note, you can do > > > > BUG=#1030, #1034 > > > > and github should link the bugs correctly when committed. > > Hi Dan, I choose to put the whole URL because it's convenient to just grab the > link and get to the github bug :P I see. I've never tried to follow them, I'm just trying to read the BUG= line and it's a lot more cluttered with the full URL.
Sign in to reply to this message.
https://codereview.appspot.com/252880043/diff/110001/tracing/build/d8_bootstr... File tracing/build/d8_bootstrap.js (right): https://codereview.appspot.com/252880043/diff/110001/tracing/build/d8_bootstr... tracing/build/d8_bootstrap.js:158: global.path_to_parse5 = '<%parse5_js_path%>'; On 2015/07/06 17:06:45, dsinclair wrote: > I'd call this path_to_js_parser. If we happen to change parse5 to something else > in the future we don't need to update the variable name. Done. https://codereview.appspot.com/252880043/diff/110001/tracing/build/d8_bootstr... tracing/build/d8_bootstrap.js:245: ': File does not exist'); On 2015/07/06 17:06:44, dsinclair wrote: > Wouldn't it be better to output the actual error? Are there not other reasons > for read to fail then just file doesn't exist? For some reason the stack trace messed up if we don't catch error thrown by read. My guess the read() function does not throw new Error but just a string. https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... File tracing/build/html_to_js_generator.js (right): https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:19: // parse5.js. On 2015/07/06 17:06:45, dsinclair wrote: > I'm not sure I understand what this means? Why do we need to hoist the |global| > to parse5? And, do you mean it isn't available to the load()'d file? Updated documentation, is this better? https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:33: this.content = content; On 2015/07/06 17:06:45, dsinclair wrote: > Internal members should have a trailing _. So, this.content_, etc. I intend to use this as "struct" rather than "class", so make all these members "public". Is this an acceptable style? https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:52: throw new Error('Cannot generate js content for ' + chunks[i].content); On 2015/07/06 17:06:45, dsinclair wrote: > Can you add a bit more into this error? Something along the lines of 'Expected > to add x blank lines but got y') Done. https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:71: if (!node.__location) { On 2015/07/06 17:06:45, dsinclair wrote: > Where does __location come from? Is it a parse5 thing? Is it part of the public > api or is it internal? It's part of parse5 API: https://github.com/inikulin/parse5#optionslocationinfo https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:78: if (!node.__location) { On 2015/07/06 17:06:45, dsinclair wrote: > nit: No {}'s on single line if statements. Done. https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:110: if (is_import_link) On 2015/07/06 17:06:45, dsinclair wrote: > Nit: Braces on multiline if statements. Done. https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:152: } else if (node.nodeName === 'script') { On 2015/07/06 17:06:45, dsinclair wrote: > Don't need the else's since each if returns. Done.
Sign in to reply to this message.
https://codereview.appspot.com/252880043/diff/130001/tracing/third_party/pars... File tracing/third_party/parse5/parse5_lib.js (right): https://codereview.appspot.com/252880043/diff/130001/tracing/third_party/pars... tracing/third_party/parse5/parse5_lib.js:3: global.parse5 = {}; +Dan: this is where parse5's module is exported to "global". The global's definition is done by browserify.
Sign in to reply to this message.
https://codereview.appspot.com/252880043/diff/110001/tracing/build/d8_bootstr... File tracing/build/d8_bootstrap.js (right): https://codereview.appspot.com/252880043/diff/110001/tracing/build/d8_bootstr... tracing/build/d8_bootstrap.js:245: ': File does not exist'); On 2015/07/06 19:00:57, nednguyen wrote: > On 2015/07/06 17:06:44, dsinclair wrote: > > Wouldn't it be better to output the actual error? Are there not other reasons > > for read to fail then just file doesn't exist? > > For some reason the stack trace messed up if we don't catch error thrown by > read. My guess the read() function does not throw new Error but just a string. I'm fine with catching the error. My question is, you say "File does not exist". Is that the _only_ error read() can produce? You have (err) above, why not ': ' + err? So you can see the real error string? https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... File tracing/build/html_to_js_generator.js (right): https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:33: this.content = content; On 2015/07/06 19:00:57, nednguyen wrote: > On 2015/07/06 17:06:45, dsinclair wrote: > > Internal members should have a trailing _. So, this.content_, etc. > > I intend to use this as "struct" rather than "class", so make all these members > "public". Is this an acceptable style? What is this in this context? The wrapping function or is it the global object? https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:152: } else if (node.nodeName === 'script') { On 2015/07/06 19:00:57, nednguyen wrote: > On 2015/07/06 17:06:45, dsinclair wrote: > > Don't need the else's since each if returns. > > Done. You also don't need the else on the above line either. The if() before it returns. https://codereview.appspot.com/252880043/diff/130001/tracing/third_party/pars... File tracing/third_party/parse5/parse5_lib.js (right): https://codereview.appspot.com/252880043/diff/130001/tracing/third_party/pars... tracing/third_party/parse5/parse5_lib.js:3: global.parse5 = {}; On 2015/07/06 22:54:06, nednguyen wrote: > +Dan: this is where parse5's module is exported to "global". The global's > definition is done by browserify. Can we make this global.js_parser or something so it isn't tied to parse5? parse5 is an implementation detail.
Sign in to reply to this message.
https://codereview.appspot.com/252880043/diff/110001/tracing/build/d8_bootstr... File tracing/build/d8_bootstrap.js (right): https://codereview.appspot.com/252880043/diff/110001/tracing/build/d8_bootstr... tracing/build/d8_bootstrap.js:245: ': File does not exist'); On 2015/07/07 14:33:06, dsinclair wrote: > On 2015/07/06 19:00:57, nednguyen wrote: > > On 2015/07/06 17:06:44, dsinclair wrote: > > > Wouldn't it be better to output the actual error? Are there not other > reasons > > > for read to fail then just file doesn't exist? > > > > For some reason the stack trace messed up if we don't catch error thrown by > > read. My guess the read() function does not throw new Error but just a string. > > > I'm fine with catching the error. My question is, you say "File does not exist". > Is that the _only_ error read() can produce? You have (err) above, why not ': ' > + err? So you can see the real error string? Ah, I see. Yeah, the only error read() produce is: "Error loading file", which I think is more cryptic than "File does not exists". https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... File tracing/build/html_to_js_generator.js (right): https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:33: this.content = content; On 2015/07/07 14:33:06, dsinclair wrote: > On 2015/07/06 19:00:57, nednguyen wrote: > > On 2015/07/06 17:06:45, dsinclair wrote: > > > Internal members should have a trailing _. So, this.content_, etc. > > > > I intend to use this as "struct" rather than "class", so make all these > members > > "public". Is this an acceptable style? > > > What is this in this context? The wrapping function or is it the global object? The wrapping function. In the code below, I do: chunk = new JsChunk(...); .. do stuffs to chunk.content; .. do stuffs to chunk.starting_location; ... This can also be achieve by: chunk = {content: , starting_location: ..., expected_starting_line:..} But I think having a light weight "struct" is better as it does data validation & make the API clear. Please lemme know if this is not a common js paradigm for this situation. :P https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:152: } else if (node.nodeName === 'script') { On 2015/07/07 14:33:06, dsinclair wrote: > On 2015/07/06 19:00:57, nednguyen wrote: > > On 2015/07/06 17:06:45, dsinclair wrote: > > > Don't need the else's since each if returns. > > > > Done. > > You also don't need the else on the above line either. The if() before it > returns. Done. https://codereview.appspot.com/252880043/diff/130001/tracing/third_party/pars... File tracing/third_party/parse5/parse5_lib.js (right): https://codereview.appspot.com/252880043/diff/130001/tracing/third_party/pars... tracing/third_party/parse5/parse5_lib.js:3: global.parse5 = {}; On 2015/07/07 14:33:06, dsinclair wrote: > On 2015/07/06 22:54:06, nednguyen wrote: > > +Dan: this is where parse5's module is exported to "global". The global's > > definition is done by browserify. > > > Can we make this global.js_parser or something so it isn't tied to parse5? > parse5 is an implementation detail. I am not sure whether this is a good idea since the APIs below is very specific to parse5. If we ever switch the implementation, I think it's better to be explicit about the naming of the library that offers these APIs, so the code reader of the files that use this can look it up easily.
Sign in to reply to this message.
https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... File tracing/build/html_to_js_generator.js (right): https://codereview.appspot.com/252880043/diff/110001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:33: this.content = content; On 2015/07/07 16:32:52, nednguyen wrote: > On 2015/07/07 14:33:06, dsinclair wrote: > > On 2015/07/06 19:00:57, nednguyen wrote: > > > On 2015/07/06 17:06:45, dsinclair wrote: > > > > Internal members should have a trailing _. So, this.content_, etc. > > > > > > I intend to use this as "struct" rather than "class", so make all these > > members > > > "public". Is this an acceptable style? > > > > > > What is this in this context? The wrapping function or is it the global > object? > > The wrapping function. In the code below, I do: > chunk = new JsChunk(...); > .. do stuffs to chunk.content; > .. do stuffs to chunk.starting_location; > > ... > This can also be achieve by: > chunk = {content: , starting_location: ..., expected_starting_line:..} > > But I think having a light weight "struct" is better as it does data validation > & make the API clear. > > Please lemme know if this is not a common js paradigm for this situation. :P Sorry, I wasn't clear. You do this.content = content. What is 'this' pointing to currently? The global? The function? The anonymous function?
Sign in to reply to this message.
https://codereview.appspot.com/252880043/diff/150001/tracing/build/d8_bootstr... File tracing/build/d8_bootstrap.js (right): https://codereview.appspot.com/252880043/diff/150001/tracing/build/d8_bootstr... tracing/build/d8_bootstrap.js:158: global.path_to_js_parser = '<%parse5_js_path%>'; nit: s/parse5_js_path/js_path/ https://codereview.appspot.com/252880043/diff/150001/tracing/build/d8_runner.py File tracing/build/d8_runner.py (right): https://codereview.appspot.com/252880043/diff/150001/tracing/build/d8_runner.... tracing/build/d8_runner.py:35: _PARSE5_JS_DIR = os.path.abspath( Let's call this JS_DIR https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... File tracing/build/html_to_js_generator.js (right): https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:10: * This file is pure js instead of an HTML imports module, I don't think this comment is needed. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:29: function JsChunk(content, starting_location, expected_starting_line) { Can this move into JsGenerator? https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:30: if (typeof(starting_location) !== 'number') if (!(starting_location instanceof Number)) should also work I believe and is probably clearer https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:41: function getNumLinesOfString(s) { Can this be moved into JsGenerator? https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:45: function generateJsFromJsChunks(chunks) { Why isn't this part of JsGenerator? https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:59: var new_lines = new Array(num_blank_lines_to_insert + 1).join(['\n']); Does this need the ['\n'] instead of just '\n'? https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:65: return results.join([]); [] -> '' https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:75: getStartingLocationForNode(node) { We don't prefix with get. So this should be startingLocationForNode. (and others below) https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:76: if (!node.__location) { nit: don't need {}'s (and below) https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:84: return; What does it mean if there is no __location? This should probably return something undefined at a minimum. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:86: return getNumLinesOfString(this.html_text_.substring(0, start)) + 1; Start is just used once, just use node.__location.start in the method call. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:90: if (!node.__location || !node.__location.startTag) { What does it mean for these to be missing? https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:96: getStartingLineNumberForNodeContent(node) { The naming of these is confusing. staringLocation vs startLineNumber. Can we name them to make the meanings more clear? https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:113: } Instead of the for() can you do: node.attributes.getNamedItem('rel') and 'href'? https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:114: if (is_import_link) { if (!is_import_link) return undefined; What does it mean if this doesn't return anythign? https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:128: if (src) {}'s around multiline if statements https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:133: }, This returns undefined if !src. Can you make that explicit. What does it mean if this returns undefined? https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:158: return this.generateJsChunksForScriptNode(node); Why does link return an array and script not? I would be better for generateJSChunkForLinkNode to return an array to be consistent with generateJsChunkForScriptNode as seeing the difference here is confusing. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:159: return null; This should be undefined. We don't use null. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:168: var chunks = this.generateJsChunksForNode(node); What happens if chunks is undefined?
Sign in to reply to this message.
https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... File tracing/build/html_to_js_generator.js (right): https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:10: * This file is pure js instead of an HTML imports module, On 2015/07/07 17:08:57, dsinclair wrote: > I don't think this comment is needed. Done. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:29: function JsChunk(content, starting_location, expected_starting_line) { On 2015/07/07 17:08:57, dsinclair wrote: > Can this move into JsGenerator? Done. Modify this to createJsChunk(...) which just returns a dict. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:30: if (typeof(starting_location) !== 'number') On 2015/07/07 17:08:57, dsinclair wrote: > if (!(starting_location instanceof Number)) should also work I believe and is > probably clearer Done. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:41: function getNumLinesOfString(s) { On 2015/07/07 17:08:56, dsinclair wrote: > Can this be moved into JsGenerator? Done. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:45: function generateJsFromJsChunks(chunks) { On 2015/07/07 17:08:56, dsinclair wrote: > Why isn't this part of JsGenerator? Done. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:59: var new_lines = new Array(num_blank_lines_to_insert + 1).join(['\n']); On 2015/07/07 17:08:57, dsinclair wrote: > Does this need the ['\n'] instead of just '\n'? '\n' works. Done. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:65: return results.join([]); On 2015/07/07 17:08:56, dsinclair wrote: > [] -> '' Done. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:75: getStartingLocationForNode(node) { On 2015/07/07 17:08:56, dsinclair wrote: > We don't prefix with get. So this should be startingLocationForNode. (and others > below) Done. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:76: if (!node.__location) { On 2015/07/07 17:08:56, dsinclair wrote: > nit: don't need {}'s (and below) Done. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:84: return; On 2015/07/07 17:08:57, dsinclair wrote: > What does it mean if there is no __location? If it's a node generated by parse5 but not present in the actual html. For example, parse5 generate html node even if the original text doesn't have <html> tags. > This should probably return something undefined at a minimum. Done. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:86: return getNumLinesOfString(this.html_text_.substring(0, start)) + 1; On 2015/07/07 17:08:56, dsinclair wrote: > Start is just used once, just use node.__location.start in the method call. Done. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:90: if (!node.__location || !node.__location.startTag) { On 2015/07/07 17:08:56, dsinclair wrote: > What does it mean for these to be missing? It meant that the node is self closing tag, e.g: <link ...> https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:113: } On 2015/07/07 17:08:56, dsinclair wrote: > Instead of the for() can you do: > > node.attributes.getNamedItem('rel') and 'href'? parse5 doesn't support those methods :-( https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:114: if (is_import_link) { On 2015/07/07 17:08:56, dsinclair wrote: > if (!is_import_link) > return undefined; > > What does it mean if this doesn't return anythign? Return empty array instead. If this doesn't return anything, the callsites should just ignore it. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:128: if (src) On 2015/07/07 17:08:57, dsinclair wrote: > {}'s around multiline if statements Done. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:133: }, On 2015/07/07 17:08:56, dsinclair wrote: > This returns undefined if !src. Can you make that explicit. What does it mean if > this returns undefined? the call site (generateJsChunksForScriptNode) should ignore this if it return undefined. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:158: return this.generateJsChunksForScriptNode(node); On 2015/07/07 17:08:56, dsinclair wrote: > Why does link return an array and script not? I would be better for > generateJSChunkForLinkNode to return an array to be consistent with > generateJsChunkForScriptNode as seeing the difference here is confusing. Done. I make it generateJSChunksForLinkNode return a list. The reason one returns a list & other returns a single node is because <script src="foo.js"> var x = 1; </script> can return 2 JsChunks, whereas <link import=...> must always return 1 jschunk as most. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:159: return null; On 2015/07/07 17:08:57, dsinclair wrote: > This should be undefined. We don't use null. Return an empty array here instead. https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:168: var chunks = this.generateJsChunksForNode(node); On 2015/07/07 17:08:57, dsinclair wrote: > What happens if chunks is undefined? Oops, it should only return a list. See my comment above.
Sign in to reply to this message.
https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... File tracing/build/html_to_js_generator.js (right): https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:96: getStartingLineNumberForNodeContent(node) { On 2015/07/07 17:08:57, dsinclair wrote: > The naming of these is confusing. staringLocation vs startLineNumber. Can we > name them to make the meanings more clear? Renamed. Does the new names look ok to you?
Sign in to reply to this message.
https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... File tracing/build/html_to_js_generator.js (right): https://codereview.appspot.com/252880043/diff/150001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:10: * This file is pure js instead of an HTML imports module, On 2015/07/07 18:26:33, nednguyen wrote: > On 2015/07/07 17:08:57, dsinclair wrote: > > I don't think this comment is needed. > > Done. I don't see any changes? Was this not done, or do you think the comment is useful? https://codereview.appspot.com/252880043/diff/230001/tracing/build/html_to_js... File tracing/build/html_to_js_generator.js (right): https://codereview.appspot.com/252880043/diff/230001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:45: if (!start_location instanceof Number) { I'm pretty sure this has to be if (!(start_location instanceof Number)) in order to work correctly. https://codereview.appspot.com/252880043/diff/230001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:65: generateJsFromJsChunks(chunks) { Move this down with the generate methods below. https://codereview.appspot.com/252880043/diff/230001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:124: if (is_import_link) { if (!is_import_link) return []; https://codereview.appspot.com/252880043/diff/230001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:140: if (src) { if (!src) return undefined;
Sign in to reply to this message.
https://codereview.appspot.com/252880043/diff/230001/tracing/build/html_to_js... File tracing/build/html_to_js_generator.js (right): https://codereview.appspot.com/252880043/diff/230001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:45: if (!start_location instanceof Number) { On 2015/07/07 18:51:56, dsinclair wrote: > I'm pretty sure this has to be if (!(start_location instanceof Number)) in order > to work correctly. Ooops, actually after switching this to the right syntax, I discover that "1 instanceof Number" evaluates to false. http://stackoverflow.com/questions/472418/why-is-4-not-an-instance-of-number https://codereview.appspot.com/252880043/diff/230001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:65: generateJsFromJsChunks(chunks) { On 2015/07/07 18:51:56, dsinclair wrote: > Move this down with the generate methods below. Done. https://codereview.appspot.com/252880043/diff/230001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:124: if (is_import_link) { On 2015/07/07 18:51:55, dsinclair wrote: > if (!is_import_link) > return []; Done. https://codereview.appspot.com/252880043/diff/230001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:140: if (src) { On 2015/07/07 18:51:56, dsinclair wrote: > if (!src) > return undefined; Done.
Sign in to reply to this message.
lgtm w/ nit https://codereview.appspot.com/252880043/diff/250001/tracing/build/html_to_js... File tracing/build/html_to_js_generator.js (right): https://codereview.appspot.com/252880043/diff/250001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:140: return undefined; Should this return [] to be consistent with generateJsChunksForLinkNode above?
Sign in to reply to this message.
https://codereview.appspot.com/252880043/diff/250001/tracing/build/html_to_js... File tracing/build/html_to_js_generator.js (right): https://codereview.appspot.com/252880043/diff/250001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:140: return undefined; On 2015/07/07 20:02:56, dsinclair wrote: > Should this return [] to be consistent with generateJsChunksForLinkNode above? the generateJsChunkForScriptTag & generateJsChunkForScriptText are both supposed to return just a single chunk or undefined.
Sign in to reply to this message.
https://codereview.appspot.com/252880043/diff/250001/tracing/build/html_to_js... File tracing/build/html_to_js_generator.js (right): https://codereview.appspot.com/252880043/diff/250001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:140: return undefined; On 2015/07/07 20:09:43, nednguyen wrote: > On 2015/07/07 20:02:56, dsinclair wrote: > > Should this return [] to be consistent with generateJsChunksForLinkNode above? > > the generateJsChunkForScriptTag & generateJsChunkForScriptText are both supposed > to return just a single chunk or undefined. It's weird that these return undefined and generateJsChunksForLinkNode returns [].
Sign in to reply to this message.
https://codereview.appspot.com/252880043/diff/250001/tracing/build/html_to_js... File tracing/build/html_to_js_generator.js (right): https://codereview.appspot.com/252880043/diff/250001/tracing/build/html_to_js... tracing/build/html_to_js_generator.js:140: return undefined; On 2015/07/07 20:11:09, dsinclair wrote: > On 2015/07/07 20:09:43, nednguyen wrote: > > On 2015/07/07 20:02:56, dsinclair wrote: > > > Should this return [] to be consistent with generateJsChunksForLinkNode > above? > > > > the generateJsChunkForScriptTag & generateJsChunkForScriptText are both > supposed > > to return just a single chunk or undefined. > > It's weird that these return undefined and generateJsChunksForLinkNode returns > []. Done. I switched all the generateJsChunks.. method to return array.
Sign in to reply to this message.
Message was sent while issue was closed.
Committed patchset #7 (id:290001) manually as b734ae94ff81ed6de98648db8f7357b9129d3939 (presubmit successful).
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
