[webkit-reviews] review denied: [Bug 44917] [chromium] Implement Readable/Writable versions of ChromiumDataObjectNew : [Attachment 65991] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Aug 30 18:39:43 PDT 2010
Tony Chang <tony at chromium.org> has denied Daniel Cheng <dcheng at chromium.org>'s
request for review:
Bug 44917: [chromium] Implement Readable/Writable versions of
ChromiumDataObjectNew
https://bugs.webkit.org/show_bug.cgi?id=44917
Attachment 65991: Patch
https://bugs.webkit.org/attachment.cgi?id=65991&action=review
------- Additional Comments from Tony Chang <tony at chromium.org>
> 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
More information about the webkit-reviews
mailing list