Bug 64601 – Web Inspector: implement import/export for timeline data.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Ilya Tikhonovsky]

%subj%

[Ilya Tikhonovsky]

Created attachment 100992 [details]
[patch] initial version

[Yury Semikhatsky]

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]

Created attachment 101008 [details]
[patch] second version

[Yury Semikhatsky]

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]

Created attachment 101135 [details]
[patch] third version

[Ilya Tikhonovsky]

Created attachment 101137 [details]
[patch] third version with binary changes.

[Pavel Feldman]

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]

Created attachment 101140 [details]
screenshot

[Pavel Feldman]

I'd say we should have "Export" and "Import" in context menu + support drag'n'drop both ways instead.

[Ilya Tikhonovsky]

Created attachment 101168 [details]
[patch] fourth version with context menu

[WebKit Review Bot]

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]

Created attachment 101276 [details]
[patch] fourth version with fixed test

[Pavel Feldman]

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]

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]

Created attachment 101294 [details]
[patch] fifth version with extracted model code.

[Pavel Feldman]

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]

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 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]

Committed r91252: <http://trac.webkit.org/changeset/91252>

[Ilya Tikhonovsky]

*** Bug 46276 has been marked as a duplicate of this bug. ***