[webkit-reviews] review denied: [Bug 30469] We should not bubble up events if we drag something to an iframe that has an invalid source : [Attachment 41531] Proposed Patch

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 30469: We should not bubble up events if we drag something to an iframe
that has an invalid source
https://bugs.webkit.org/show_bug.cgi?id=30469

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

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
Almost there.

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> +	   Tests that we don't bubble up the events if we drag something to an
> +	   iframe that has an invalid source.

Now that you have a test that verifies both bubbling with valid and invalid
targets, this description could change to reflect this.

> +	   *
http/tests/misc/drag-over-iframe-invalid-source-does-not-bubble-expected.txt:
Added.
> +	   *
http/tests/misc/drag-over-iframe-invalid-source-does-not-bubble.html: Added.

I guess the test name could now also be different. Perhaps
"bubble-drag-events-from-iframe.html"?

> diff --git
a/LayoutTests/http/tests/misc/drag-over-iframe-invalid-source-does-not-bubble.h
tml
b/LayoutTests/http/tests/misc/drag-over-iframe-invalid-source-does-not-bubble.h
tml
> +function log(msg)
> +{
> +    document.getElementById("log").appendChild(document.createTextNode(msg +
"\n"));
> +}

Mix of styles - opening '{' of a functions follows different style through the
file. Need to stick to one style.

> +    testDragEventBubbling("dragTargetParent", "dragTarget");
> +
> +    log("Tests that we bubble up the events if we drag something to a valid
source");
> +    testDragEventBubbling("dragTargetParent2", "dragTarget2");

Perhaps better names for valid/invalid targets could be
validDragTarget/validDragTargetParent and
invalidDragTarget/invalidDragTartgetParent?

> +
> +<div id="dragTargetParent">
> +<iframe id="dragTarget" src="file:"></iframe>
> +</div>
> +<div contentEditable id="dragTargetParent2">
> +<span id="dragTarget2">Drag here.</span>
> +</div>

In one case it is iframe and in another a span. Should it be both iframes, one
with valid url and another with invalid one?
This way, only one variable would change.

> diff --git a/WebCore/page/EventHandler.cpp b/WebCore/page/EventHandler.cpp
> +bool EventHandler::handleDragAndDropForTarget(DragAndDropHandleType type,
Node* target, const AtomicString& eventType, const PlatformMouseEvent& event,
Clipboard* clipboard)
> +		   default:
> +		       ASSERT(false);
> +		       break;

The 'default' can be removed. Now it can not be hit and if someone adds more
values to the enum, gcc will issue a warning in compile time, but if there is
'default' it won't.
r- because it seems the valid target in the test should also be iframe.