Bug 97226 – Web Inspector: [refactoring] simplify interface to FileOutputStream

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

[Andrey Kosyakov]

This brings FileOutputStream closer to the classical notion of stream, having just open(), write() and close() methods and using individual callbacks instead of a delegate with multiple methods.

[Andrey Kosyakov]

Created attachment 164948 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 164948 [details]
Patch

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

> Source/WebCore/inspector/front-end/HeapSnapshotView.js:861
> -        if (!this._receiver)
> -            this._setupWorker();
>          var loaderProxy = /** @type {WebInspector.HeapSnapshotLoaderProxy} */ this._receiver;
> -
> -        this._numberOfChunks = 0;
> -        if (!loaderProxy.isStarted()) {
> -            loaderProxy.startTransfer();
> +        
> +        if (!loaderProxy) {

looks strange. I hope it could be simplified.

[Yury Semikhatsky]

Comment on attachment 164948 [details]
Patch

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

> Source/WebCore/inspector/front-end/FileUtils.js:316
> +        WebInspector.fileManager.addEventListener(WebInspector.FileManager.EventTypes.SavedURL, callbackWrapper, this);

callbackWrapper -> callbackWrapper.bind(this)

> Source/WebCore/inspector/front-end/HeapSnapshotLoader.js:-141
> -            this.transferChunk("");

I'd say that after this change it is easier to break the code by changing one of the cases.

> Source/WebCore/inspector/front-end/HeapSnapshotView.js:864
> +            this._numberOfChunks = 0;

Now it is only reset if there is now worker yet, is it intentional?

> Source/WebCore/inspector/front-end/HeapSnapshotView.js:936
> +        if (!transferFinished && this._receiver)

You should make sure the receiver is closed when it is reset to null.

[Andrey Kosyakov]

Comment on attachment 164948 [details]
Patch

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

>> Source/WebCore/inspector/front-end/FileUtils.js:316
>> +        WebInspector.fileManager.addEventListener(WebInspector.FileManager.EventTypes.SavedURL, callbackWrapper, this);
> 
> callbackWrapper -> callbackWrapper.bind(this)

this is implicitly bound with third argument to addEventListener. This is critically more convenient, as otherwise we would have to store the resulting bound function to be able to removeEventListener(). I did miss this in call to removeEventListener(), though.

>> Source/WebCore/inspector/front-end/HeapSnapshotLoader.js:-141
>> -            this.transferChunk("");
> 
> I'd say that after this change it is easier to break the code by changing one of the cases.

So what we're doing here is essentially a piece of code that would be executed sequentially if the data in chunk were not incomplete. We only need state to jump in to appropriate parsing stage when more data come in the next block. I think recursion here is quite confusing, we should just keep falling through cases while we have enough data.

>> Source/WebCore/inspector/front-end/HeapSnapshotView.js:861
>> +        if (!loaderProxy) {
> 
> looks strange. I hope it could be simplified.

done.

>> Source/WebCore/inspector/front-end/HeapSnapshotView.js:864
>> +            this._numberOfChunks = 0;
> 
> Now it is only reset if there is now worker yet, is it intentional?

fixed.

>> Source/WebCore/inspector/front-end/HeapSnapshotView.js:936
>> +        if (!transferFinished && this._receiver)
> 
> You should make sure the receiver is closed when it is reset to null.

done (in snapshotReceived).

[Andrey Kosyakov]

Created attachment 165094 [details]
Patch

[Andrey Kosyakov]

Committed r129218: <http://trac.webkit.org/changeset/129218>