[webkit-reviews] review denied: [Bug 44075] Add idl and mock classes for FileWriter. : [Attachment 64659] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Eric U. <ericu at chromium.org>'s
request for review:
Bug 44075: Add idl and mock classes for FileWriter.
https://bugs.webkit.org/show_bug.cgi?id=44075

Attachment 64659: Patch
https://bugs.webkit.org/attachment.cgi?id=64659&action=review

------- Additional Comments from David Levin <levin at chromium.org>

> 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;
> +    };
> +}