[webkit-reviews] review requested: [Bug 44917] [chromium] Implement Readable/Writable versions of ChromiumDataObjectNew : [Attachment 66076] Patch

[bugzilla-daemon at webkit.org]

Daniel Cheng <dcheng at chromium.org> has asked  for review:
Bug 44917: [chromium] Implement Readable/Writable versions of
ChromiumDataObjectNew
https://bugs.webkit.org/show_bug.cgi?id=44917

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

------- Additional Comments from Daniel Cheng <dcheng at chromium.org>
(In reply to comment #3)
> (From update of attachment 65991 [details])
> > diff --git a/WebCore/platform/chromium/ReadableDataObject.cpp
b/WebCore/platform/chromium/ReadableDataObject.cpp
> > + * Copyright (c) 2008, 2009, Google Inc. All rights reserved.
> 
> Nit: 2010
> 

Done.

> > +static void ensureTypeCacheInitialized(bool isForDragging,
> > +					    bool& isTypeCacheInitialized,
> > +					    HashSet<String>& types,
> > +					    bool& containsFilenames)
> 
> It looks like all the calls to this pass in the same member variables.  Can
we just make it a member function?
> 

Done.

> > diff --git a/WebCore/platform/chromium/ReadableDataObject.h
b/WebCore/platform/chromium/ReadableDataObject.h
> > + * Copyright (c) 2008, 2009, Google Inc. All rights reserved.
> 
> Nit: 2010
> 

Done.

> > +	 // To avoid making a lot of IPC calls for each drag event, we cache
some
> > +	 // values in the renderer.
> > +	 // TODO: Is this actually the right place to do the caching? Isn't the
data
> 
> WebKit style is to use FIXME rather than TODO.
> 

Removed the comment, as I don't think there's a better place to do it anyway.

> > diff --git a/WebCore/platform/chromium/WritableDataObject.cpp
b/WebCore/platform/chromium/WritableDataObject.cpp
> > + * Copyright (c) 2008, 2009, Google Inc. All rights reserved.
> 
> Nit: 2010
> 

Done.

> > +// TODO: Verify it the implementations for read-only methods are needed.
Per the
> > +// code currently in ClipboardChromium, it's actually unnecessary, but we
should
> > +// double check if this is intended by the spec, especially since it
doesn't
> > +// really address when the clipboard object should be readable and when it

> > +// should be writable.
> 
> FIXME, also, "Verify it the" sounds funny.  I'm also not sure I understand
this comment.  Are you saying you're not sure if you need these methods at all?


Sorry, it was a typo. It should have been 'verify if'. And yes, I'm not sure if
an implementation is even needed for these methods.

> 
> > +void WritableDataObject::clearAllExceptFiles()
> > +{
> > +	 // TODO: The spec does not provide a way to populate FileList
currently. In
> > +	 // fact, the spec explicitly states that dragging files can only
happen from
> > +	 // outside a browsing context.
> 
> FIXME
> 

Done.

> > +void WritableDataObject::clear()
> 
> What is the difference between clear() and clearData()?  Why do we need both?

> 

clear() clears all the data and clearData() clears only one type of data. It
was originally inherited from Clipboard as well; I've renamed clear() to
clearAll().

> > +HashSet<String> WritableDataObject::types() const
> > +{
> 
> Would it be better for the caller to pass in a HasSet<String> to avoid a
copy?
> 

I opted to inherit most of the signatures from Clipboard.

> > +String WritableDataObject::getData(const String& type, bool& succeeded)
const
> > +{
> > +	 // TODO: For pasting, we need to read directly from the pasteboard.
Right
> > +	 // now, Clipboard::getData doesn't allow reads when the clipboard is
> > +	 // writable, but we have may to address this in the future.
> 
> FIXME
> 

Done.

> > diff --git a/WebCore/platform/chromium/WritableDataObject.h
b/WebCore/platform/chromium/WritableDataObject.h
> > + * Copyright (c) 2008, 2009, Google Inc. All rights reserved.
> 
> Nit: 2010

Done.

> 
> > +	 // Special accessors for URL and HTML since they carry additional
> > +	 // metadata.
> 
> I don't think you need to duplicate the comments from the parent class.
> 

Done.

> > +private:
> > +	 WritableDataObject(bool isForDragging);
> 
> Nit: explicit

Done.