[Webkit-unassigned] [Bug 44917] [chromium] Implement Readable/Writable versions of ChromiumDataObjectNew

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=44917


Tony Chang <tony at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #65991|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #3 from Tony Chang <tony at chromium.org>  2010-08-30 18:39:43 PST ---
(From update of attachment 65991)
> 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

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

> 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

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

> 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

> +// 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?

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

> +void WritableDataObject::clear()

What is the difference between clear() and clearData()?  Why do we need both?

> +HashSet<String> WritableDataObject::types() const
> +{

Would it be better for the caller to pass in a HasSet<String> to avoid a copy?

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

> 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

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

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

Nit: explicit

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.