[Webkit-unassigned] [Bug 44917] [chromium] Implement Readable/Writable versions of ChromiumDataObjectNew
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Aug 31 11:06:14 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=44917
Daniel Cheng <dcheng at chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #65991|0 |1
is obsolete| |
Attachment #66076| |review?, commit-queue?
Flag| |
--- Comment #4 from Daniel Cheng <dcheng at chromium.org> 2010-08-31 11:06:14 PST ---
Created an attachment (id=66076)
--> (https://bugs.webkit.org/attachment.cgi?id=66076)
Patch
(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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list