%subj%
Created attachment 100992 [details] [patch] initial version
Comment on attachment 100992 [details] [patch] initial version View in context: https://bugs.webkit.org/attachment.cgi?id=100992&action=review > Source/WebCore/inspector/front-end/TimelinePanel.js:211 > + this.saveAsButton = new WebInspector.StatusBarButton(WebInspector.UIString("Save as..."), "save-as-status-bar-item"); saveAsButton should be private > Source/WebCore/inspector/front-end/TimelinePanel.js:214 > + this.loadButton = new WebInspector.StatusBarButton(WebInspector.UIString("Load from..."), "load-from-status-bar-item"); Ditto. > Source/WebCore/inspector/front-end/TimelinePanel.js:247 > + nextOffset = data.indexOf("\n", offset); This code assumes that input data is not a simple JSON array but an array with additional line breaks which looks odd. > Source/WebCore/inspector/front-end/TimelinePanel.js:289 > + InspectorFrontendHost.saveAs("TimelineRawData-" + date + ".json", records.join("\n")); Output format should be covered with a test, r- for this.
Created attachment 101008 [details] [patch] second version
Comment on attachment 101008 [details] [patch] second version View in context: https://bugs.webkit.org/attachment.cgi?id=101008&action=review > LayoutTests/inspector/timeline/timeline-load.html:16 > + InspectorTest.addSniffer(WebInspector.TimelinePanel.prototype, "_createFileSelector", loaded); _createFileSelector could be renamed to _loadFromFileComplete for better readability.
Created attachment 101135 [details] [patch] third version
Created attachment 101137 [details] [patch] third version with binary changes.
Comment on attachment 101137 [details] [patch] third version with binary changes. View in context: https://bugs.webkit.org/attachment.cgi?id=101137&action=review > Source/WebCore/inspector/front-end/inspector.css:3876 > + -webkit-mask-position: -64px -48px; Could you please attach the image?
Created attachment 101140 [details] screenshot
I'd say we should have "Export" and "Import" in context menu + support drag'n'drop both ways instead.
Created attachment 101168 [details] [patch] fourth version with context menu
Comment on attachment 101168 [details] [patch] fourth version with context menu Attachment 101168 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9110471 New failing tests: inspector/timeline/timeline-load.html
Created attachment 101276 [details] [patch] fourth version with fixed test
Comment on attachment 101276 [details] [patch] fourth version with fixed test View in context: https://bugs.webkit.org/attachment.cgi?id=101276&action=review > Source/WebCore/inspector/InspectorFrontendHost.cpp:95 > + UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture); yay > Source/WebCore/inspector/front-end/TimelinePanel.js:248 > + for (var i = 0; i < 20 && index < data.length; ++i, ++index) Why do you schedule continuation? > Source/WebCore/inspector/front-end/TimelinePanel.js:257 > + _importComplete: function() Why is this needed? If for tests, please name accordingly and put comment inside. > Source/WebCore/inspector/front-end/TimelinePanel.js:263 > + function onLoad(e) { { on the next line > Source/WebCore/inspector/front-end/TimelinePanel.js:283 > + var date = String.sprintf("%4d-%2d-%2d-%2d-%2d", now.getFullYear(), now.getMonth() + 1, now.getDate(), now.getHours(), now.getMinutes()); This does not look like any of the standard notations. Why not to use a "short" or tfc2445? (20110719T1245)? > Source/WebCore/inspector/front-end/TimelinePanel.js:390 > + this._rawRecords.push(record); Can we store records in the TimelineAgentModel instead? > Source/WebCore/inspector/front-end/inspector.css:3875 > +.save-as-status-bar-item .glyph { Should this be rolled back?
Created attachment 101287 [details] [patch] fifth version > > > Source/WebCore/inspector/front-end/TimelinePanel.js:248 > > + for (var i = 0; i < 20 && index < data.length; ++i, ++index) > > Why do you schedule continuation? otherwise Inspector is unresponsive until insertion is completed. With this solution the user see the progress of insertion. > > > Source/WebCore/inspector/front-end/TimelinePanel.js:257 > > + _importComplete: function() > > Why is this needed? If for tests, please name accordingly and put comment inside. removed. > > > Source/WebCore/inspector/front-end/TimelinePanel.js:263 > > + function onLoad(e) { > > { on the next line fixed. > > > Source/WebCore/inspector/front-end/TimelinePanel.js:283 > > + var date = String.sprintf("%4d-%2d-%2d-%2d-%2d", now.getFullYear(), now.getMonth() + 1, now.getDate(), now.getHours(), now.getMinutes()); > > This does not look like any of the standard notations. Why not to use a "short" or tfc2445? (20110719T1245)? fixed. > > > Source/WebCore/inspector/front-end/TimelinePanel.js:390 > > + this._rawRecords.push(record); > > Can we store records in the TimelineAgentModel instead? There is no such thing as TimelineAgentModel and I see no reason to introduce it. > > > Source/WebCore/inspector/front-end/inspector.css:3875 > > +.save-as-status-bar-item .glyph { > > Should this be rolled back? reverted.
Created attachment 101294 [details] [patch] fifth version with extracted model code.
Comment on attachment 101294 [details] [patch] fifth version with extracted model code. View in context: https://bugs.webkit.org/attachment.cgi?id=101294&action=review > LayoutTests/inspector/timeline/timeline-load.html:9 > + if (window.layoutTestController) remove? > LayoutTests/inspector/timeline/timeline-load.html:40 > + if (JSON.stringify(data) == JSON.stringify(saveData)) InspectorTest.addResult(JSON.stringify(saveData)); > Source/WebCore/inspector/front-end/TimelinePanel.js:224 > + _createFileSelector: function() Can this be done lazily? > Source/WebCore/inspector/front-end/TimelinePanel.js:240 > + contextMenu.appendItem(WebInspector.UIString("&Export Timeline data..."), this._exportToFile.bind(this)); Use ellipsis character > Source/WebCore/inspector/front-end/TimelinePanel.js:1253 > + WebInspector.error(WebInspector.UIString('Timeline.importFromFile: File "%s" not found.', file.name)); There is no WebInspector.error. > Source/WebCore/inspector/front-end/TimelinePanel.js:1274 > + var date = String.sprintf("%4d%2d%2dT%2d%2d", now.getFullYear(), now.getMonth() + 1, now.getDate(), now.getHours(), now.getMinutes()); You can move this to utilities.js (Date.prototype.toRFC2445)
Created attachment 101306 [details] [patch] next version (In reply to comment #16) > (From update of attachment 101294 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101294&action=review > > > LayoutTests/inspector/timeline/timeline-load.html:9 > > + if (window.layoutTestController) > > remove? done > > > LayoutTests/inspector/timeline/timeline-load.html:40 > > + if (JSON.stringify(data) == JSON.stringify(saveData)) > > InspectorTest.addResult(JSON.stringify(saveData)); The test doesn't verify the data but tests that import/export things work. > > > Source/WebCore/inspector/front-end/TimelinePanel.js:224 > > + _createFileSelector: function() > > Can this be done lazily? Nope. It doesn't work if I create input element just on the fly. I can't use setTimeout because timer call has no UserGesture flag enabled. > > > Source/WebCore/inspector/front-end/TimelinePanel.js:240 > > + contextMenu.appendItem(WebInspector.UIString("&Export Timeline data..."), this._exportToFile.bind(this)); > > Use ellipsis character done. > > Source/WebCore/inspector/front-end/TimelinePanel.js:1253 > > + WebInspector.error(WebInspector.UIString('Timeline.importFromFile: File "%s" not found.', file.name)); > > There is no WebInspector.error. done > > > Source/WebCore/inspector/front-end/TimelinePanel.js:1274 > > + var date = String.sprintf("%4d%2d%2dT%2d%2d", now.getFullYear(), now.getMonth() + 1, now.getDate(), now.getHours(), now.getMinutes()); > > You can move this to utilities.js (Date.prototype.toRFC2445) done. toRFC3339
Comment on attachment 101306 [details] [patch] next version View in context: https://bugs.webkit.org/attachment.cgi?id=101306&action=review > Source/WebCore/inspector/front-end/TimelinePanel.js:1261 > + WebInspector.log(WebInspector.UIString('Timeline.importFromFile: An error occurred reading the file "%s"', file.name)); reading -> while reading
Committed r91252: <http://trac.webkit.org/changeset/91252>
*** Bug 46276 has been marked as a duplicate of this bug. ***