Motivation: Now timeline tries to save all data with one chunk. Sometimes this causes "IPC message to big" error. Solution: Refactor Profiles/Heap code - extract chunked IO and reuse it.
Created attachment 153790 [details] Patch refactoring-preview
Comment on attachment 153790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153790&action=review > Source/WebCore/WebCore.gypi:6302 > + 'inspector/front-end/FileUtils.js', please fix WebKit.qrc and WebCore.vcproj files too
Comment on attachment 153790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153790&action=review Looks like the existing implementation has bad design. Could you please fix it (replace the callbacks with delegate interface) in the first patch and extract the code into separate file in the second or vice versa. > Source/WebCore/inspector/front-end/FileUtils.js:101 > + * @param {function(WebInspector.HeapSnapshotProxy)=} callback please remove this > Source/WebCore/inspector/front-end/FileUtils.js:110 > + return true; why do we need a return value here
Comment on attachment 153790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153790&action=review >> Source/WebCore/inspector/front-end/FileUtils.js:101 >> + * @param {function(WebInspector.HeapSnapshotProxy)=} callback > > please remove this Fixed in https://bugs.webkit.org/show_bug.cgi?id=92348 Please take a look.
Comment on attachment 153790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153790&action=review > Source/WebCore/ChangeLog:9 > + Sometimes this causes "IPC message to big" error. Wrong indent > Source/WebCore/ChangeLog:11 > + Solution: Refactor Profiles/Heap code - extract chunked IO and reuse it. Now that you are describing your solution, it's a good idea to mention the FileUtils and quickly cover its contents. > Source/WebCore/ChangeLog:19 > + Ditto. Please remove non-meaningful comments, as they obscure the changelog entry contents. You can remove all of these along with added (WebInspector.*.prototype.*) entries in FileUtils.js (so, retain only the new type names). I believe this was mentioned about a month ago in the webkit-dev mailing list thread named "Please include function-level comments in change log entries". Ultimately, your goal is NOT commenting every entry added by webkit-patch but creating a changelog that will help others understand what your patch does. > Source/WebCore/ChangeLog:70 > + (WebInspector.HeapSnapshotLoader.prototype.startTransfer): You don't need these entries > Source/WebCore/ChangeLog:87 > + (WebInspector.HeapProfileHeader.prototype.load): Ditto > Source/WebCore/ChangeLog:115 > + (WebInspector.ProfilesPanel.prototype._createFileSelectorElement): Ditto > Source/WebCore/inspector/front-end/FileUtils.js:34 > +WebInspector.ChunkedTransferer = function() ChunkedTransferrer. But I believe a more meaningful name is needed, if possible. > Source/WebCore/inspector/front-end/FileUtils.js:42 > + * @ What is this line here for? > Source/WebCore/inspector/front-end/FileUtils.js:56 > + * @param {function(*)=} callback Can you make the parameter/return types more specific? > Source/WebCore/inspector/front-end/FileUtils.js:63 > + dispose: function() What is this method necessary for? It has no non-trivial implementations. > Source/WebCore/inspector/front-end/FileUtils.js:80 > + onChunkTransfered: function() onChunkTransferred > Source/WebCore/inspector/front-end/FileUtils.js:148 > + this._delegate && this._delegate.onTransferStarted(); Please avoid these hacky shortcuts > Source/WebCore/inspector/front-end/FileUtils.js:160 > + this._delegate && this._delegate.onChunkTransfered(); ditto > Source/WebCore/inspector/front-end/FileUtils.js:268 > + lastIndex = lastIndex ? lastIndex : source.length; lastIndex = lastIndex || source.length; (this is one of a few acceptable, JS-specific shortcuts) > Source/WebCore/inspector/front-end/FileUtils.js:271 > + for (var index = startIndex ? startIndex : 0; index < lastIndex; ++index) { ... = startIndex || 0;... > Source/WebCore/inspector/front-end/FileUtils.js:272 > + var character = source.charCodeAt(index); A good rule is to choose variable names that reflect the data they hold. In this case, |character| holds not a real character but its charCode (a number). I understand that this is just code motion, but refactoring is a good reason (and the most common one :)) to clean up the code. > Source/WebCore/inspector/front-end/HeapSnapshotView.js:987 > +WebInspector.HeapSnapshotLoadFileTransferer = function(snapshotHeader, fileReader) ...Transferrer > LayoutTests/ChangeLog:8 > + Motivation: Now timeline tries to save all data with one chunk. LayoutTests ChangeLog entry requires only treatment of changes relevant to the modified tests, as the timeline issue is fixed in Source/WebCore > LayoutTests/inspector/profiler/heap-snapshot-loader.html:112 > + }, Trailing comma
Comment on attachment 153790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153790&action=review >> Source/WebCore/ChangeLog:9 >> + Sometimes this causes "IPC message to big" error. > > Wrong indent Got it. >> Source/WebCore/ChangeLog:11 >> + Solution: Refactor Profiles/Heap code - extract chunked IO and reuse it. > > Now that you are describing your solution, it's a good idea to mention the FileUtils and quickly cover its contents. OK. >> Source/WebCore/ChangeLog:19 >> + Ditto. > > Please remove non-meaningful comments, as they obscure the changelog entry contents. You can remove all of these along with added (WebInspector.*.prototype.*) entries in FileUtils.js (so, retain only the new type names). I believe this was mentioned about a month ago in the webkit-dev mailing list thread named "Please include function-level comments in change log entries". > > Ultimately, your goal is NOT commenting every entry added by webkit-patch but creating a changelog that will help others understand what your patch does. Done. Thanks. >> Source/WebCore/ChangeLog:70 >> + (WebInspector.HeapSnapshotLoader.prototype.startTransfer): > > You don't need these entries Fixed. >> Source/WebCore/ChangeLog:87 >> + (WebInspector.HeapProfileHeader.prototype.load): > > Ditto Fixed. >> Source/WebCore/ChangeLog:115 >> + (WebInspector.ProfilesPanel.prototype._createFileSelectorElement): > > Ditto Fixed. >> Source/WebCore/WebCore.gypi:6302 >> + 'inspector/front-end/FileUtils.js', > > please fix WebKit.qrc and WebCore.vcproj files too Fixed. >> Source/WebCore/inspector/front-end/FileUtils.js:34 >> +WebInspector.ChunkedTransferer = function() > > ChunkedTransferrer. But I believe a more meaningful name is needed, if possible. Fixed. >> Source/WebCore/inspector/front-end/FileUtils.js:42 >> + * @ > > What is this line here for? Typo. Fixed. >> Source/WebCore/inspector/front-end/FileUtils.js:56 >> + * @param {function(*)=} callback > > Can you make the parameter/return types more specific? Callbacks are removed in new patch. >> Source/WebCore/inspector/front-end/FileUtils.js:63 >> + dispose: function() > > What is this method necessary for? It has no non-trivial implementations. HeapSnapshotLoader has. >> Source/WebCore/inspector/front-end/FileUtils.js:80 >> + onChunkTransfered: function() > > onChunkTransferred fixed >> Source/WebCore/inspector/front-end/FileUtils.js:110 >> + return true; > > why do we need a return value here Fixed >> Source/WebCore/inspector/front-end/FileUtils.js:148 >> + this._delegate && this._delegate.onTransferStarted(); > > Please avoid these hacky shortcuts OK >> Source/WebCore/inspector/front-end/FileUtils.js:160 >> + this._delegate && this._delegate.onChunkTransfered(); > > ditto Done. >> Source/WebCore/inspector/front-end/FileUtils.js:268 >> + lastIndex = lastIndex ? lastIndex : source.length; > > lastIndex = lastIndex || source.length; (this is one of a few acceptable, JS-specific shortcuts) Fixed. >> Source/WebCore/inspector/front-end/FileUtils.js:271 >> + for (var index = startIndex ? startIndex : 0; index < lastIndex; ++index) { > > ... = startIndex || 0;... Thanks. >> Source/WebCore/inspector/front-end/FileUtils.js:272 >> + var character = source.charCodeAt(index); > > A good rule is to choose variable names that reflect the data they hold. In this case, |character| holds not a real character but its charCode (a number). > > I understand that this is just code motion, but refactoring is a good reason (and the most common one :)) to clean up the code. Fixed, thanks. >> Source/WebCore/inspector/front-end/HeapSnapshotView.js:987 >> +WebInspector.HeapSnapshotLoadFileTransferer = function(snapshotHeader, fileReader) > > ...Transferrer Fixed. >> LayoutTests/ChangeLog:8 >> + Motivation: Now timeline tries to save all data with one chunk. > > LayoutTests ChangeLog entry requires only treatment of changes relevant to the modified tests, as the timeline issue is fixed in Source/WebCore OK. >> LayoutTests/inspector/profiler/heap-snapshot-loader.html:112 >> + }, > > Trailing comma fixed.
Created attachment 156334 [details] Patch
Comment on attachment 156334 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156334&action=review > Source/WebCore/inspector/front-end/FileUtils.js:198 > + var charCode = source.charCodeAt(index); > + if (charCode === openingBracket) > + ++counter; > + else if (charCode === closingBracket) { > + if (--counter === 0) > + return index + 1; > + } so what if we have brackets within strings?
Comment on attachment 156334 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156334&action=review >> Source/WebCore/inspector/front-end/FileUtils.js:198 >> + } > > so what if we have brackets within strings? fixed
Created attachment 156538 [details] Patch
Comment on attachment 156538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156538&action=review > Source/WebCore/inspector/front-end/FileUtils.js:193 > + if (character === '"' && source[index - 1] !== '\\') What is the performance impact of using character directly instead of its char code for comparison as it used to be? > Source/WebCore/inspector/front-end/TimelineModel.js:167 > + /* TODO: Prevent model recording/reset/etc during loading. */ TODO -> FIXME > Source/WebCore/inspector/front-end/TimelineModel.js:302 > + /* TODO: Show progress indicator */ TODO -> FIXME > Source/WebCore/inspector/front-end/TimelineModel.js:390 > + writer.transferChunk(data.join(",\n")); This way you'll miss ",\n" between records in consecutive chunks.
Comment on attachment 156538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156538&action=review >> Source/WebCore/inspector/front-end/FileUtils.js:193 >> + if (character === '"' && source[index - 1] !== '\\') > > What is the performance impact of using character directly instead of its char code for comparison as it used to be? So how about "\\"?
Comment on attachment 156538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156538&action=review >>> Source/WebCore/inspector/front-end/FileUtils.js:193 >>> + if (character === '"' && source[index - 1] !== '\\') >> >> What is the performance impact of using character directly instead of its char code for comparison as it used to be? > > So how about "\\"? @Yury: On synthetic tests direct comparison to char works somewhat faster. @Andrey: Oh, I missed this case =( will be fixed in next patch (est. 20 Aug) >> Source/WebCore/inspector/front-end/TimelineModel.js:390 >> + writer.transferChunk(data.join(",\n")); > > This way you'll miss ",\n" between records in consecutive chunks. Not sure we'll suffer of that, but I'll fix it in next patch.
Created attachment 159638 [details] Patch
Comment on attachment 159638 [details] Patch Attachment 159638 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13542700 New failing tests: inspector/timeline/timeline-load.html inspector/timeline/timeline-load-incompatible.html
Created attachment 159647 [details] Archive of layout-test-results from gce-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 159638 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159638&action=review > Source/WebCore/inspector/front-end/FileUtils.js:97 > + this._isCanceled = false; I think ultimately we just need to pass progress to the file reader. > Source/WebCore/inspector/front-end/FileUtils.js:215 > + if (inString) { > + if (character === '\\') > + ++index; > + else if (character === '"') > + inString = false; > + } else { > + if (character === '"') > + inString = true; > + else if (character === '{') > + ++counter; > + else if (character === '}') { > + if (--counter === 0) > + return index + 1; > + } > + } use switch? > Source/WebCore/inspector/front-end/TimelineModel.js:178 > + return new WebInspector.ChunkedFileReader(file, 5000000, delegate); extract a const for 5000000? > Source/WebCore/inspector/front-end/TimelineModel.js:245 > + this._json = ''; s/_json/_buffer/? Also, double quotes please. > Source/WebCore/inspector/front-end/TimelineModel.js:270 > + while (true) { > + lastIndex = WebInspector.findBalancedCurlyBrackets(data, index); > + if (lastIndex != -1) > + index = lastIndex; > + else > + break; > + } do { index = lastIndex; lastIndex = WebInspector.findBalancedCurlyBrackets(data, index); } while (lastIndex !== -1) > Source/WebCore/inspector/front-end/TimelineModel.js:271 > + var json = data.slice(0, index) + ']'; Double quotes, please. > Source/WebCore/inspector/front-end/TimelineModel.js:275 > + json = '[0' + json; This [0 thingie looks like a hack. Also, double quotes please. > Source/WebCore/inspector/front-end/TimelineModel.js:346 > + case event.target.error.NOT_FOUND_ERR: > + WebInspector.log(WebInspector.UIString('Timeline.loadFromFile: File "%s" not found.', reader.fileName())); > + break; > + case event.target.error.NOT_READABLE_ERR: > + WebInspector.log(WebInspector.UIString('Timeline.loadFromFile: File "%s" is not readable', reader.fileName())); > + break; > + case event.target.error.ABORT_ERR: > + break; > + default: > + WebInspector.log(WebInspector.UIString('Timeline.loadFromFile: An error occurred while reading the file "%s"', reader.fileName())); Please note Pavel has just changed the text for these error messages and changed the way we report errors. See bug 91528 for details. Also, please indent breaks. I would also alias event.target.error to var error for brevity. > Source/WebCore/inspector/front-end/TimelineModel.js:361 > + this._prologue = '[' + JSON.stringify(new String(version)); why do you need new String()? Also, double quotes on strings please. > Source/WebCore/inspector/front-end/TimelineModel.js:382 > + onTransferFinished: function(writer) { > + }, > + > + onError: function(writer, event) { > + }, style: empty functions should be like function foo() { } > Source/WebCore/inspector/front-end/TimelineModel.js:384 > + _pushChunk: function (writer) function (writer) => function(writer) > Source/WebCore/inspector/front-end/TimelineModel.js:395 > + data.push(''); so we'll have an empty line at the beginning of each chunk and an empty line at the end of each chunk. is that the intent? > Source/WebCore/inspector/front-end/TimelineModel.js:400 > + var itemLength = item.length + 2; + 2? > Source/WebCore/inspector/front-end/TimelineModel.js:407 > + if (this._recordIndex == this._records.length) { === > LayoutTests/inspector/timeline/timeline-load.html:74 > + _loadedSize: 0, > + _fileSize: stringifiedData.length, > + > + start: function(output) > + { > + output.startTransfer(); > + delegate.onTransferStarted(this); > + > + var chunk = stringifiedData; > + this._loadedSize += chunk.length; > + output.transferChunk(chunk); > + delegate.onChunkTransferred(this); > + > + output.finishTransfer(); > + delegate.onTransferFinished(this); > + > + model.saveToFile(); > + }, > + > + cancel: function() > + { > + }, > + > + loadedSize: function() > + { > + return this._loadedSize; > + }, > + > + fileSize: function() > + { > + return this._fileSize; > + }, > + > + fileName: function() > + { > + return 'fakeFile'; > + } Please avoid duplicating such large pieces of code. > LayoutTests/inspector/timeline/timeline-load.html:100 > + startTransfer: function() > + { > + delegate.onTransferStarted(this); > + }, > + > + transferChunk: function(chunk) > + { > + saveData += chunk; > + delegate.onChunkTransferred(this); > + }, > + > + finishTransfer: function() > + { > + delegate.onTransferFinished(this); > + checkSaveData(); > + }, > + > + dispose: function() > + { > + }, -ditto- Please avoid duplicating such large pieces of code.
Comment on attachment 159638 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159638&action=review >> Source/WebCore/inspector/front-end/FileUtils.js:97 >> + this._isCanceled = false; > > I think ultimately we just need to pass progress to the file reader. I think it is better to keep IO and UI distinct. IMHO we could introduce "Cancelable" interface, make "Progress" implement it and pass "Progress" downcasted to "Cancelable". >> Source/WebCore/inspector/front-end/FileUtils.js:215 >> + } > > use switch? Currently switch is 3 times slower. >> Source/WebCore/inspector/front-end/TimelineModel.js:178 >> + return new WebInspector.ChunkedFileReader(file, 5000000, delegate); > > extract a const for 5000000? Done >> Source/WebCore/inspector/front-end/TimelineModel.js:245 >> + this._json = ''; > > s/_json/_buffer/? > Also, double quotes please. Done >> Source/WebCore/inspector/front-end/TimelineModel.js:270 >> + } > > do { > index = lastIndex; > lastIndex = WebInspector.findBalancedCurlyBrackets(data, index); > } while (lastIndex !== -1) OK >> Source/WebCore/inspector/front-end/TimelineModel.js:271 >> + var json = data.slice(0, index) + ']'; > > Double quotes, please. Fixed >> Source/WebCore/inspector/front-end/TimelineModel.js:275 >> + json = '[0' + json; > > This [0 thingie looks like a hack. > Also, double quotes please. Done >> Source/WebCore/inspector/front-end/TimelineModel.js:346 >> + WebInspector.log(WebInspector.UIString('Timeline.loadFromFile: An error occurred while reading the file "%s"', reader.fileName())); > > Please note Pavel has just changed the text for these error messages and changed the way we report errors. See bug 91528 for details. > Also, please indent breaks. > I would also alias event.target.error to var error for brevity. Fixed. >> Source/WebCore/inspector/front-end/TimelineModel.js:361 >> + this._prologue = '[' + JSON.stringify(new String(version)); > > why do you need new String()? > Also, double quotes on strings please. I don't know the real reason. It was so as long as history can be tracked (about 13 months). >> Source/WebCore/inspector/front-end/TimelineModel.js:382 >> + }, > > style: empty functions should be like function foo() { } Fixed >> Source/WebCore/inspector/front-end/TimelineModel.js:384 >> + _pushChunk: function (writer) > > function (writer) => function(writer) fixed >> Source/WebCore/inspector/front-end/TimelineModel.js:395 >> + data.push(''); > > so we'll have an empty line at the beginning of each chunk and an empty line at the end of each chunk. is that the intent? Ooops. Broke this under the influence of Yury's comment. >> Source/WebCore/inspector/front-end/TimelineModel.js:400 >> + var itemLength = item.length + 2; > > + 2? separator length. fixed. >> LayoutTests/inspector/timeline/timeline-load.html:74 >> + } > > Please avoid duplicating such large pieces of code. Done.
Created attachment 160090 [details] Patch
Comment on attachment 160090 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160090&action=review > Source/WebCore/inspector/front-end/FileUtils.js:225 > + if (data.length == 0) if (!data.length) > Source/WebCore/inspector/front-end/FileUtils.js:303 > +WebInspector.ChunkedFileWriter = function(fileName, delegate) FileOutputStream? > Source/WebCore/inspector/front-end/TimelineModel.js:289 > + if (index === 0) if (!index) > Source/WebCore/inspector/front-end/TimelineModel.js:436 > + if (this._recordIndex == this._records.length) { > + data.push(data.pop() + "]"); > + } drop {} > LayoutTests/inspector/timeline/timeline-load.html:18 > + return { > + _loadedSize: 0, > + _fileSize: input.length, > + Can this be written as our typical class definition, with constructor and prototype?
(In reply to comment #18) > (From update of attachment 159638 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159638&action=review > > >> Source/WebCore/inspector/front-end/FileUtils.js:97 > >> + this._isCanceled = false; > > > > I think ultimately we just need to pass progress to the file reader. > > I think it is better to keep IO and UI distinct. > IMHO we could introduce "Cancelable" interface, make "Progress" implement it and pass "Progress" downcasted to "Cancelable". Progress is not a UI-level concept, it's just an interface (ProgressIndicator is the UI component). The idea is that besides being able to canel the operation, we often also want to track its progress.
Comment on attachment 160090 [details] Patch Attachment 160090 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13558624 New failing tests: inspector/timeline/timeline-load.html
Created attachment 160096 [details] Archive of layout-test-results from gce-cr-linux-07 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 160090 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160090&action=review >> Source/WebCore/inspector/front-end/FileUtils.js:225 >> + if (data.length == 0) > > if (!data.length) fixed >> Source/WebCore/inspector/front-end/FileUtils.js:303 >> +WebInspector.ChunkedFileWriter = function(fileName, delegate) > > FileOutputStream? Sounds good. >> Source/WebCore/inspector/front-end/TimelineModel.js:289 >> + if (index === 0) > > if (!index) fixed >> Source/WebCore/inspector/front-end/TimelineModel.js:436 >> + } > > drop {} fixed >> LayoutTests/inspector/timeline/timeline-load.html:18 >> + > > Can this be written as our typical class definition, with constructor and prototype? Done
Created attachment 160129 [details] Patch
Comment on attachment 160129 [details] Patch Attachment 160129 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13560665 New failing tests: inspector/timeline/timeline-load.html
Created attachment 160134 [details] Archive of layout-test-results from gce-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 160358 [details] Patch examine-objects-on-bot
Comment on attachment 160358 [details] Patch Attachment 160358 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13600061 New failing tests: inspector/timeline/timeline-load.html
Created attachment 160367 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 160371 [details] Patch
Comment on attachment 160371 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160371&action=review > Source/WebCore/inspector/front-end/FileUtils.js:286 > + else if (character === '{') "{" > Source/WebCore/inspector/front-end/FileUtils.js:288 > + else if (character === '}') { ditto > Source/WebCore/inspector/front-end/TimelineModel.js:361 > + onError: function(reader, event) { { => next line > Source/WebCore/inspector/front-end/TimelineModel.js:423 > + } else { > + data.push(""); > + } no need for {} > LayoutTests/inspector/timeline/timeline-load.html:18 > + function callback() { > + model.saveToFile(); > + } { needs to be on its own line. I think you can inline or bind this, though. > LayoutTests/inspector/timeline/timeline-load.html:35 > + if (JSON.stringify(expectedOutput) == JSON.stringify(saveData)) > + InspectorTest.addResult("passed"); > + else > + InspectorTest.addResult("saved data is not equal to restored"); Use InspectorTest.assertEquals() > LayoutTests/inspector/timeline/timeline-load.html:43 > + var mockProgress = new WebInspector.Progress(); This could be out of try block. > LayoutTests/inspector/timeline/timeline-load.html:48 > + } catch (e) { > + InspectorTest.addResult("failed"); > + InspectorTest.completeTest(); > + } Doesn't test harness take care of this? > LayoutTests/inspector/timeline/timeline-test.js:175 > + var half = Math.ceil(length / 2); (length + 1) >> 1
Comment on attachment 160371 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160371&action=review >> Source/WebCore/inspector/front-end/FileUtils.js:286 >> + else if (character === '{') > > "{" Done >> Source/WebCore/inspector/front-end/FileUtils.js:288 >> + else if (character === '}') { > > ditto Done >> Source/WebCore/inspector/front-end/TimelineModel.js:361 >> + onError: function(reader, event) { > > { => next line fixed >> Source/WebCore/inspector/front-end/TimelineModel.js:423 >> + } > > no need for {} fixed >> LayoutTests/inspector/timeline/timeline-load.html:18 >> + } > > { needs to be on its own line. I think you can inline or bind this, though. Done >> LayoutTests/inspector/timeline/timeline-load.html:35 >> + InspectorTest.addResult("saved data is not equal to restored"); > > Use InspectorTest.assertEquals() Done >> LayoutTests/inspector/timeline/timeline-load.html:48 >> + } > > Doesn't test harness take care of this? Removed >> LayoutTests/inspector/timeline/timeline-test.js:175 >> + var half = Math.ceil(length / 2); > > (length + 1) >> 1 Done
Created attachment 160695 [details] Patch
Comment on attachment 160695 [details] Patch LGTM.
Comment on attachment 160695 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160695&action=review > Source/WebCore/ChangeLog:9 > + Sometimes this causes "IPC message to big" error. typo: to -> too > Source/WebCore/inspector/front-end/FileUtils.js:161 > + * @param {!string} url "Functions and all value types (boolean, number, and string) are non-nullable by default whether or not they are declared with the Non-nullable operator." (https://developers.google.com/closure/compiler/docs/js-for-compiler#nonnull) > Source/WebCore/inspector/front-end/FileUtils.js:230 > + if (this._isCanceled) Does this check imply that this._output.transferChunk() can cancel the transfer? This doesn't look consistent: the data is transferred in any case but the event is not dispatched in all cases. > Source/WebCore/inspector/front-end/TimelineModel.js:46 > +WebInspector.TimelineModel.TransferChunkLength = 5000000; Having the "Bytes" suffix would be awesome. > Source/WebCore/inspector/front-end/TimelineModel.js:284 > + } while (lastIndex != -1) !== > Source/WebCore/inspector/front-end/TimelineModel.js:292 > + // Prepending "0" to turn string to valid JSON. typo: to -> into > Source/WebCore/inspector/front-end/TimelineModel.js:313 > + for (var i = 1, l = items.length; i < l; ++i) We try to abstain from using "l" as the variable name at all times, which is a general coding practice. You can use "size" instead. > Source/WebCore/inspector/front-end/TimelineModel.js:366 > + switch(error.code) { Whitespace after "switch" > Source/WebCore/inspector/front-end/TimelineModel.js:434 > + if (this._recordIndex == this._records.length) === > LayoutTests/ChangeLog:10 > + * inspector/timeline/timeline-load.html: Turned to test suite to avoid code typo: to -> into > LayoutTests/inspector/timeline/timeline-load.html:95 > +<a href="https://bugs.webkit.org/show_bug.cgi?id=91714">Bug 91714</a> hint: This is usually appended to the test description ("Also tests...") in parentheses on the same line to avoid adding new lines to the golden expectation.
Comment on attachment 160695 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160695&action=review > Source/WebCore/inspector/front-end/TimelineModel.js:367 > + case error.NOT_FOUND_ERR: After a talk with Andrey, we decided to use the FileError object directly for this switch (so, "FileError.NOT_FOUND_ERR" and so on) to make the origin of these errors a bit more clear.
Comment on attachment 160695 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160695&action=review >> Source/WebCore/ChangeLog:9 >> + Sometimes this causes "IPC message to big" error. > > typo: to -> too fixed >> Source/WebCore/inspector/front-end/FileUtils.js:161 >> + * @param {!string} url > > "Functions and all value types (boolean, number, and string) are non-nullable by default whether or not they are declared with the Non-nullable operator." (https://developers.google.com/closure/compiler/docs/js-for-compiler#nonnull) OK >> Source/WebCore/inspector/front-end/FileUtils.js:230 >> + if (this._isCanceled) > > Does this check imply that this._output.transferChunk() can cancel the transfer? This doesn't look consistent: the data is transferred in any case but the event is not dispatched in all cases. That is not stated, but the contract is "do not bother delegate after cancel() is invoked". I.e. the party that calls "cancel()" is responsible for proper cancellation of things that are not controlled by reader. And, yes, transfer can be canceled by this._output.transferChunk() >> Source/WebCore/inspector/front-end/TimelineModel.js:46 >> +WebInspector.TimelineModel.TransferChunkLength = 5000000; > > Having the "Bytes" suffix would be awesome. Done. Thanks. >> Source/WebCore/inspector/front-end/TimelineModel.js:284 >> + } while (lastIndex != -1) > > !== Fixed. >> Source/WebCore/inspector/front-end/TimelineModel.js:292 >> + // Prepending "0" to turn string to valid JSON. > > typo: to -> into Fixed. >> Source/WebCore/inspector/front-end/TimelineModel.js:313 >> + for (var i = 1, l = items.length; i < l; ++i) > > We try to abstain from using "l" as the variable name at all times, which is a general coding practice. You can use "size" instead. Done. Thanks. >> Source/WebCore/inspector/front-end/TimelineModel.js:366 >> + switch(error.code) { > > Whitespace after "switch" Fixed >> Source/WebCore/inspector/front-end/TimelineModel.js:434 >> + if (this._recordIndex == this._records.length) > > === Done >> LayoutTests/ChangeLog:10 >> + * inspector/timeline/timeline-load.html: Turned to test suite to avoid code > > typo: to -> into Fixed >> LayoutTests/inspector/timeline/timeline-load.html:95 >> +<a href="https://bugs.webkit.org/show_bug.cgi?id=91714">Bug 91714</a> > > hint: This is usually appended to the test description ("Also tests...") in parentheses on the same line to avoid adding new lines to the golden expectation. Fixed
Created attachment 161182 [details] Patch
Created attachment 161192 [details] Patch
Created attachment 161219 [details] Patch
Comment on attachment 161219 [details] Patch r=me
Comment on attachment 161219 [details] Patch Clearing flags on attachment: 161219 Committed r127039: <http://trac.webkit.org/changeset/127039>
All reviewed patches have been landed. Closing bug.