Bug 80325

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 Flags
proposed patch
none
updated to ToT fpizlo: review+, webkit.review.bot: commit-queue-

Description Alexey Proskuryakov 2012-03-05 13:46:38 PST
There is only one subclass.
Comment 1 Alexey Proskuryakov 2012-03-05 14:16:31 PST
Created attachment 130204 [details]
proposed patch
Comment 2 Alexey Proskuryakov 2012-03-05 14:19:18 PST
ResourceHandle still serves as interface to blob: URL loading, and has a subclass, but it no longer knows anything about these directly.
Comment 3 Alexey Proskuryakov 2012-03-05 14:20:29 PST
So this improves modularization of WebKit, too.
Comment 4 Alexey Proskuryakov 2012-03-05 14:58:56 PST
Created attachment 130209 [details]
updated to ToT

Would like EWS to run...
Comment 5 Filip Pizlo 2012-03-06 22:53:40 PST
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?
Comment 6 Alexey Proskuryakov 2012-03-06 23:03:21 PST
> 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.
Comment 7 Filip Pizlo 2012-03-06 23:10:23 PST
(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 8 Filip Pizlo 2012-03-06 23:15:57 PST
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 9 Alexey Proskuryakov 2012-03-07 08:54:28 PST
Comment on attachment 130209 [details]
updated to ToT

Thank you for review!
Comment 10 WebKit Review Bot 2012-03-07 08:57:57 PST
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
Comment 11 Alexey Proskuryakov 2012-03-07 09:48:36 PST
Committed <http://trac.webkit.org/changeset/110066>. Post-landing comments from CC'ed experts are still welcome.
Comment 12 Alexey Proskuryakov 2012-03-07 11:58:53 PST
Build fix in <http://trac.webkit.org/changeset/110080>.