923054 - Convert DataTransfer to WebIDL bindings

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Move files and rename class v1

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Add WebIDL API and switch to the WebIDL binding v1

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 813071 [details] [diff] [review]
Add WebIDL API and switch to the WebIDL binding v1

Review of attachment 813071 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/events/src/DataTransfer.cpp
@@ +142,5 @@
> +                          const nsAString& aEventType, bool aIsExternal,
> +                          ErrorResult& aRv)
> +{
> +  nsAutoCString onEventType("on");
> +  AppendUTF16toUTF8(aEventType, onEventType);

Looks like doing this in UTF-16 might be less work

Comment 4

[Jan Varga [:janv]]

Comment on attachment 813071 [details] [diff] [review]
Add WebIDL API and switch to the WebIDL binding v1

Review of attachment 813071 [details] [diff] [review]:
-----------------------------------------------------------------

The new DataTransfer + DOMStringList impl seems to work even on workers.
However there's a strange crash in memory reporter tests.
https://tbpl.mozilla.org/?tree=Try&rev=2717af4b60a0

I quickly went through the patch and found some probably unrelated "nits".

::: content/events/src/DataTransfer.cpp
@@ +261,2 @@
>    if (mEventType != NS_DRAGDROP_DROP && mEventType != NS_DRAGDROP_DRAGDROP &&
>        mEventType != NS_PASTE) {

shouldn't you throw here ?

@@ +497,5 @@
>  NS_IMETHODIMP
>  DataTransfer::GetMozSourceNode(nsIDOMNode** aSourceNode)
>  {
> +  nsCOMPtr<nsINode> sourceNode = GetMozSourceNode();
> +  return sourceNode ? CallQueryInterface(sourceNode, aSourceNode) : NS_OK;

aSourceNode might end up being uninitialized ?

Comment 5

[Jan Varga [:janv]]

Comment on attachment 813071 [details] [diff] [review]
Add WebIDL API and switch to the WebIDL binding v1

Review of attachment 813071 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/events/src/DataTransfer.cpp
@@ +50,5 @@
>  NS_IMPL_CYCLE_COLLECTING_ADDREF(DataTransfer)
>  NS_IMPL_CYCLE_COLLECTING_RELEASE(DataTransfer)
>  
>  NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(DataTransfer)
> +  NS_INTERFACE_MAP_ENTRY(mozilla::dom::DataTransfer)

nsWrapperCache doesn't need to be hooked up to the cycle collector ?

::: content/events/src/DataTransfer.h
@@ +45,5 @@
>  };
>  
> +#define NS_DATATRANSFER_IID \
> +{ 0xe24a9ddc, 0x2979, 0x40e3, \
> +  { 0x82, 0xb0, 0x9d, 0xf8, 0xb0, 0x41, 0xe5, 0x6a } }

A copy and paste issue ? It defines the same IID as NS_INODE_IID, it's very likely that this is causing the crash.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Peter, what's this blocked on?

Comment 7

[Peter Van der Beken [:peterv]]

Figuring out the right parent I think.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hmm.  DataTransfer instances only come from events, right?  Could we just tell the DataTransfer what event it's for?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Yeah, I think that should work, especially because http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#4964 clones DataTransfer. (and in other cases we just create a new one anyway)

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: Move files and rename class v2

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: Add WebIDL API and switch to the WebIDL binding v2

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8362942 [details] [diff] [review]
Add WebIDL API and switch to the WebIDL binding v2

>+  } else {
>     // A dataTransfer won't exist when a drag was started by some other
>     // means, for instance calling the drag service directly, or a drag
>     // from another application. In either case, a new dataTransfer should
>     // be created that reflects the data.
>-    initialDataTransfer = new DataTransfer(aDragEvent->message, true, -1);
>-
>-    NS_ENSURE_TRUE(initialDataTransfer, NS_ERROR_OUT_OF_MEMORY);
>+    initialDataTransfer = new DataTransfer(aDragEvent->target, aDragEvent->message, true, -1);
Do we need to initialize this DataTransfer with aDragEvent->target?
I guess it is fine.
> nsDOMClipboardEvent::InitClipboardEvent(const nsAString& aType,
>                                         bool aCanBubble,
>                                         bool aCancelable,
>                                         nsIDOMDataTransfer* aClipboardData)
> {
>-  nsresult rv = nsDOMEvent::InitEvent(aType, aCanBubble, aCancelable);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  nsCOMPtr<DataTransfer> clipboardData = do_QueryInterface(aClipboardData);
>+  NS_ENSURE_ARG(clipboardData);
Null as aClipboardData param should be ok, so drop NS_ENSURE_ARG

>+void
>+nsDOMDragEvent::InitDragEvent(const nsAString& aType, bool aCanBubble,
>+                              bool aCancelable, nsIDOMWindow* aView,
>+                              int32_t aDetail, int32_t aScreenX,
>+                              int32_t aScreenY, int32_t aClientX,
>+                              int32_t aClientY, bool aCtrlKey, bool aAltKey,
>+                              bool aShiftKey, bool aMetaKey, uint16_t aButton,
>+                              EventTarget* aRelatedTarget,
>+                              DataTransfer* aDataTransfer, ErrorResult& aError)
>+{
>+  aError = nsDOMMouseEvent::InitMouseEvent(aType, aCanBubble, aCancelable,
>+                  aView, aDetail, aScreenX, aScreenY, aClientX, aClientY,
>+                  aCtrlKey, aAltKey, aShiftKey, aMetaKey, aButton,
>+                  aRelatedTarget);
align params under aType, not under useEvent


>+      nsCOMPtr<nsISupports> container = aPresContext->GetContainerWeak();
>+      nsCOMPtr<nsPIDOMWindow> window = do_GetInterface(container);
>+      if (!window)
>+        return;
{}


>+
>       nsRefPtr<DataTransfer> dataTransfer =
>-        new DataTransfer(NS_DRAGDROP_START, false, -1);
>-      if (!dataTransfer)
>-        return;
>+        new DataTransfer(window, NS_DRAGDROP_START, false, -1);
> 
>       nsCOMPtr<nsISelection> selection;
>       nsCOMPtr<nsIContent> eventContent, targetContent;
>       mCurrentTarget->GetContentForEvent(aEvent, getter_AddRefs(eventContent));
>       if (eventContent)
>-        DetermineDragTarget(aPresContext, eventContent, dataTransfer,
>+        DetermineDragTarget(window, eventContent, dataTransfer,
>                             getter_AddRefs(selection), getter_AddRefs(targetContent));
> 
>       // Stop tracking the drag gesture now. This should stop us from
>       // reentering GenerateDragGesture inside DOM event processing.
>       StopTrackingDragGesture();
> 
>       if (!targetContent)
>         return;
Hmm, what guarantees that DataTranfer has the same parent global as targetContent.
I think I'd prefer adding explicit SetParentObject to DataTransfer and pass targetContent here.
Then in DataTransfer GetParentObject assert that it is not null.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff addressing Olli's review comments on part 2

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1 - Convert DataTransfer to WebIDL bindings, move files and rename class.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2 - Convert DataTransfer to WebIDL bindings, add WebIDL API and switch to the WebIDL binding.

Comment 16

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/6dacf81bb525
   https://hg.mozilla.org/integration/mozilla-inbound/rev/a40bcf02bb60

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6dacf81bb525
https://hg.mozilla.org/mozilla-central/rev/a40bcf02bb60

Comment 18

[Gary [:streetwolf52]]

Based on my regression testing it appears this patch brakes the addon <a href="https://addons.mozilla.org/en-US/firefox/addon/super-drag/?src=ss">Super Drag :: Add-ons for Firefox</a>

Good:  https://hg.mozilla.org/integration/mozilla-inbound/rev/26bfe4ef1bc2
Bad:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a40bcf02bb60

Comment 19

[Boris Zbarsky [:bzbarsky]]

Gary, can you please file a bug on that, with clear steps to reproduce?

Comment 20

[Carsten Book [:Tomcat]]

bz: do we need to back this changes out. Ms2ger told me to back this change out but due to 975688 the backout does not apply cleanly.

Comment 21

[Boris Zbarsky [:bzbarsky]]

The backout doesn't apply cleanly because other bugs depend on this one.  I wish people had cced me on those regressions... looking into them now.

Comment 22

[Boris Zbarsky [:bzbarsky]]

OK, patches up for all known regressions.