[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