Peter Van der Beken [:peterv] Assignee
	Description • 11 years ago

      No description provided.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 3 • 11 years ago

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

	Jan Varga [:janv]
	Comment 4 • 11 years ago

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 ?

	Jan Varga [:janv]
	Comment 5 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 11 years ago

Peter, what's this blocked on?

	Peter Van der Beken [:peterv] Assignee
	Comment 7 • 11 years ago

Figuring out the right parent I think.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 11 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 9 • 11 years ago

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)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 12 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 17 • 10 years ago

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

	Gary [:streetwolf52]
	Comment 18 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 20 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 10 years ago

OK, patches up for all known regressions.