[webkit-reviews] review denied: [Bug 99355] plugins: Allow a plugin to dictate whether it can process drag or not : [Attachment 168763] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 15 13:33:57 PDT 2012


Tony Chang <tony at chromium.org> has denied sadrul at chromium.org's request for
review:
Bug 99355: plugins: Allow a plugin to dictate whether it can process drag or
not
https://bugs.webkit.org/show_bug.cgi?id=99355

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

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


> Source/WebCore/ChangeLog:4
> +	   Need the bug URL (OOPS!).

Need the bug URL (other ChangeLog files too).

> Source/WebCore/page/DragController.cpp:557
> +    if (result.innerNonSharedNode()->isPluginElement()) {
> +	   HTMLPlugInElement* plugin =
static_cast<HTMLPlugInElement*>(result.innerNonSharedNode());

It seems a bit weird that DragController needs to handle plugin elements
specially.  I guess DragController::operationForLoad also special cases
plugins, so maybe that's not a big deal.

> Source/WebCore/page/DragController.cpp:560
> +	   if (!plugin->canProcessDrag(*dragData))
> +	       return false;
> +    } else if (!result.innerNonSharedNode()->rendererIsEditable())

Doesn't this change the default behavior? If canProcessDrag returns the default
value, we'll never get to check the contentEditable case.  I.e., pages that
currently have contentEditable on an <embed> will no longer process drags.

> LayoutTests/platform/chromium/plugins/drag-events.html:12
> -<embed id="plugin" type="application/x-webkit-test-webplugin"
contentEditable></embed>
> +<embed id="plugin" type="application/x-webkit-test-webplugin"></embed>

This looks like we're replacing an old test with a new test.  Can we test both
(editable + !canProcessDrag and !editable + canProcessDrag)?


More information about the webkit-reviews mailing list