Bug 44362 - Implement the DOM FileWriter class
Summary: Implement the DOM FileWriter class
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Eric U.
Depends on:
Blocks: 44358
  Show dependency treegraph
Reported: 2010-08-20 14:45 PDT by Eric U.
Modified: 2010-08-28 01:49 PDT (History)
7 users (show)

See Also:

Implement html/FileWriter (20.81 KB, patch)
2010-08-20 15:17 PDT, Eric U.
no flags Details | Formatted Diff | Diff
Patch (26.94 KB, patch)
2010-08-26 16:33 PDT, Eric U.
fishd: review+
fishd: commit-queue-
Details | Formatted Diff | Diff
Fixed whitespace, pulled extra entry out of vcproj (26.72 KB, patch)
2010-08-27 14:54 PDT, Eric U.
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric U. 2010-08-20 14:45:32 PDT
This bug is for fleshing out html/FileWriter; it will depend on an abstract class which will be implemented in a separate bug.
Comment 1 Eric U. 2010-08-20 15:17:59 PDT
Created attachment 65000 [details]
Implement html/FileWriter

This implements FileWriter, but delegates the hard work to the forthcoming implementation of AsyncFileWriter, which is abstract.
Comment 2 Kinuko Yasuda 2010-08-21 00:01:50 PDT
I'm not a reviewer and I haven't looked into it carefully yet, but here're some first comments.

 +      , m_length(length)
Don't we want to initialize m_readyState as well?

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

 +      DCHECK_GT(bytes, 0);
I don't think we have DCHECK_XXX stuff in WebKit.  (ASSERT(...) is used everywhere in WebKit)

 +          return;
In what case does this happen?

 +      FileError* error() const { return m_error; };

 +      RefPtr<FileError*> m_error;
'*' is not necessary.

 +      RefPtr<AsyncFileWriter*> m_writer;

 +      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.
Comment 3 Kinuko Yasuda 2010-08-21 00:26:02 PDT
 +  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?
Comment 4 Eric U. 2010-08-23 13:58:45 PDT
Comment on attachment 65000 [details]
Implement html/FileWriter

This patch needs a lot of work; I'm retracting it and will repost soon.
Comment 5 Eric U. 2010-08-26 16:33:45 PDT
Created attachment 65644 [details]
Comment 6 Darin Fisher (:fishd, Google) 2010-08-27 14:42:35 PDT
Comment on attachment 65644 [details]

nit: looks like there are some extra new lines here that should be nuked.

Comment 7 Eric U. 2010-08-27 14:54:26 PDT
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.
Comment 8 WebKit Commit Bot 2010-08-28 01:49:28 PDT
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>
Comment 9 WebKit Commit Bot 2010-08-28 01:49:35 PDT
All reviewed patches have been landed.  Closing bug.