Bug 97226 - Web Inspector: [refactoring] simplify interface to FileOutputStream
Summary: Web Inspector: [refactoring] simplify interface to FileOutputStream
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: Andrey Kosyakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-20 09:27 PDT by Andrey Kosyakov
Modified: 2012-09-21 05:49 PDT (History)
10 users (show)

See Also:


Attachments
Patch (29.50 KB, patch)
2012-09-20 11:05 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Patch (29.61 KB, patch)
2012-09-21 02:35 PDT, Andrey Kosyakov
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Kosyakov 2012-09-20 09:27:08 PDT
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.
Comment 1 Andrey Kosyakov 2012-09-20 11:05:29 PDT
Created attachment 164948 [details]
Patch
Comment 2 Ilya Tikhonovsky 2012-09-21 02:02:30 PDT
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.
Comment 3 Yury Semikhatsky 2012-09-21 02:03:02 PDT
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.
Comment 4 Andrey Kosyakov 2012-09-21 02:32:22 PDT
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).
Comment 5 Andrey Kosyakov 2012-09-21 02:35:44 PDT
Created attachment 165094 [details]
Patch
Comment 6 Andrey Kosyakov 2012-09-21 05:49:17 PDT
Committed r129218: <http://trac.webkit.org/changeset/129218>