[webkit-reviews] review denied: [Bug 64601] Web Inspector: implement import/export for timeline data. : [Attachment 101276] [patch] fourth version with fixed test

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 64601: Web Inspector: implement import/export for timeline data.
https://bugs.webkit.org/show_bug.cgi?id=64601

Attachment 101276: [patch] fourth version with fixed test
https://bugs.webkit.org/attachment.cgi?id=101276&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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?