Bug 80325 – Merge AsyncFileStream with FileStreamProxy

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alexey Proskuryakov]

There is only one subclass.

[Alexey Proskuryakov]

Created attachment 130204 [details]
proposed patch

[Alexey Proskuryakov]

ResourceHandle still serves as interface to blob: URL loading, and has a subclass, but it no longer knows anything about these directly.

[Alexey Proskuryakov]

So this improves modularization of WebKit, too.

[Alexey Proskuryakov]

Created attachment 130209 [details]
updated to ToT

Would like EWS to run...

[Filip Pizlo]

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?

[Alexey Proskuryakov]

> 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.

[Filip Pizlo]

(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.

[Filip Pizlo]

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. :-)

[Alexey Proskuryakov]

Comment on attachment 130209 [details]
updated to ToT

Thank you for review!

[WebKit Review Bot]

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

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/110066>. Post-landing comments from CC'ed experts are still welcome.

[Alexey Proskuryakov]

Build fix in <http://trac.webkit.org/changeset/110080>.