Bug 44075 – Add idl and mock classes for FileWriter.

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

[Eric U.]

Add the idl and stubbed-out classes for the FileWriter class, as specified in http://dev.w3.org/2009/dap/file-system/file-writer.html.

[Eric U.]

Created attachment 64659 [details]
Patch

[David Levin]

Comment on attachment 64659 [details]
Patch


> Index: WebCore/dom/EventNames.h
> @@ -103,6 +103,9 @@ namespace WebCore {
>      macro(textInput) \
>      macro(unload) \
>      macro(updateready) \
> +    macro(write) \
> +    macro(writestart) \
> +    macro(writeend) \

I understand the logic of putting start before end but I htink it would be better just to sort alphabetically like many -- but not all :( -- of these items are.


> Index: WebCore/dom/EventTarget.cpp
> +FileWriter* EventTarget::toFileWriter()
> +{
> +    return 0;
> +}

Shouldn't this be guarded by "FILE_WRITER" instead of "BLOB"?


> Index: WebCore/dom/EventTarget.h
>  #if ENABLE(BLOB)
>          virtual FileReader* toFileReader();
> +        virtual FileWriter* toFileWriter();

Shouldn't this be guarded by "FILE_WRITER" instead of "BLOB"?


> Index: WebCore/html/FileWriter.h
> +#include "FileError.h"

I suspect when you change the return type of error() (see below) that you can remove this #include and simmply have a fwd declaration for FileError.



> +// FIXME: This is an empty, do-nothing placeholder for implementation yet to come.
> +class FileWriter : public RefCounted<FileWriter>, public ActiveDOMObject, public EventTarget {
> +    virtual ~FileWriter();

When I was doing some coding in Chromium, I saw this practice of making the destructor private and making the RefCounted class a friend. It looks like a good practice, so please consider it.


> +
> +    enum ReadyState {
> +        EMPTY = 0,
> +        WRITING = 1,
> +        DONE = 2

"Enum members should user InterCaps with an initial capital letter." -- http://webkit.org/coding/coding-style.html
Also I wonder if empty should be init -- see comment in idl file.

Lastly, it looks like you are keeping these in step with the idl file. It seems like there should be a compile assert somewhere to verify this.



> +    PassRefPtr<FileError> error() const { return m_error; };
This should be a FileError* 

"* If a function’s result is an object, but ownership is not being transferred, the result should be a raw pointer. This includes most getter functions.
* If a function’s result is a new object or ownership is being transferred for any other reason, the result should be a PassRefPtr. Since local variables are typically RefPtr, it’s common to call release in the return statement to transfer the RefPtr to the PassRefPtr."

-- http://webkit.org/coding/RefPtr.html


> Index: WebCore/html/FileWriter.idl
> +module html {
> +    interface [
> +        Conditional=BLOB,

Shouldn't this be "FILE_WRITER" instead of "BLOB"?

> +        CallWith=ScriptExecutionContext,
> +        EventTarget,
> +        NoStaticTables
> +    ] FileWriter {
> +        // ready states
> +        const unsigned short EMPTY = 0;

In http://www.w3.org/TR/file-writer-api/, this is INIT (instead of EMPTY).

> +        const unsigned short WRITING = 1;
> +        const unsigned short DONE = 2;
> +        readonly attribute unsigned short readyState;
> +
> +        // async write/modify methods

Ideally comments start with a capital and end with a "."

> +        void write(in Blob data);

add "raises (FileException)"

> +        void seek(in long long position);

add "raises (FileException)"

> +        void truncate(in long long size);

add "raises (FileException)"

> +
> +        void abort();
> +
> +        readonly attribute FileError error;
> +        readonly attribute long long position;
> +        readonly attribute long long length;
> +
> +        attribute EventListener onwritestart;

In other idl files, I've noticed that "attribute" is vertically aligned. (Space or readonly is always left, just like the specs.)

> +        attribute EventListener onprogress;
> +        attribute EventListener onwrite;
> +        attribute EventListener onabort;
> +        attribute EventListener onerror;
> +        attribute EventListener onwriteend;
> +    };
> +}

[Eric U.]

Created attachment 64743 [details]
Patch

[Eric U.]

>> Index: WebCore/dom/EventNames.h
>> @@ -103,6 +103,9 @@ namespace WebCore {
>>      macro(textInput) \
>>      macro(unload) \
>>      macro(updateready) \
>> +    macro(write) \
>> +    macro(writestart) \
>> +    macro(writeend) \
>
> I understand the logic of putting start before end but I htink it would be better just to sort alphabetically like many -- but not all :( -- of these items are.

Fixed.

>> Index: WebCore/dom/EventTarget.cpp
>> +FileWriter* EventTarget::toFileWriter()
>> +{
>> +    return 0;
>> +}
>
> Shouldn't this be guarded by "FILE_WRITER" instead of "BLOB"?

See below.

>> Index: WebCore/dom/EventTarget.h
>>  #if ENABLE(BLOB)
>>          virtual FileReader* toFileReader();
>> +        virtual FileWriter* toFileWriter();
>
> Shouldn't this be guarded by "FILE_WRITER" instead of "BLOB"?
>
>
>> Index: WebCore/html/FileWriter.h
>> +#include "FileError.h"
>
> I suspect when you change the return type of error() (see below) that you can remove this #include and simmply have a fwd declaration for FileError.

See below.

>> +// FIXME: This is an empty, do-nothing placeholder for implementation yet to come.
>> +class FileWriter : public RefCounted<FileWriter>, public ActiveDOMObject, public EventTarget {
>> +    virtual ~FileWriter();
>
> When I was doing some coding in Chromium, I saw this practice of making the destructor private and making the RefCounted class a friend. It looks like a good practice, so please consider it.

Sounds like a fine idea; done.

>> +    enum ReadyState {
>> +        EMPTY = 0,
>> +        WRITING = 1,
>> +        DONE = 2
>
> "Enum members should user InterCaps with an initial capital letter." -- http://webkit.org/coding/coding-style.html

Can't do that--see below re: COMPILE_ASSERT.

> Also I wonder if empty should be init -- see comment in idl file.

Yup--typo based on my using FileReader as a template.

> Lastly, it looks like you are keeping these in step with the idl file. It seems like there should be a compile assert somewhere to verify this.

The generated JSFileWriter.cpp does this automatically.  It contains
lines like this:

  COMPILE_ASSERT(0 == FileWriter::INIT,
FileWriterEnumINITIsWrongUseDontCheckEnums);

...which incidentally force the enum to be INIT, not Init.

>> +    PassRefPtr<FileError> error() const { return m_error; };
> This should be a FileError*
>
> "* If a function’s result is an object, but ownership is not being transferred, the result should be a raw pointer. This includes most getter functions.
> * If a function’s result is a new object or ownership is being transferred for any other reason, the result should be a PassRefPtr. Since local variables are typically RefPtr, it’s common to call release in the return statement to transfer the RefPtr to the PassRefPtr."

This is copied right from FileReader; I'm assuming the point is that
the reference to the FileError might outlive the member variable
[perhaps if someone calls abort() after a previous failure led to a
call to didFail()?].  So ownership is not transferred as such, but
shared.  It seems like FileWriter could end up in a similar situation.

> -- http://webkit.org/coding/RefPtr.html
>
>
>> Index: WebCore/html/FileWriter.idl
>> +module html {
>> +    interface [
>> +        Conditional=BLOB,
>
> Shouldn't this be "FILE_WRITER" instead of "BLOB"?

Actually, it should probably be FILE_SYSTEM, as this is really only
useful for the filesystem API for now, and we don't really need a
separate flag just for it.  Fixed [all references].

>> +        CallWith=ScriptExecutionContext,
>> +        EventTarget,
>> +        NoStaticTables
>> +    ] FileWriter {
>> +        // ready states
>> +        const unsigned short EMPTY = 0;
>
> In http://www.w3.org/TR/file-writer-api/, this is INIT (instead of EMPTY).

Fixed.

>> +        const unsigned short WRITING = 1;
>> +        const unsigned short DONE = 2;
>> +        readonly attribute unsigned short readyState;
>> +
>> +        // async write/modify methods
>
> Ideally comments start with a capital and end with a "."

Here they're far too short to be actual sentences; do you think it's
an improvement to make them look like ungrammatical sentences, or
better to flesh them out into longer comments?  I feel like either
would make them less readable/useful.

>> +        void write(in Blob data);
>
> add "raises (FileException)"
>
>> +        void seek(in long long position);
>
> add "raises (FileException)"
>
>> +        void truncate(in long long size);
>
> add "raises (FileException)"

Fixed [abort too].

>> +
>> +        void abort();
>> +
>> +        readonly attribute FileError error;
>> +        readonly attribute long long position;
>> +        readonly attribute long long length;
>> +
>> +        attribute EventListener onwritestart;
>
> In other idl files, I've noticed that "attribute" is vertically aligned. (Space or readonly is always left, just like the specs.)

Done.

[Eric U.]

Created attachment 64749 [details]
Patch

[Eric U.]

Changed as discussed offline.
Note that the new event names in EventNames.h are unconditional, as with all the others there.

[Kinuko Yasuda]

(Oh so now it's guarded by FILE_WRITER... looks good to me:))

> +class FileWriter : public RefCounted<FileWriter>, public ActiveDOMObject, public EventTarget {

One point; as for ActiveDOMObject... as far as I believe we need to add the class name in IsActiveDomType method in WebCore/bindings/scripts/CodeGeneratorV8.pm to make it kept in the wrapper in v8.  Same for FileReader.   (Am I right?)

[WebKit Commit Bot]

Comment on attachment 64749 [details]
Patch

Rejecting patch 64749 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'David Levin', u'--force']" exit_code: 1
Last 500 characters of output:
6 (offset 2 lines).
Hunk #2 succeeded at 6727 (offset 6 lines).
Hunk #3 succeeded at 14020 (offset 7 lines).
Hunk #4 FAILED at 20320.
Hunk #5 FAILED at 22770.
2 out of 5 hunks FAILED -- saving rejects to file WebCore/WebCore.xcodeproj/project.pbxproj.rej
patching file WebCore/dom/EventNames.h
patching file WebCore/dom/EventTarget.cpp
patching file WebCore/dom/EventTarget.h
patching file WebCore/html/FileWriter.cpp
patching file WebCore/html/FileWriter.h
patching file WebCore/html/FileWriter.idl

Full output: http://queues.webkit.org/results/3799038

[Eric U.]

Created attachment 64796 [details]
Patch

[Eric U.]

Re-posted the same patch with the merge conflict resolved.

[WebKit Commit Bot]

Comment on attachment 64796 [details]
Patch

Clearing flags on attachment: 64796

Committed r65656: <http://trac.webkit.org/changeset/65656>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.