| Summary: | Merge AsyncFileStream with FileStreamProxy | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||
| Component: | WebCore Misc. | Assignee: | Alexey Proskuryakov <ap> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | dimich, ericu, fpizlo, japhet, jianli, kinuko, levin+threading, rakuco, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 79327 | ||||||||
| Attachments: |
|
||||||||
|
Description
Alexey Proskuryakov
2012-03-05 13:46:38 PST
Created attachment 130204 [details]
proposed patch
ResourceHandle still serves as interface to blob: URL loading, and has a subclass, but it no longer knows anything about these directly. So this improves modularization of WebKit, too. Created attachment 130209 [details]
updated to ToT
Would like EWS to run...
Comment on attachment 130209 [details] updated to ToT View in context: https://bugs.webkit.org/attachment.cgi?id=130209&action=review I am not an expert on this code, but this feels like a refactoring step that leaves the code in an irregular state. Is this the only way to do it? > Source/WebCore/ChangeLog:26 > + * fileapi/AsyncFileStream.cpp: Copied from Source/WebCore/fileapi/FileStreamProxy.cpp. It feels strange to me that this creates files called "AsyncFileStream.[h|cpp]" that have a class called "FileStreamProxy". > Source/WebCore/fileapi/AsyncFileStream.h:37 > +#include "AsyncFileStream.h" Is this file including itself? > I am not an expert on this code, but this feels like a refactoring step that leaves the code in an irregular state. Is this the only way to do it? It does not. This is how svn-create-patch works for moved files - it first shows the whole moved file, and then modifications to it. > Is this file including itself? Yes, that include is wrong and should be removed. (In reply to comment #6) > > I am not an expert on this code, but this feels like a refactoring step that leaves the code in an irregular state. Is this the only way to do it? > > It does not. This is how svn-create-patch works for moved files - it first shows the whole moved file, and then modifications to it. Aha!! I was massively confused, apparently. > > > Is this file including itself? > > Yes, that include is wrong and should be removed. Looks like you did remove the #include. I just missed it because of my confusion regarding how svn-create-patch worked. Comment on attachment 130209 [details]
updated to ToT
R=me. After learning how to grok svn-create-patch's handling of moved-and-modified files, I find this patch to be perfectly sensible. :-)
Comment on attachment 130209 [details]
updated to ToT
Thank you for review!
Comment on attachment 130209 [details] updated to ToT Rejecting attachment 130209 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ileStream.h' patching file Source/WebCore/platform/network/BlobRegistry.h patching file Source/WebCore/platform/network/BlobRegistryImpl.cpp patching file Source/WebCore/platform/network/BlobRegistryImpl.h patching file Source/WebCore/platform/network/ResourceHandle.cpp patching file Source/WebCore/platform/network/ResourceHandle.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Filip Pizlo']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/11852116 Committed <http://trac.webkit.org/changeset/110066>. Post-landing comments from CC'ed experts are still welcome. Build fix in <http://trac.webkit.org/changeset/110080>. |