Bug 44362 – Implement the DOM FileWriter class

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Eric U.]

This bug is for fleshing out html/FileWriter; it will depend on an abstract class which will be implemented in a separate bug.

[Eric U.]

Created attachment 65000 [details]
Implement html/FileWriter

This implements FileWriter, but delegates the hard work to the forthcoming implementation of AsyncFileWriter, which is abstract.

[Kinuko Yasuda]

I'm not a reviewer and I haven't looked into it carefully yet, but here're some first comments.

WebCore/html/FileWriter.cpp:45
 +      , m_length(length)
Don't we want to initialize m_readyState as well?

WebCore/html/FileWriter.cpp:76
 +          ec = INVALID_STATE_ERR;
I think we need to save the error value to m_error to return the last error for FileWriter.error attribute.

WebCore/html/FileWriter.cpp:134
 +      DCHECK_GT(bytes, 0);
I don't think we have DCHECK_XXX stuff in WebKit.  (ASSERT(...) is used everywhere in WebKit)

WebCore/html/FileWriter.cpp:170
 +          return;
In what case does this happen?

WebCore/html/FileWriter.h:69
 +      FileError* error() const { return m_error; };
m_error.get()?

WebCore/html/FileWriter.h:109
 +      RefPtr<FileError*> m_error;
'*' is not necessary.

WebCore/html/FileWriter.h:111
 +      RefPtr<AsyncFileWriter*> m_writer;
ditto.

WebCore/html/FileWriter.cpp:58
 +      return m_readyState == WRITING || ActiveDOMObject::hasPendingActivity();
If we want to keep an instance while its hasPendingActivity() returns true, we need to add the class name to WebCore/bindings/scripts/CodeGeneratorV8.pm::IsActiveDomType() for v8.  Otherwise it gets GC'ed even it is defined as a subclass of ActiveDOMObject.

[Kinuko Yasuda]

WebCore/html/FileWriter.cpp:39
 +  FileWriter::FileWriter(ScriptExecutionContext* context, AsyncFileWriter* writer, const String& path, long long length)
Does this assume the creator/caller of FileWriter holds the AsyncFileWriter's reference while the writer is writing?

[Eric U.]

Comment on attachment 65000 [details]
Implement html/FileWriter

This patch needs a lot of work; I'm retracting it and will repost soon.

[Eric U.]

Created attachment 65644 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 65644 [details]
Patch

WebCore/html/FileWriterClient.h:54
 +  
nit: looks like there are some extra new lines here that should be nuked.


R=me

[Eric U.]

Created attachment 65771 [details]
Fixed whitespace, pulled extra entry out of vcproj

I removed those two blank lines, and I also removed an entry for FileWriterCallback from the vcproj file--that snuck in from the next changelist.  Everything else is the same.

[WebKit Commit Bot]

Comment on attachment 65771 [details]
Fixed whitespace, pulled extra entry out of vcproj

Clearing flags on attachment: 65771

Committed r66303: <http://trac.webkit.org/changeset/66303>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.