[webkit-reviews] review denied: [Bug 23695] Drag data store should be readable in dragstart event : [Attachment 194415] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Daniel Cheng <dcheng at chromium.org>'s
request for review:
Bug 23695: Drag data store should be readable in dragstart event
https://bugs.webkit.org/show_bug.cgi?id=23695

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=194415&action=review


The code change for the bug fix is actually pretty small.  I recomment
splitting this into two changes.  First, a refactor change that renames
ClipboardWritable to ClipboardReadableAndWritable, adds the can*() methods and
switches to using them.  A second change with just the bug fix (should be
pretty small at that point).

> Source/WebCore/ChangeLog:18
> +	   implementations to check permissions.
> +
> +	   Tests: editing/pasteboard/can-read-in-copy-and-cut-events.html

You should also mention that this change allows reading the data store during
cut and copy events.

> Source/WebCore/ChangeLog:38
> +	   (WebCore::Editor::tryDHTMLCopy):
> +	   (WebCore::Editor::tryDHTMLCut):

Please highlight the behavior changes in this part of the ChangeLog.  E.g., the
cut/copy bug fix is here.

> Source/WebCore/ChangeLog:48
> +	   * page/EventHandler.cpp:
> +	   (WebCore::EventHandler::handleDrag):

And this is the drag fix.

> Source/WebCore/dom/Clipboard.cpp:54
> +    ASSERT(m_policy != None || policy == ClipboardNumb);
> +    switch (policy) {
> +    case ClipboardReadable:
> +	   m_policy = static_cast<AccessPolicy>(ReadableTypes | ReadableData);
> +	   break;

Having m_policy be a different type doesn't seem to be a huge benefit and is a
bit confusing.	Why not keep m_policy as a ClipboardAccessPolicy and use the
helper methods (canReadTypes(), canReadData(), etc) for checking the different
enum values?

> Source/WebCore/dom/Clipboard.cpp:65
> +    default:

No default needed and without it, adding a new value to ClipboardAccessPolicy
will trigger a compiler error.