RESOLVED FIXED 64601
Web Inspector: implement import/export for timeline data.
https://bugs.webkit.org/show_bug.cgi?id=64601
Summary Web Inspector: implement import/export for timeline data.
Ilya Tikhonovsky
Reported 2011-07-15 09:12:38 PDT
%subj%
Attachments
[patch] initial version (18.16 KB, patch)
2011-07-15 09:22 PDT, Ilya Tikhonovsky
yurys: review-
[patch] second version (21.81 KB, patch)
2011-07-15 11:16 PDT, Ilya Tikhonovsky
no flags
[patch] third version (11.33 KB, patch)
2011-07-18 00:38 PDT, Ilya Tikhonovsky
no flags
[patch] third version with binary changes. (21.80 KB, patch)
2011-07-18 01:41 PDT, Ilya Tikhonovsky
pfeldman: review-
screenshot (4.21 KB, image/png)
2011-07-18 02:18 PDT, Ilya Tikhonovsky
no flags
[patch] fourth version with context menu (22.56 KB, patch)
2011-07-18 10:21 PDT, Ilya Tikhonovsky
webkit.review.bot: commit-queue-
[patch] fourth version with fixed test (22.78 KB, patch)
2011-07-18 23:46 PDT, Ilya Tikhonovsky
pfeldman: review-
[patch] fifth version (11.03 KB, patch)
2011-07-19 01:32 PDT, Ilya Tikhonovsky
no flags
[patch] fifth version with extracted model code. (12.47 KB, patch)
2011-07-19 03:01 PDT, Ilya Tikhonovsky
pfeldman: review-
[patch] next version (13.50 KB, patch)
2011-07-19 05:46 PDT, Ilya Tikhonovsky
yurys: review+
Ilya Tikhonovsky
Comment 1 2011-07-15 09:22:21 PDT
Created attachment 100992 [details] [patch] initial version
Yury Semikhatsky
Comment 2 2011-07-15 09:36:44 PDT
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.
Ilya Tikhonovsky
Comment 3 2011-07-15 11:16:54 PDT
Created attachment 101008 [details] [patch] second version
Yury Semikhatsky
Comment 4 2011-07-15 11:24:16 PDT
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.
Ilya Tikhonovsky
Comment 5 2011-07-18 00:38:35 PDT
Created attachment 101135 [details] [patch] third version
Ilya Tikhonovsky
Comment 6 2011-07-18 01:41:52 PDT
Created attachment 101137 [details] [patch] third version with binary changes.
Pavel Feldman
Comment 7 2011-07-18 01:56:40 PDT
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?
Ilya Tikhonovsky
Comment 8 2011-07-18 02:18:49 PDT
Created attachment 101140 [details] screenshot
Pavel Feldman
Comment 9 2011-07-18 02:19:51 PDT
I'd say we should have "Export" and "Import" in context menu + support drag'n'drop both ways instead.
Ilya Tikhonovsky
Comment 10 2011-07-18 10:21:38 PDT
Created attachment 101168 [details] [patch] fourth version with context menu
WebKit Review Bot
Comment 11 2011-07-18 10:44:30 PDT
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
Ilya Tikhonovsky
Comment 12 2011-07-18 23:46:50 PDT
Created attachment 101276 [details] [patch] fourth version with fixed test
Pavel Feldman
Comment 13 2011-07-19 00:00:02 PDT
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?
Ilya Tikhonovsky
Comment 14 2011-07-19 01:32:25 PDT
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.
Ilya Tikhonovsky
Comment 15 2011-07-19 03:01:43 PDT
Created attachment 101294 [details] [patch] fifth version with extracted model code.
Pavel Feldman
Comment 16 2011-07-19 04:15:18 PDT
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)
Ilya Tikhonovsky
Comment 17 2011-07-19 05:46:11 PDT
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
Yury Semikhatsky
Comment 18 2011-07-19 06:38:14 PDT
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
Ilya Tikhonovsky
Comment 19 2011-07-19 06:45:21 PDT
Ilya Tikhonovsky
Comment 20 2011-08-03 08:28:14 PDT
*** Bug 46276 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.