Bug 99179 – Web Inspector: Saving HAR, snapshots and timeline data do not work in remote debugging mode

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

CRBUG: http://code.google.com/p/chromium/issues/detail?id=155494

Created attachment 168426 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 168426 [details]
Patch

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

> Source/WebCore/inspector/front-end/FileManager.js:58
> +        return InspectorFrontendHost.canSave() && ("append" in InspectorFrontendHost) && (!("canAppend" in InspectorFrontendHost) || InspectorFrontendHost.canAppend());

Looks like InspectorFrontendHost.canAppend() is always false.

> Source/WebCore/inspector/front-end/FileUtils.js:350
> +    this._isBuffering = !WebInspector.fileManager.canAppend();

So now each method of the class looks like if (this._isBuffering) { ... } else { ... }. I think that's an indication that we should be using a different implementation of OutputStream instead of abusing FileOutputStrem. I'd suggest adding BlobOutputStream and a factory method that creates either of the streams based on whether we canAppend().

> Source/WebCore/inspector/front-end/FileUtils.js:391
> +        function callbackWrapper()
> +        {
> +            callback(this);
> +        }

why not bind to callback directly?

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:151
> +        function cleanup() {
> +            window.URL.revokeObjectURL(objectUrl);
>          }

style: { => next line
I think this could also be inlined into setTimeout()

Comment on attachment 168426 [details]
Patch

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

>> Source/WebCore/inspector/front-end/FileManager.js:58
>> +        return InspectorFrontendHost.canSave() && ("append" in InspectorFrontendHost) && (!("canAppend" in InspectorFrontendHost) || InspectorFrontendHost.canAppend());
> 
> Looks like InspectorFrontendHost.canAppend() is always false.

Addressed.

>> Source/WebCore/inspector/front-end/FileUtils.js:350
>> +    this._isBuffering = !WebInspector.fileManager.canAppend();
> 
> So now each method of the class looks like if (this._isBuffering) { ... } else { ... }. I think that's an indication that we should be using a different implementation of OutputStream instead of abusing FileOutputStrem. I'd suggest adding BlobOutputStream and a factory method that creates either of the streams based on whether we canAppend().

Done.

>> Source/WebCore/inspector/front-end/FileUtils.js:391
>> +        }
> 
> why not bind to callback directly?

Fixed.

>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:151
>>          }
> 
> style: { => next line
> I think this could also be inlined into setTimeout()

Done.

Created attachment 168636 [details]
Patch

Created attachment 168714 [details]
Patch

[Build Bot]

Comment on attachment 168714 [details]
Patch

Attachment 168714 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14299700

Created attachment 168851 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 168851 [details]
Patch

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

> Source/WebCore/inspector/front-end/HeapSnapshotView.js:956
> +        return !this.fromFile() && !!this._snapshotProxy && !this._receiver;

nit: I don't think !! has any effect here.

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:151
> +        setTimeout(function() {

style: { => next line.
consider bind?

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:163
> +        setTimeout(function() {

ditto.

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:179
> +        setTimeout(function() {

ditto

Comment on attachment 168851 [details]
Patch

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

>> Source/WebCore/inspector/front-end/HeapSnapshotView.js:956
>> +        return !this.fromFile() && !!this._snapshotProxy && !this._receiver;
> 
> nit: I don't think !! has any effect here.

This makes influence on return value type.
Without cast, return value is {boolean|null} instead of {boolean}.

>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:151
>> +        setTimeout(function() {
> 
> style: { => next line.
> consider bind?

OK.

>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:163
>> +        setTimeout(function() {
> 
> ditto.

Done

>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:179
>> +        setTimeout(function() {
> 
> ditto

Binding is impossible. Explicitly declared nested function.

Created attachment 168879 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 168879 [details]
Patch

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

LGTM with a nit.

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:176
> +        function cleanup() {

{ => next line, please.

Created attachment 168934 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 168934 [details]
Patch

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

> Source/WebCore/inspector/InspectorFrontendHost.cpp:253
> +void InspectorFrontendHost::close(const String& /* url */) { }

style: remove comment and put each brace on its own line.

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:151
> +        setTimeout(WebInspector.fileManager.savedURL.bind(WebInspector.fileManager, url), 0);

Why do you need setTimeout here?

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:161
> +        setTimeout(WebInspector.fileManager.appendedToURL.bind(WebInspector.fileManager, url), 0);

Why do you need setTimeout here?

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:180
> +        setTimeout(cleanup, 0);

Is there an event that we could listen to instead of scheduling revokeObjectURL on timeout?

Comment on attachment 168934 [details]
Patch

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

>> Source/WebCore/inspector/InspectorFrontendHost.cpp:253
>> +void InspectorFrontendHost::close(const String& /* url */) { }
> 
> style: remove comment and put each brace on its own line.

Done.

>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:151
>> +        setTimeout(WebInspector.fileManager.savedURL.bind(WebInspector.fileManager, url), 0);
> 
> Why do you need setTimeout here?

It is expected (by caller) that callback is invoked asynchronously.

>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:161
>> +        setTimeout(WebInspector.fileManager.appendedToURL.bind(WebInspector.fileManager, url), 0);
> 
> Why do you need setTimeout here?

Ditto.

>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:180
>> +        setTimeout(cleanup, 0);
> 
> Is there an event that we could listen to instead of scheduling revokeObjectURL on timeout?

There are no specific events for Blobs.
window events do not fire either.

Created attachment 169158 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 169158 [details]
Patch

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

> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:174
> +        window.location = objectUrl + "#" + url;

Can we use hostname instead?

Comment on attachment 169158 [details]
Patch

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

>> Source/WebCore/inspector/front-end/InspectorFrontendHostStub.js:174
>> +        window.location = objectUrl + "#" + url;
> 
> Can we use hostname instead?

addressed

Created attachment 169182 [details]
Patch

[WebKit Review Bot]

Comment on attachment 169182 [details]
Patch

Rejecting attachment 169182 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14412891

[Andrey Kosyakov]

Committed r131872: <http://trac.webkit.org/changeset/131872>