Bug 92348 – Web Inspector: Profiles: extract save to file / load from code

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

Motivation: extract reusable code for timeline load/save.

Created attachment 154575 [details]
Patch

Comment on attachment 154575 [details]
Patch

Preparing new patch, based on review notes in 91991.

Created attachment 154619 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 154619 [details]
Patch

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

> Source/WebCore/ChangeLog:19
> +        transfer process: onTransferStarted, onChunktransfered, onTransferFinished.

typo: onChunktransfered -> onChunkTransferred

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

Maybe ChunkedTransferDelegate ?

> Source/WebCore/inspector/front-end/FileUtils.js:67
> +    this.onerror = null;

Can you make it private and pass onerror handler into the constructor, or better add onError method to ChunkedTransferrerDelegate?

> Source/WebCore/inspector/front-end/FileUtils.js:74
> +    start: function(output)

Why do we have separate ChunkedTransferrer and not merge it into ChunkedTransferrerDelegate ?

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

We don't use get prefix for getters in WebKit.

> Source/WebCore/inspector/front-end/HeapSnapshotLoader.js:-45
> -    startLoading: function(callback)

Why did it change? It should only be invoked by HeapSnapshotLoaderProxy and I don't see why it is affected by this patch.

> Source/WebCore/inspector/front-end/HeapSnapshotLoader.js:69
> +        for (var i = startIndex; i < lastIndex; ++i) {

Why did it change?

> Source/WebCore/inspector/front-end/HeapSnapshotProxy.js:404
> +            snapshotProxy.updateStaticData(notifyPendingConsumers.bind(this));

Could you make heap profiler loading changes in a separate patch?

Comment on attachment 154619 [details]
Patch

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

>> Source/WebCore/ChangeLog:19
>> +        transfer process: onTransferStarted, onChunktransfered, onTransferFinished.
> 
> typo: onChunktransfered -> onChunkTransferred

fixed.

>> Source/WebCore/inspector/front-end/FileUtils.js:34
>> +WebInspector.ChunkedTransferrerDelegate = function()
> 
> Maybe ChunkedTransferDelegate ?

Done.

>> Source/WebCore/inspector/front-end/FileUtils.js:67
>> +    this.onerror = null;
> 
> Can you make it private and pass onerror handler into the constructor, or better add onError method to ChunkedTransferrerDelegate?

Done.

>> Source/WebCore/inspector/front-end/FileUtils.js:74
>> +    start: function(output)
> 
> Why do we have separate ChunkedTransferrer and not merge it into ChunkedTransferrerDelegate ?

Separate transferrer are more useful than merged class: transferrer may be something out of our control while delegate is always managed.
In our case ChunkedFileWriter is a transferrer, to receive notifications from it we pass it a delegate.
ChunkedFileReader is not a transferrer, but "output" is not an object we want to add behavior to, because it is used for different transfer types.
So we add behavior through delegate notifications.

>> Source/WebCore/inspector/front-end/FileUtils.js:87
>> +    getLoadedSize: function()
> 
> We don't use get prefix for getters in WebKit.

fixed

>> Source/WebCore/inspector/front-end/HeapSnapshotLoader.js:-45
>> -    startLoading: function(callback)
> 
> Why did it change? It should only be invoked by HeapSnapshotLoaderProxy and I don't see why it is affected by this patch.

Because it was declared to implement HeapSnapshotReceiver, which is ChunkedTransferrer now.

>> Source/WebCore/inspector/front-end/HeapSnapshotLoader.js:69
>> +        for (var i = startIndex; i < lastIndex; ++i) {
> 
> Why did it change?

This method will be reused in next patch. It is not moved do FileUtils, to make review easier.

>> Source/WebCore/inspector/front-end/HeapSnapshotProxy.js:404
>> +            snapshotProxy.updateStaticData(notifyPendingConsumers.bind(this));
> 
> Could you make heap profiler loading changes in a separate patch?

OK, I'll add it as a dependent issue.

Created attachment 154659 [details]
Patch

Created attachment 155767 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 155767 [details]
Patch

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

> Source/WebCore/inspector/front-end/HeapSnapshotLoader.js:74
> -    _findBalancedCurlyBrackets: function()
> +    /**
> +     * @param {!string} source
> +     * @param {number=} startIndex
> +     * @param {number=} lastIndex
> +     * @return {!number}
> +     */
> +    _findBalancedCurlyBrackets: function(source, startIndex, lastIndex)
>      {
> +        startIndex = startIndex || 0;
> +        lastIndex = lastIndex || source.length;
>          var counter = 0;
>          var openingBracket = "{".charCodeAt(0), closingBracket = "}".charCodeAt(0);
> -        for (var i = 0, l = this._json.length; i < l; ++i) {
> -            var character = this._json.charCodeAt(i);
> -            if (character === openingBracket)
> +        for (var i = startIndex; i < lastIndex; ++i) {
> +            var charCode = source.charCodeAt(i);
> +            if (charCode === openingBracket)
>                  ++counter;
> -            else if (character === closingBracket) {
> +            else if (charCode === closingBracket) {
>                  if (--counter === 0)

it is not clear why did you change this.

Comment on attachment 155767 [details]
Patch

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

>> Source/WebCore/inspector/front-end/HeapSnapshotLoader.js:74
>>                  if (--counter === 0)
> 
> it is not clear why did you change this.

1) I'm going to reuse this code for Timeline loading
2) character to charcode was proposed by Alexander.

[Ilya Tikhonovsky]

Comment on attachment 155767 [details]
Patch

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

>>> Source/WebCore/inspector/front-end/HeapSnapshotLoader.js:74
>>>                  if (--counter === 0)
>> 
>> it is not clear why did you change this.
> 
> 1) I'm going to reuse this code for Timeline loading
> 2) character to charcode was proposed by Alexander.

Ok. I'd prefer to see this change as a part of your next patch. Otherwise looks good.

[Yury Semikhatsky]

Comment on attachment 155767 [details]
Patch

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

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

Can we require the delegate passed to WebInspector.ChunkedFileReader constructor to always be non-null? This way we would avoid many null-checks and could inline _onError.

> Source/WebCore/inspector/front-end/HeapSnapshotView.js:790
> +WebInspector.ChunkedTransferrer = function()

Should we rename it to ChunkedTransfer for consistency?

[Andrey Kosyakov]

(In reply to comment #11)
 
> > Source/WebCore/inspector/front-end/HeapSnapshotView.js:790
> > +WebInspector.ChunkedTransferrer = function()
> 
> Should we rename it to ChunkedTransfer for consistency?

How about OutputStream?

Comment on attachment 155767 [details]
Patch

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

>> Source/WebCore/inspector/front-end/FileUtils.js:83
>> +        if (this._delegate)
> 
> Can we require the delegate passed to WebInspector.ChunkedFileReader constructor to always be non-null? This way we would avoid many null-checks and could inline _onError.

Done.

>>>> Source/WebCore/inspector/front-end/HeapSnapshotLoader.js:74
>>>>                  if (--counter === 0)
>>> 
>>> it is not clear why did you change this.
>> 
>> 1) I'm going to reuse this code for Timeline loading
>> 2) character to charcode was proposed by Alexander.
> 
> Ok. I'd prefer to see this change as a part of your next patch. Otherwise looks good.

Done.

Created attachment 156118 [details]
Patch

[WebKit Review Bot]

Comment on attachment 156118 [details]
Patch

Clearing flags on attachment: 156118

Committed r124591: <http://trac.webkit.org/changeset/124591>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.