[webkit-reviews] review requested: [Bug 64601] Web Inspector: implement import/export for timeline data. : [Attachment 101287] [patch] fifth version

[bugzilla-daemon at webkit.org]

Ilya Tikhonovsky <loislo at chromium.org> has asked  for review:
Bug 64601: Web Inspector: implement import/export for timeline data.
https://bugs.webkit.org/show_bug.cgi?id=64601

Attachment 101287: [patch] fifth version
https://bugs.webkit.org/attachment.cgi?id=101287&action=review

------- Additional Comments from Ilya Tikhonovsky <loislo at chromium.org>
> 
> > 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.