Bug 92348 - Web Inspector: Profiles: extract save to file / load from code
Summary: Web Inspector: Profiles: extract save to file / load from code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: eustas.bug
URL:
Keywords:
Depends on: 92390
Blocks: 91991
  Show dependency treegraph
 
Reported: 2012-07-26 01:24 PDT by eustas.bug
Modified: 2012-08-03 03:28 PDT (History)
13 users (show)

See Also:


Attachments
Patch (40.34 KB, patch)
2012-07-26 01:36 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (37.46 KB, patch)
2012-07-26 05:35 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (39.11 KB, patch)
2012-07-26 08:45 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (34.21 KB, patch)
2012-08-01 02:56 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (32.53 KB, patch)
2012-08-02 10:36 PDT, eustas.bug
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description eustas.bug 2012-07-26 01:24:03 PDT
Motivation: extract reusable code for timeline load/save.
Comment 1 eustas.bug 2012-07-26 01:36:10 PDT
Created attachment 154575 [details]
Patch
Comment 2 eustas.bug 2012-07-26 05:22:21 PDT
Comment on attachment 154575 [details]
Patch

Preparing new patch, based on review notes in 91991.
Comment 3 eustas.bug 2012-07-26 05:35:23 PDT
Created attachment 154619 [details]
Patch
Comment 4 Yury Semikhatsky 2012-07-26 06:46:28 PDT
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 5 eustas.bug 2012-07-26 08:22:04 PDT
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.
Comment 6 eustas.bug 2012-07-26 08:45:22 PDT
Created attachment 154659 [details]
Patch
Comment 7 eustas.bug 2012-08-01 02:56:49 PDT
Created attachment 155767 [details]
Patch
Comment 8 Ilya Tikhonovsky 2012-08-01 08:10:56 PDT
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 9 eustas.bug 2012-08-01 09:02:34 PDT
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.
Comment 10 Ilya Tikhonovsky 2012-08-02 05:36:22 PDT
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.
Comment 11 Yury Semikhatsky 2012-08-02 06:02:50 PDT
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?
Comment 12 Andrey Kosyakov 2012-08-02 06:10:08 PDT
(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 13 eustas.bug 2012-08-02 09:56:27 PDT
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.
Comment 14 eustas.bug 2012-08-02 10:36:21 PDT
Created attachment 156118 [details]
Patch
Comment 15 WebKit Review Bot 2012-08-03 03:28:39 PDT
Comment on attachment 156118 [details]
Patch

Clearing flags on attachment: 156118

Committed r124591: <http://trac.webkit.org/changeset/124591>
Comment 16 WebKit Review Bot 2012-08-03 03:28:44 PDT
All reviewed patches have been landed.  Closing bug.