CRBUG: http://code.google.com/p/chromium/issues/detail?id=155494
Created attachment 168426 [details] Patch
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
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
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
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
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
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
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
Committed r131872: <http://trac.webkit.org/changeset/131872>