Bug 64601

Summary: Web Inspector: implement import/export for timeline data.
Product: WebKit Reporter: Ilya Tikhonovsky <loislo>
Component: Web Inspector (Deprecated)Assignee: Ilya Tikhonovsky <loislo>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, dglazkov, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[patch] initial version
yurys: review-
[patch] second version
none
[patch] third version
none
[patch] third version with binary changes.
pfeldman: review-
screenshot
none
[patch] fourth version with context menu
webkit.review.bot: commit-queue-
[patch] fourth version with fixed test
pfeldman: review-
[patch] fifth version
none
[patch] fifth version with extracted model code.
pfeldman: review-
[patch] next version yurys: review+

Description Ilya Tikhonovsky 2011-07-15 09:12:38 PDT
%subj%
Comment 1 Ilya Tikhonovsky 2011-07-15 09:22:21 PDT
Created attachment 100992 [details]
[patch] initial version
Comment 2 Yury Semikhatsky 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.
Comment 3 Ilya Tikhonovsky 2011-07-15 11:16:54 PDT
Created attachment 101008 [details]
[patch] second version
Comment 4 Yury Semikhatsky 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.
Comment 5 Ilya Tikhonovsky 2011-07-18 00:38:35 PDT
Created attachment 101135 [details]
[patch] third version
Comment 6 Ilya Tikhonovsky 2011-07-18 01:41:52 PDT
Created attachment 101137 [details]
[patch] third version with binary changes.
Comment 7 Pavel Feldman 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?
Comment 8 Ilya Tikhonovsky 2011-07-18 02:18:49 PDT
Created attachment 101140 [details]
screenshot
Comment 9 Pavel Feldman 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.
Comment 10 Ilya Tikhonovsky 2011-07-18 10:21:38 PDT
Created attachment 101168 [details]
[patch] fourth version with context menu
Comment 11 WebKit Review Bot 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
Comment 12 Ilya Tikhonovsky 2011-07-18 23:46:50 PDT
Created attachment 101276 [details]
[patch] fourth version with fixed test
Comment 13 Pavel Feldman 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?
Comment 14 Ilya Tikhonovsky 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.
Comment 15 Ilya Tikhonovsky 2011-07-19 03:01:43 PDT
Created attachment 101294 [details]
[patch] fifth version with extracted model code.
Comment 16 Pavel Feldman 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)
Comment 17 Ilya Tikhonovsky 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
Comment 18 Yury Semikhatsky 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
Comment 19 Ilya Tikhonovsky 2011-07-19 06:45:21 PDT
Committed r91252: <http://trac.webkit.org/changeset/91252>
Comment 20 Ilya Tikhonovsky 2011-08-03 08:28:14 PDT
*** Bug 46276 has been marked as a duplicate of this bug. ***