Bug 91991 – Web Inspector: Timeline: avoid "IPC message too big" on save/load

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

Motivation: Now timeline tries to save all data with one chunk.
 Sometimes this causes "IPC message to big" error.

Solution: Refactor Profiles/Heap code - extract chunked IO and reuse it.

Created attachment 153790 [details]
Patch

refactoring-preview

[Ilya Tikhonovsky]

Comment on attachment 153790 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153790&action=review

> Source/WebCore/WebCore.gypi:6302
> +            'inspector/front-end/FileUtils.js',

please fix WebKit.qrc and WebCore.vcproj files too

[Ilya Tikhonovsky]

Comment on attachment 153790 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153790&action=review

Looks like the existing implementation has bad design. Could you please fix it (replace the callbacks with delegate interface) in the first patch and extract the code into separate file in the second or vice versa.

> Source/WebCore/inspector/front-end/FileUtils.js:101
> +     * @param {function(WebInspector.HeapSnapshotProxy)=} callback

please remove this

> Source/WebCore/inspector/front-end/FileUtils.js:110
> +        return true;

why do we need a return value here

Comment on attachment 153790 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153790&action=review

>> Source/WebCore/inspector/front-end/FileUtils.js:101
>> +     * @param {function(WebInspector.HeapSnapshotProxy)=} callback
> 
> please remove this

Fixed in https://bugs.webkit.org/show_bug.cgi?id=92348
Please take a look.

[Alexander Pavlov (apavlov)]

Comment on attachment 153790 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153790&action=review

> Source/WebCore/ChangeLog:9
> +         Sometimes this causes "IPC message to big" error.

Wrong indent

> Source/WebCore/ChangeLog:11
> +        Solution: Refactor Profiles/Heap code - extract chunked IO and reuse it.

Now that you are describing your solution, it's a good idea to mention the FileUtils and quickly cover its contents.

> Source/WebCore/ChangeLog:19
> +        Ditto.

Please remove non-meaningful comments, as they obscure the changelog entry contents. You can remove all of these along with added (WebInspector.*.prototype.*) entries in FileUtils.js (so, retain only the new type names). I believe this was mentioned about a month ago in the webkit-dev mailing list thread named "Please include function-level comments in change log entries".

Ultimately, your goal is NOT commenting every entry added by webkit-patch but creating a changelog that will help others understand what your patch does.

> Source/WebCore/ChangeLog:70
> +        (WebInspector.HeapSnapshotLoader.prototype.startTransfer):

You don't need these entries

> Source/WebCore/ChangeLog:87
> +        (WebInspector.HeapProfileHeader.prototype.load):

Ditto

> Source/WebCore/ChangeLog:115
> +        (WebInspector.ProfilesPanel.prototype._createFileSelectorElement):

Ditto

> Source/WebCore/inspector/front-end/FileUtils.js:34
> +WebInspector.ChunkedTransferer = function()

ChunkedTransferrer. But I believe a more meaningful name is needed, if possible.

> Source/WebCore/inspector/front-end/FileUtils.js:42
> +     * @

What is this line here for?

> Source/WebCore/inspector/front-end/FileUtils.js:56
> +     * @param {function(*)=} callback

Can you make the parameter/return types more specific?

> Source/WebCore/inspector/front-end/FileUtils.js:63
> +    dispose: function()

What is this method necessary for? It has no non-trivial implementations.

> Source/WebCore/inspector/front-end/FileUtils.js:80
> +    onChunkTransfered: function()

onChunkTransferred

> Source/WebCore/inspector/front-end/FileUtils.js:148
> +        this._delegate && this._delegate.onTransferStarted();

Please avoid these hacky shortcuts

> Source/WebCore/inspector/front-end/FileUtils.js:160
> +        this._delegate && this._delegate.onChunkTransfered();

ditto

> Source/WebCore/inspector/front-end/FileUtils.js:268
> +    lastIndex = lastIndex ? lastIndex : source.length;

lastIndex = lastIndex || source.length; (this is one of a few acceptable, JS-specific shortcuts)

> Source/WebCore/inspector/front-end/FileUtils.js:271
> +    for (var index = startIndex ? startIndex : 0; index < lastIndex; ++index) {

... = startIndex || 0;...

> Source/WebCore/inspector/front-end/FileUtils.js:272
> +        var character = source.charCodeAt(index);

A good rule is to choose variable names that reflect the data they hold. In this case, |character| holds not a real character but its charCode (a number).

I understand that this is just code motion, but refactoring is a good reason (and the most common one :)) to clean up the code.

> Source/WebCore/inspector/front-end/HeapSnapshotView.js:987
> +WebInspector.HeapSnapshotLoadFileTransferer = function(snapshotHeader, fileReader)

...Transferrer

> LayoutTests/ChangeLog:8
> +        Motivation: Now timeline tries to save all data with one chunk.

LayoutTests ChangeLog entry requires only treatment of changes relevant to the modified tests, as the timeline issue is fixed in Source/WebCore

> LayoutTests/inspector/profiler/heap-snapshot-loader.html:112
> +                    },

Trailing comma

Comment on attachment 153790 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153790&action=review

>> Source/WebCore/ChangeLog:9
>> +         Sometimes this causes "IPC message to big" error.
> 
> Wrong indent

Got it.

>> Source/WebCore/ChangeLog:11
>> +        Solution: Refactor Profiles/Heap code - extract chunked IO and reuse it.
> 
> Now that you are describing your solution, it's a good idea to mention the FileUtils and quickly cover its contents.

OK.

>> Source/WebCore/ChangeLog:19
>> +        Ditto.
> 
> Please remove non-meaningful comments, as they obscure the changelog entry contents. You can remove all of these along with added (WebInspector.*.prototype.*) entries in FileUtils.js (so, retain only the new type names). I believe this was mentioned about a month ago in the webkit-dev mailing list thread named "Please include function-level comments in change log entries".
> 
> Ultimately, your goal is NOT commenting every entry added by webkit-patch but creating a changelog that will help others understand what your patch does.

Done. Thanks.

>> Source/WebCore/ChangeLog:70
>> +        (WebInspector.HeapSnapshotLoader.prototype.startTransfer):
> 
> You don't need these entries

Fixed.

>> Source/WebCore/ChangeLog:87
>> +        (WebInspector.HeapProfileHeader.prototype.load):
> 
> Ditto

Fixed.

>> Source/WebCore/ChangeLog:115
>> +        (WebInspector.ProfilesPanel.prototype._createFileSelectorElement):
> 
> Ditto

Fixed.

>> Source/WebCore/WebCore.gypi:6302
>> +            'inspector/front-end/FileUtils.js',
> 
> please fix WebKit.qrc and WebCore.vcproj files too

Fixed.

>> Source/WebCore/inspector/front-end/FileUtils.js:34
>> +WebInspector.ChunkedTransferer = function()
> 
> ChunkedTransferrer. But I believe a more meaningful name is needed, if possible.

Fixed.

>> Source/WebCore/inspector/front-end/FileUtils.js:42
>> +     * @
> 
> What is this line here for?

Typo. Fixed.

>> Source/WebCore/inspector/front-end/FileUtils.js:56
>> +     * @param {function(*)=} callback
> 
> Can you make the parameter/return types more specific?

Callbacks are removed in new patch.

>> Source/WebCore/inspector/front-end/FileUtils.js:63
>> +    dispose: function()
> 
> What is this method necessary for? It has no non-trivial implementations.

HeapSnapshotLoader has.

>> Source/WebCore/inspector/front-end/FileUtils.js:80
>> +    onChunkTransfered: function()
> 
> onChunkTransferred

fixed

>> Source/WebCore/inspector/front-end/FileUtils.js:110
>> +        return true;
> 
> why do we need a return value here

Fixed

>> Source/WebCore/inspector/front-end/FileUtils.js:148
>> +        this._delegate && this._delegate.onTransferStarted();
> 
> Please avoid these hacky shortcuts

OK

>> Source/WebCore/inspector/front-end/FileUtils.js:160
>> +        this._delegate && this._delegate.onChunkTransfered();
> 
> ditto

Done.

>> Source/WebCore/inspector/front-end/FileUtils.js:268
>> +    lastIndex = lastIndex ? lastIndex : source.length;
> 
> lastIndex = lastIndex || source.length; (this is one of a few acceptable, JS-specific shortcuts)

Fixed.

>> Source/WebCore/inspector/front-end/FileUtils.js:271
>> +    for (var index = startIndex ? startIndex : 0; index < lastIndex; ++index) {
> 
> ... = startIndex || 0;...

Thanks.

>> Source/WebCore/inspector/front-end/FileUtils.js:272
>> +        var character = source.charCodeAt(index);
> 
> A good rule is to choose variable names that reflect the data they hold. In this case, |character| holds not a real character but its charCode (a number).
> 
> I understand that this is just code motion, but refactoring is a good reason (and the most common one :)) to clean up the code.

Fixed, thanks.

>> Source/WebCore/inspector/front-end/HeapSnapshotView.js:987
>> +WebInspector.HeapSnapshotLoadFileTransferer = function(snapshotHeader, fileReader)
> 
> ...Transferrer

Fixed.

>> LayoutTests/ChangeLog:8
>> +        Motivation: Now timeline tries to save all data with one chunk.
> 
> LayoutTests ChangeLog entry requires only treatment of changes relevant to the modified tests, as the timeline issue is fixed in Source/WebCore

OK.

>> LayoutTests/inspector/profiler/heap-snapshot-loader.html:112
>> +                    },
> 
> Trailing comma

fixed.

Created attachment 156334 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 156334 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156334&action=review

> Source/WebCore/inspector/front-end/FileUtils.js:198
> +        var charCode = source.charCodeAt(index);
> +        if (charCode === openingBracket)
> +            ++counter;
> +        else if (charCode === closingBracket) {
> +            if (--counter === 0)
> +                return index + 1;
> +        }

so what if we have brackets within strings?

Comment on attachment 156334 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156334&action=review

>> Source/WebCore/inspector/front-end/FileUtils.js:198
>> +        }
> 
> so what if we have brackets within strings?

fixed

Created attachment 156538 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 156538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156538&action=review

> Source/WebCore/inspector/front-end/FileUtils.js:193
> +            if (character === '"' && source[index - 1] !== '\\')

What is the performance impact of using character directly instead of its char code for comparison as it used to be?

> Source/WebCore/inspector/front-end/TimelineModel.js:167
> +        /* TODO: Prevent model recording/reset/etc during loading. */

TODO -> FIXME

> Source/WebCore/inspector/front-end/TimelineModel.js:302
> +        /* TODO: Show progress indicator */

TODO -> FIXME

> Source/WebCore/inspector/front-end/TimelineModel.js:390
> +        writer.transferChunk(data.join(",\n"));

This way you'll miss ",\n" between records in consecutive chunks.

[Andrey Kosyakov]

Comment on attachment 156538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156538&action=review

>> Source/WebCore/inspector/front-end/FileUtils.js:193
>> +            if (character === '"' && source[index - 1] !== '\\')
> 
> What is the performance impact of using character directly instead of its char code for comparison as it used to be?

So how about "\\"?

Comment on attachment 156538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156538&action=review

>>> Source/WebCore/inspector/front-end/FileUtils.js:193
>>> +            if (character === '"' && source[index - 1] !== '\\')
>> 
>> What is the performance impact of using character directly instead of its char code for comparison as it used to be?
> 
> So how about "\\"?

@Yury: On synthetic tests direct comparison to char works somewhat faster.

@Andrey: Oh, I missed this case =( will be fixed in next patch (est. 20 Aug)

>> Source/WebCore/inspector/front-end/TimelineModel.js:390
>> +        writer.transferChunk(data.join(",\n"));
> 
> This way you'll miss ",\n" between records in consecutive chunks.

Not sure we'll suffer of that, but I'll fix it in next patch.

Created attachment 159638 [details]
Patch

[WebKit Review Bot]

Comment on attachment 159638 [details]
Patch

Attachment 159638 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13542700

New failing tests:
inspector/timeline/timeline-load.html
inspector/timeline/timeline-load-incompatible.html

[WebKit Review Bot]

Created attachment 159647 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Andrey Kosyakov]

Comment on attachment 159638 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159638&action=review

> Source/WebCore/inspector/front-end/FileUtils.js:97
> +    this._isCanceled = false;

I think ultimately we just need to pass progress to the file reader.

> Source/WebCore/inspector/front-end/FileUtils.js:215
> +        if (inString) {
> +            if (character === '\\')
> +                ++index;
> +            else if (character === '"')
> +                inString = false;
> +        } else {
> +            if (character === '"')
> +                inString = true;
> +            else if (character === '{')
> +                ++counter;
> +            else if (character === '}') {
> +                if (--counter === 0)
> +                    return index + 1;
> +            }
> +        }

use switch?

> Source/WebCore/inspector/front-end/TimelineModel.js:178
> +        return new WebInspector.ChunkedFileReader(file, 5000000, delegate);

extract a const for 5000000?

> Source/WebCore/inspector/front-end/TimelineModel.js:245
> +    this._json = '';

s/_json/_buffer/?
Also, double quotes please.

> Source/WebCore/inspector/front-end/TimelineModel.js:270
> +        while (true) {
> +            lastIndex = WebInspector.findBalancedCurlyBrackets(data, index);
> +            if (lastIndex != -1)
> +                index = lastIndex;
> +            else
> +                break;
> +        }

do {
   index = lastIndex;
   lastIndex = WebInspector.findBalancedCurlyBrackets(data, index);
} while (lastIndex !== -1)

> Source/WebCore/inspector/front-end/TimelineModel.js:271
> +        var json = data.slice(0, index) + ']';

Double quotes, please.

> Source/WebCore/inspector/front-end/TimelineModel.js:275
> +            json = '[0' + json;

This [0 thingie looks like a hack.
Also, double quotes please.

> Source/WebCore/inspector/front-end/TimelineModel.js:346
> +        case event.target.error.NOT_FOUND_ERR:
> +            WebInspector.log(WebInspector.UIString('Timeline.loadFromFile: File "%s" not found.', reader.fileName()));
> +        break;
> +        case event.target.error.NOT_READABLE_ERR:
> +            WebInspector.log(WebInspector.UIString('Timeline.loadFromFile: File "%s" is not readable', reader.fileName()));
> +        break;
> +        case event.target.error.ABORT_ERR:
> +            break;
> +        default:
> +            WebInspector.log(WebInspector.UIString('Timeline.loadFromFile: An error occurred while reading the file "%s"', reader.fileName()));

Please note Pavel has just changed the text for these error messages and changed the way we report errors. See bug 91528 for details.
Also, please indent breaks.
I would also alias event.target.error to var error for brevity.

> Source/WebCore/inspector/front-end/TimelineModel.js:361
> +    this._prologue = '[' + JSON.stringify(new String(version));

why do you need new String()?
Also, double quotes on strings please.

> Source/WebCore/inspector/front-end/TimelineModel.js:382
> +    onTransferFinished: function(writer) {
> +    },
> +
> +    onError: function(writer, event) {
> +    },

style: empty functions should be like function foo() { }

> Source/WebCore/inspector/front-end/TimelineModel.js:384
> +    _pushChunk: function (writer)

function (writer) => function(writer)

> Source/WebCore/inspector/front-end/TimelineModel.js:395
> +            data.push('');

so we'll have an empty line at the beginning of each chunk and an empty line at the end of each chunk. is that the intent?

> Source/WebCore/inspector/front-end/TimelineModel.js:400
> +            var itemLength = item.length + 2;

+ 2?

> Source/WebCore/inspector/front-end/TimelineModel.js:407
> +        if (this._recordIndex == this._records.length) {

===

> LayoutTests/inspector/timeline/timeline-load.html:74
> +            _loadedSize: 0,
> +            _fileSize: stringifiedData.length,
> +
> +            start: function(output)
> +            {
> +                output.startTransfer();
> +                delegate.onTransferStarted(this);
> +
> +                var chunk = stringifiedData;
> +                this._loadedSize += chunk.length;
> +                output.transferChunk(chunk);
> +                delegate.onChunkTransferred(this);
> +
> +                output.finishTransfer();
> +                delegate.onTransferFinished(this);
> +
> +                model.saveToFile();
> +            },
> +
> +            cancel: function()
> +            {
> +            },
> +
> +            loadedSize: function()
> +            {
> +                return this._loadedSize;
> +            },
> +
> +            fileSize: function()
> +            {
> +                return this._fileSize;
> +            },
> +
> +            fileName: function()
> +            {
> +                return 'fakeFile';
> +            }

Please avoid duplicating such large pieces of code.

> LayoutTests/inspector/timeline/timeline-load.html:100
> +            startTransfer: function()
> +            {
> +                delegate.onTransferStarted(this);
> +            },
> +
> +            transferChunk: function(chunk)
> +            {
> +                saveData += chunk;
> +                delegate.onChunkTransferred(this);
> +            },
> +
> +            finishTransfer: function()
> +            {
> +                delegate.onTransferFinished(this);
> +                checkSaveData();
> +            },
> +
> +            dispose: function()
> +            {
> +            },

-ditto- Please avoid duplicating such large pieces of code.

Comment on attachment 159638 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159638&action=review

>> Source/WebCore/inspector/front-end/FileUtils.js:97
>> +    this._isCanceled = false;
> 
> I think ultimately we just need to pass progress to the file reader.

I think it is better to keep IO and UI distinct.
IMHO we could introduce "Cancelable" interface, make "Progress" implement it and pass "Progress" downcasted to "Cancelable".

>> Source/WebCore/inspector/front-end/FileUtils.js:215
>> +        }
> 
> use switch?

Currently switch is 3 times slower.

>> Source/WebCore/inspector/front-end/TimelineModel.js:178
>> +        return new WebInspector.ChunkedFileReader(file, 5000000, delegate);
> 
> extract a const for 5000000?

Done

>> Source/WebCore/inspector/front-end/TimelineModel.js:245
>> +    this._json = '';
> 
> s/_json/_buffer/?
> Also, double quotes please.

Done

>> Source/WebCore/inspector/front-end/TimelineModel.js:270
>> +        }
> 
> do {
>    index = lastIndex;
>    lastIndex = WebInspector.findBalancedCurlyBrackets(data, index);
> } while (lastIndex !== -1)

OK

>> Source/WebCore/inspector/front-end/TimelineModel.js:271
>> +        var json = data.slice(0, index) + ']';
> 
> Double quotes, please.

Fixed

>> Source/WebCore/inspector/front-end/TimelineModel.js:275
>> +            json = '[0' + json;
> 
> This [0 thingie looks like a hack.
> Also, double quotes please.

Done

>> Source/WebCore/inspector/front-end/TimelineModel.js:346
>> +            WebInspector.log(WebInspector.UIString('Timeline.loadFromFile: An error occurred while reading the file "%s"', reader.fileName()));
> 
> Please note Pavel has just changed the text for these error messages and changed the way we report errors. See bug 91528 for details.
> Also, please indent breaks.
> I would also alias event.target.error to var error for brevity.

Fixed.

>> Source/WebCore/inspector/front-end/TimelineModel.js:361
>> +    this._prologue = '[' + JSON.stringify(new String(version));
> 
> why do you need new String()?
> Also, double quotes on strings please.

I don't know the real reason. It was so as long as history can be tracked (about 13 months).

>> Source/WebCore/inspector/front-end/TimelineModel.js:382
>> +    },
> 
> style: empty functions should be like function foo() { }

Fixed

>> Source/WebCore/inspector/front-end/TimelineModel.js:384
>> +    _pushChunk: function (writer)
> 
> function (writer) => function(writer)

fixed

>> Source/WebCore/inspector/front-end/TimelineModel.js:395
>> +            data.push('');
> 
> so we'll have an empty line at the beginning of each chunk and an empty line at the end of each chunk. is that the intent?

Ooops. Broke this under the influence of Yury's comment.

>> Source/WebCore/inspector/front-end/TimelineModel.js:400
>> +            var itemLength = item.length + 2;
> 
> + 2?

separator length. fixed.

>> LayoutTests/inspector/timeline/timeline-load.html:74
>> +            }
> 
> Please avoid duplicating such large pieces of code.

Done.

Created attachment 160090 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 160090 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160090&action=review

> Source/WebCore/inspector/front-end/FileUtils.js:225
> +        if (data.length == 0)

if (!data.length)

> Source/WebCore/inspector/front-end/FileUtils.js:303
> +WebInspector.ChunkedFileWriter = function(fileName, delegate)

FileOutputStream?

> Source/WebCore/inspector/front-end/TimelineModel.js:289
> +        if (index === 0)

if (!index)

> Source/WebCore/inspector/front-end/TimelineModel.js:436
> +        if (this._recordIndex == this._records.length) {
> +            data.push(data.pop() + "]");
> +        }

drop {}

> LayoutTests/inspector/timeline/timeline-load.html:18
> +            return {
> +                _loadedSize: 0,
> +                _fileSize: input.length,
> +

Can this be written as our typical class definition, with constructor and prototype?

[Andrey Kosyakov]

(In reply to comment #18)
> (From update of attachment 159638 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159638&action=review
> 
> >> Source/WebCore/inspector/front-end/FileUtils.js:97
> >> +    this._isCanceled = false;
> > 
> > I think ultimately we just need to pass progress to the file reader.
> 
> I think it is better to keep IO and UI distinct.
> IMHO we could introduce "Cancelable" interface, make "Progress" implement it and pass "Progress" downcasted to "Cancelable".

Progress is not a UI-level concept, it's just an interface (ProgressIndicator is the UI component). The idea is that besides being able to canel the operation, we often also want to track its progress.

[WebKit Review Bot]

Comment on attachment 160090 [details]
Patch

Attachment 160090 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13558624

New failing tests:
inspector/timeline/timeline-load.html

[WebKit Review Bot]

Created attachment 160096 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 160090 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160090&action=review

>> Source/WebCore/inspector/front-end/FileUtils.js:225
>> +        if (data.length == 0)
> 
> if (!data.length)

fixed

>> Source/WebCore/inspector/front-end/FileUtils.js:303
>> +WebInspector.ChunkedFileWriter = function(fileName, delegate)
> 
> FileOutputStream?

Sounds good.

>> Source/WebCore/inspector/front-end/TimelineModel.js:289
>> +        if (index === 0)
> 
> if (!index)

fixed

>> Source/WebCore/inspector/front-end/TimelineModel.js:436
>> +        }
> 
> drop {}

fixed

>> LayoutTests/inspector/timeline/timeline-load.html:18
>> +
> 
> Can this be written as our typical class definition, with constructor and prototype?

Done

Created attachment 160129 [details]
Patch

[WebKit Review Bot]

Comment on attachment 160129 [details]
Patch

Attachment 160129 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13560665

New failing tests:
inspector/timeline/timeline-load.html

[WebKit Review Bot]

Created attachment 160134 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 160358 [details]
Patch

examine-objects-on-bot

[WebKit Review Bot]

Comment on attachment 160358 [details]
Patch

Attachment 160358 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13600061

New failing tests:
inspector/timeline/timeline-load.html

[WebKit Review Bot]

Created attachment 160367 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 160371 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 160371 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160371&action=review

> Source/WebCore/inspector/front-end/FileUtils.js:286
> +            else if (character === '{')

"{"

> Source/WebCore/inspector/front-end/FileUtils.js:288
> +            else if (character === '}') {

ditto

> Source/WebCore/inspector/front-end/TimelineModel.js:361
> +    onError: function(reader, event) {

{ => next line

> Source/WebCore/inspector/front-end/TimelineModel.js:423
> +        } else {
> +            data.push("");
> +        }

no need for {}

> LayoutTests/inspector/timeline/timeline-load.html:18
> +            function callback() {
> +                model.saveToFile();
> +            }

{ needs to be on its own line. I think you can inline or bind this, though.

> LayoutTests/inspector/timeline/timeline-load.html:35
> +            if (JSON.stringify(expectedOutput) == JSON.stringify(saveData))
> +                InspectorTest.addResult("passed");
> +            else
> +                InspectorTest.addResult("saved data is not equal to restored");

Use InspectorTest.assertEquals()

> LayoutTests/inspector/timeline/timeline-load.html:43
> +            var mockProgress = new WebInspector.Progress();

This could be out of try block.

> LayoutTests/inspector/timeline/timeline-load.html:48
> +        } catch (e) {
> +            InspectorTest.addResult("failed");
> +            InspectorTest.completeTest();
> +        }

Doesn't test harness take care of this?

> LayoutTests/inspector/timeline/timeline-test.js:175
> +        var half = Math.ceil(length / 2);

(length + 1) >> 1

Comment on attachment 160371 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160371&action=review

>> Source/WebCore/inspector/front-end/FileUtils.js:286
>> +            else if (character === '{')
> 
> "{"

Done

>> Source/WebCore/inspector/front-end/FileUtils.js:288
>> +            else if (character === '}') {
> 
> ditto

Done

>> Source/WebCore/inspector/front-end/TimelineModel.js:361
>> +    onError: function(reader, event) {
> 
> { => next line

fixed

>> Source/WebCore/inspector/front-end/TimelineModel.js:423
>> +        }
> 
> no need for {}

fixed

>> LayoutTests/inspector/timeline/timeline-load.html:18
>> +            }
> 
> { needs to be on its own line. I think you can inline or bind this, though.

Done

>> LayoutTests/inspector/timeline/timeline-load.html:35
>> +                InspectorTest.addResult("saved data is not equal to restored");
> 
> Use InspectorTest.assertEquals()

Done

>> LayoutTests/inspector/timeline/timeline-load.html:48
>> +        }
> 
> Doesn't test harness take care of this?

Removed

>> LayoutTests/inspector/timeline/timeline-test.js:175
>> +        var half = Math.ceil(length / 2);
> 
> (length + 1) >> 1

Done

Created attachment 160695 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 160695 [details]
Patch

LGTM.

[Alexander Pavlov (apavlov)]

Comment on attachment 160695 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160695&action=review

> Source/WebCore/ChangeLog:9
> +        Sometimes this causes "IPC message to big" error.

typo: to -> too

> Source/WebCore/inspector/front-end/FileUtils.js:161
> + * @param {!string} url

"Functions and all value types (boolean, number, and string) are non-nullable by default whether or not they are declared with the Non-nullable operator." (https://developers.google.com/closure/compiler/docs/js-for-compiler#nonnull)

> Source/WebCore/inspector/front-end/FileUtils.js:230
> +        if (this._isCanceled)

Does this check imply that this._output.transferChunk() can cancel the transfer? This doesn't look consistent: the data is transferred in any case but the event is not dispatched in all cases.

> Source/WebCore/inspector/front-end/TimelineModel.js:46
> +WebInspector.TimelineModel.TransferChunkLength = 5000000;

Having the "Bytes" suffix would be awesome.

> Source/WebCore/inspector/front-end/TimelineModel.js:284
> +        } while (lastIndex != -1)

!==

> Source/WebCore/inspector/front-end/TimelineModel.js:292
> +        // Prepending "0" to turn string to valid JSON.

typo: to -> into

> Source/WebCore/inspector/front-end/TimelineModel.js:313
> +        for (var i = 1, l = items.length; i < l; ++i)

We try to abstain from using "l" as the variable name at all times, which is a general coding practice. You can use "size" instead.

> Source/WebCore/inspector/front-end/TimelineModel.js:366
> +        switch(error.code) {

Whitespace after "switch"

> Source/WebCore/inspector/front-end/TimelineModel.js:434
> +        if (this._recordIndex == this._records.length)

===

> LayoutTests/ChangeLog:10
> +        * inspector/timeline/timeline-load.html: Turned to test suite to avoid code

typo: to -> into

> LayoutTests/inspector/timeline/timeline-load.html:95
> +<a href="https://bugs.webkit.org/show_bug.cgi?id=91714">Bug 91714</a>

hint: This is usually appended to the test description ("Also tests...") in parentheses on the same line to avoid adding new lines to the golden expectation.

[Alexander Pavlov (apavlov)]

Comment on attachment 160695 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160695&action=review

> Source/WebCore/inspector/front-end/TimelineModel.js:367
> +        case error.NOT_FOUND_ERR:

After a talk with Andrey, we decided to use the FileError object directly for this switch (so, "FileError.NOT_FOUND_ERR" and so on) to make the origin of these errors a bit more clear.

Comment on attachment 160695 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160695&action=review

>> Source/WebCore/ChangeLog:9
>> +        Sometimes this causes "IPC message to big" error.
> 
> typo: to -> too

fixed

>> Source/WebCore/inspector/front-end/FileUtils.js:161
>> + * @param {!string} url
> 
> "Functions and all value types (boolean, number, and string) are non-nullable by default whether or not they are declared with the Non-nullable operator." (https://developers.google.com/closure/compiler/docs/js-for-compiler#nonnull)

OK

>> Source/WebCore/inspector/front-end/FileUtils.js:230
>> +        if (this._isCanceled)
> 
> Does this check imply that this._output.transferChunk() can cancel the transfer? This doesn't look consistent: the data is transferred in any case but the event is not dispatched in all cases.

That is not stated, but the contract is "do not bother delegate after cancel() is invoked".
I.e. the party that calls "cancel()" is responsible for proper cancellation of things that are not controlled by reader.
And, yes, transfer can be canceled by this._output.transferChunk()

>> Source/WebCore/inspector/front-end/TimelineModel.js:46
>> +WebInspector.TimelineModel.TransferChunkLength = 5000000;
> 
> Having the "Bytes" suffix would be awesome.

Done. Thanks.

>> Source/WebCore/inspector/front-end/TimelineModel.js:284
>> +        } while (lastIndex != -1)
> 
> !==

Fixed.

>> Source/WebCore/inspector/front-end/TimelineModel.js:292
>> +        // Prepending "0" to turn string to valid JSON.
> 
> typo: to -> into

Fixed.

>> Source/WebCore/inspector/front-end/TimelineModel.js:313
>> +        for (var i = 1, l = items.length; i < l; ++i)
> 
> We try to abstain from using "l" as the variable name at all times, which is a general coding practice. You can use "size" instead.

Done. Thanks.

>> Source/WebCore/inspector/front-end/TimelineModel.js:366
>> +        switch(error.code) {
> 
> Whitespace after "switch"

Fixed

>> Source/WebCore/inspector/front-end/TimelineModel.js:434
>> +        if (this._recordIndex == this._records.length)
> 
> ===

Done

>> LayoutTests/ChangeLog:10
>> +        * inspector/timeline/timeline-load.html: Turned to test suite to avoid code
> 
> typo: to -> into

Fixed

>> LayoutTests/inspector/timeline/timeline-load.html:95
>> +<a href="https://bugs.webkit.org/show_bug.cgi?id=91714">Bug 91714</a>
> 
> hint: This is usually appended to the test description ("Also tests...") in parentheses on the same line to avoid adding new lines to the golden expectation.

Fixed

Created attachment 161182 [details]
Patch

Created attachment 161192 [details]
Patch

Created attachment 161219 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 161219 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 161219 [details]
Patch

Clearing flags on attachment: 161219

Committed r127039: <http://trac.webkit.org/changeset/127039>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.