Bug 168025 – Removed unused methods of WebCore::FileStream

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

[Karim]

Added missing methods implementation of WebCore::FileStream

[Karim]

Created attachment 300986 [details]
Patch

[Karim]

Created attachment 301066 [details]
Patch

[Darin Adler]

Comment on attachment 301066 [details]
Patch

Is someone calling these functions? Why implement functions we are not using?

[Michael Catanzaro]

Comment on attachment 301066 [details]
Patch

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

Wow. These functions have been unimplemented for years! At first I assumed that this could be hiding all sorts of interesting bugs, and I was planning to ask you if this fixed anything in particular. But I see that they are unimplemented on all ports. And I see that nothing uses these functions currently. Only NetworkDataTaskBlob and BlobResourceHandle use FileStream or AsyncFileStream, and the functions that are unimplemented in FileStream are only ever called in AsyncFileStream functions that are never themselves called.

Now, FileStream::write is still unimplemented on all ports, and you don't implement it here. So I think we need to consider what we want to do with this class. Implementing FileStream::openForWrite does not make much sense when all writes are guaranteed to fail, right? Also, keeping that function around does not make sense if it's not implemented. It does seem nice to have a more featureful FileStream API, but I'm not sure if the value of having a richer API really outweighs the cost of keeping around unused code. At any rate, we definitely need to add WebCore API tests for this code, if implementing these functions did not fix any existing tests.

Darin, I'm curious what you think we should do here. I'm inclined to suggest just removing the  affected functions, including FileStream::write and AsyncFileStream::write, so long as nothing is using them, but since they're easy to implement I guess we could keep them around unused if tests are added?

> Source/WebCore/ChangeLog:3
> +        Added missing methods implementations

This commit message isn't descriptive enough, and it should exactly match the title of the bug anyway. So write:

"Added missing methods implementation of WebCore::FileStream"

> Source/WebCore/platform/glib/FileSystemGlib.cpp:348
> +    return g_seekable_truncate(G_SEEKABLE(g_io_stream_get_input_stream(G_IO_STREAM(handle))), offset, 0, 0);

Use nullptr, not 0.

[Karim]

Created attachment 301260 [details]
Patch

[Darin Adler]

Normally, I suggest removing all unused code unless we have a specific idea of what we are going to use it for. That’s the rule I would apply here.

On the other hand I don’t see major harm in having untested functions that look correct and are relatively simple.

[Michael Catanzaro]

(In reply to comment #6)
> Normally, I suggest removing all unused code unless we have a specific idea
> of what we are going to use it for. That’s the rule I would apply here.

OK, I agree. Then instead of implementing these functions, we should remove them and also remove FileStream::write and AsyncFileStream::write.

[Karim]

(In reply to comment #7)
> (In reply to comment #6)
> > Normally, I suggest removing all unused code unless we have a specific idea
> > of what we are going to use it for. That’s the rule I would apply here.
> 
> OK, I agree. Then instead of implementing these functions, we should remove
> them and also remove FileStream::write and AsyncFileStream::write.

What about AsyncFileStream::truncate it depends on FileStream::truncate ?

[Michael Catanzaro]

(In reply to comment #8)
> What about AsyncFileStream::truncate it depends on FileStream::truncate ?

Try removing it too! It's clearly unused on non-Mac ports as it wouldn't link otherwise.

[Karim]

Created attachment 301314 [details]
Patch

[Michael Catanzaro]

Comment on attachment 301314 [details]
Patch

OK thanks. r=me assuming it passes EWS.

Kind of a shame that we don't get to use your implementations of the functions, but this seems better than keeping the unused code around.

[WebKit Commit Bot]

Comment on attachment 301314 [details]
Patch

Clearing flags on attachment: 301314

Committed r212327: <http://trac.webkit.org/changeset/212327>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.