Get rid of nsIDOMDataTransfer

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
Last year
4 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 2 bugs)

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(14 attachments)

9.87 KB, patch
Nika
: review+
Details | Diff | Splinter Review
7.02 KB, patch
Nika
: review+
Details | Diff | Splinter Review
23.70 KB, patch
Nika
: review+
Details | Diff | Splinter Review
9.23 KB, patch
Nika
: review+
Details | Diff | Splinter Review
6.25 KB, patch
Nika
: review+
Details | Diff | Splinter Review
6.75 KB, patch
Nika
: review+
Details | Diff | Splinter Review
10.66 KB, patch
Nika
: review+
Details | Diff | Splinter Review
3.53 KB, patch
Nika
: review+
Details | Diff | Splinter Review
7.79 KB, patch
Nika
: review-
Details | Diff | Splinter Review
22.67 KB, patch
Nika
: review+
Details | Diff | Splinter Review
8.98 KB, patch
Nika
: review+
Details | Diff | Splinter Review
1.92 KB, patch
Nika
: review+
Details | Diff | Splinter Review
8.05 KB, patch
Nika
: review+
Details | Diff | Splinter Review
6.05 KB, patch
Nika
: review+
Details | Diff | Splinter Review
No description provided.
Depends on: 1444919
MozReview-Commit-ID: 6Kn9uuaQYI0
Attachment #8958239 - Flags: review?(nika)
MozReview-Commit-ID: 1eo6czER8Qw
Attachment #8958240 - Flags: review?(nika)
MozReview-Commit-ID: G7vuh1uuWGv
Attachment #8958241 - Flags: review?(nika)
MozReview-Commit-ID: EQ8KXMf4mnR
Attachment #8958242 - Flags: review?(nika)
MozReview-Commit-ID: GIzIU7nWP5j
Attachment #8958243 - Flags: review?(nika)
MozReview-Commit-ID: 3gAWLI2DyyU
Attachment #8958244 - Flags: review?(nika)
MozReview-Commit-ID: 53ShdRZHlC9
Attachment #8958245 - Flags: review?(nika)
MozReview-Commit-ID: 2MR9MVbdOPP
Attachment #8958246 - Flags: review?(nika)
MozReview-Commit-ID: Dpn7YSZpDsc
Attachment #8958247 - Flags: review?(nika)
MozReview-Commit-ID: LwKqWBGXVcN
Attachment #8958248 - Flags: review?(nika)
MozReview-Commit-ID: CRmiSTSN8af
Attachment #8958249 - Flags: review?(nika)
MozReview-Commit-ID: 7hseR1dpfWG
Attachment #8958250 - Flags: review?(nika)
MozReview-Commit-ID: EFauqLMGz5S
Attachment #8958251 - Flags: review?(nika)
MozReview-Commit-ID: BLi4w10clkP
Attachment #8958252 - Flags: review?(nika)
Attachment #8958239 - Flags: review?(nika) → review+
Attachment #8958240 - Flags: review?(nika) → review+
Comment on attachment 8958241 [details] [diff] [review]
part 3.  Get rid of nsIDOMDataTransfer::Get/SetMozCursor

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

::: dom/base/nsContentUtils.cpp
@@ +6041,5 @@
>  
>    nsCOMPtr<nsIDragSession> dragSession = GetDragSession();
>    NS_ENSURE_TRUE(dragSession, NS_OK); // no drag in progress
>  
> +  nsCOMPtr<DataTransfer> initialDataTransfer =

Does this have to be a COMPtr? Can we make it a RefPtr?

::: widget/nsIDragSession.idl
@@ +77,5 @@
>  
>    /**
>     * The data transfer object for the current drag.
>     */
> +  [binaryname(DataTransferXPCOM)]

Thanks for changing this type's name - you probably knew this but windows reorders the vtable when there are overloaded methods, so xpconnect doesn't work properly :-/

::: widget/windows/nsNativeDragSource.h
@@ +58,5 @@
>    // Reference count
>    ULONG m_cRef;
>  
>    // Data object, hold information about cursor state
> +  nsCOMPtr<mozilla::dom::DataTransfer> mDataTransfer;

refptr?
Attachment #8958241 - Flags: review?(nika) → review+
Comment on attachment 8958242 [details] [diff] [review]
part 4.  Get rid of nsIDOMDataTransfer::Get/SetDropEffectInt

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

::: dom/events/DataTransfer.h
@@ +240,5 @@
>  
>    already_AddRefed<nsINode> GetMozSourceNode();
>  
> +  /*
> +   * Integer version of dropEffect, set to one of the constants in nsIDragService.

It'd be nice to change this into an enum class defined on DataTransfer or another related type at some point.

@@ +248,5 @@
> +    return mDropEffect;
> +  }
> +  void SetDropEffectInt(uint32_t aDropEffectInt)
> +  {
> +    mDropEffect = aDropEffectInt;

We definitely do an unchecked index into a fixed-size array with this integer in GetDropEffect()...

Can we perhaps assert that this is in range here? The size of that array is known in the header.
Attachment #8958242 - Flags: review?(nika) → review+
I should note that for ^ IMO we should MOZ_RELEASE_ASSERT it because it's important for correctness.
Attachment #8958243 - Flags: review?(nika) → review+
Comment on attachment 8958244 [details] [diff] [review]
part 6.  Get rid of nsIDOMDataTransfer::GetFiles

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

::: dom/events/DataTransfer.h
@@ +189,5 @@
>  
> +  /**
> +   * Holds a list of all the local files available on this data transfer.
> +   * A dataTransfer containing no files will return an empty list, and an
> +   * invalid index access on the resulting file list will return null. 

nit: trailing whitespace
Attachment #8958244 - Flags: review?(nika) → review+
Attachment #8958245 - Flags: review?(nika) → review+
Attachment #8958246 - Flags: review?(nika) → review+
Comment on attachment 8958247 [details] [diff] [review]
part 9.  Remove use of nsIDOMDataTransfer from nsITreeView

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

::: layout/xul/tree/nsITreeView.idl
@@ +75,5 @@
>     * the current location. To get the behavior where drops are only allowed on
>     * items, such as the mailNews folder pane, always return false when
>     * the orientation is not DROP_ON.
> +   *
> +   * @param dataTransfer should be a DataTransfer.

Please mention bug 1444991 here so it's easier to fix when that is fixed.

@@ +80,2 @@
>     */
> +  boolean canDrop(in long index, in long orientation, in nsISupports dataTransfer);

None of the implementations you modify even look at the dataTransfer argument.

Can we just remove it?

@@ +83,5 @@
>    /**
>     * Called when the user drops something on this view. The |orientation| param
>     * specifies before/on/after the given |row|.
> +   *
> +   * @param dataTransfer should be a DataTransfer.

Please mention bug 1444991 here.

@@ +88,2 @@
>     */
> +  void drop(in long row, in long orientation, in nsISupports dataTransfer);

Same question with this - can we just remove the dataTransfer argument?
Attachment #8958247 - Flags: review?(nika) → review-
Comment on attachment 8958248 [details] [diff] [review]
part 10.  Remove nsIDOMDragEvent::GetDataTransfer

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

::: dom/interfaces/events/nsIDOMDragEvent.idl
@@ +5,5 @@
>  
>  #include "domstubs.idl"
>  #include "nsIDOMMouseEvent.idl"
>  
> +// XXXbz we should really remove this interface.

please file a bug & mention it here.

::: editor/libeditor/EditorEventListener.h
@@ +36,5 @@
>  class EditorBase;
>  
> +namespace dom {
> +class DragEvent;
> +}

nit: add a // namespace dom next to the closing }

::: editor/libeditor/TextEditorDataTransfer.cpp
@@ -287,4 @@
>                             newSelectionOffset, deleteSelection);
>    }
>  
> -  if (NS_SUCCEEDED(rv)) {

This is an odd check... I guess it used to check if range->IsPointInRange failed?
Attachment #8958248 - Flags: review?(nika) → review+
Comment on attachment 8958249 [details] [diff] [review]
part 11.  Remove nsIDOMDataTransfer from dragsession

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

::: widget/nsIDragService.idl
@@ +86,5 @@
>     *
>     * The aDragEvent must be supplied as the current screen coordinates of the
>     * event are needed to calculate the image location.
>     */
> +  [noscript]

This [noscript] isn't necessary IIRC because of the native parameter, but doesn't hurt anything.

@@ +115,3 @@
>  
>    /**
>      * Returns the current Drag Session  

wanna clean up this trailing whitespace while you're here?

::: widget/nsIDragSession.idl
@@ +77,5 @@
>    /**
>     * The data transfer object for the current drag.
>     */
>    [binaryname(DataTransferXPCOM)]
> +  attribute nsISupports dataTransfer;

Can you mention bug 1444991 here?

@@ +83,5 @@
>    [notxpcom, nostdcall] void setDataTransfer(in DataTransferPtr aDataTransfer);
>  
>    /**
>      * Get data from a Drag&Drop. Can be called while the drag is in process
>      * or after the drop has completed.  

more trailing whitespace that would be awesome to fix up ^_^
Attachment #8958249 - Flags: review?(nika) → review+
Comment on attachment 8958250 [details] [diff] [review]
part 12.  Remove nsIDOMDataTransfer from nsIDroppedLinkHandler

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

::: dom/base/nsIDroppedLinkHandler.idl
@@ +94,5 @@
>     * dragged. Since drag/drop performs a roundtrip of parent, child, parent,
>     * it allows the parent to verify that the child did not modify links
>     * being dropped.
> +   *
> +   * @param dataTransfer is a DataTransfer

bug 1444991 ^_^
Attachment #8958250 - Flags: review?(nika) → review+
Attachment #8958251 - Flags: review?(nika) → review+
Comment on attachment 8958252 [details] [diff] [review]
part 14.  Remove nsIDOMDataTransfer

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

::: dom/events/DataTransfer.cpp
@@ +65,5 @@
>  
>  NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(DataTransfer)
>    NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
>    NS_INTERFACE_MAP_ENTRY(mozilla::dom::DataTransfer)
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)

Unambiguous nsISupports base?!?! \o/
Attachment #8958252 - Flags: review?(nika) → review+
> Does this have to be a COMPtr? Can we make it a RefPtr?

We can. Done.

> you probably knew this but windows reorders the vtable when there are overloaded methods

Yep, I knew that.  It's bitten me once or twice.  ;)

> refptr?

Done.

> It'd be nice to change this into an enum class defined on DataTransfer or another related type at some point.

Filed bug 1445410.

> Can we perhaps assert that this is in range here?
> I should note that for ^ IMO we should MOZ_RELEASE_ASSERT it because it's important for correctness.

Done, with release assert.

> nit: trailing whitespace

Fixed.

> Please mention bug 1444991 here so it's easier to fix when that is fixed.

Done.

> None of the implementations you modify even look at the dataTransfer argument.

Right, but there are JS implementations that do.  They just don't need changes here.

> Please mention bug 1444991 here.

Done.

> Same question with this - can we just remove the dataTransfer argument?

No, because of JS impls.

> please file a bug & mention it here.

Bug 1445417 filed, comments updated.

> nit: add a // namespace dom next to the closing }

Good catch, fixed.

> This is an odd check... I guess it used to check if range->IsPointInRange failed?

The _last_ IsPointInRange.  The previous calls in the loop just got failure ignored....

I checked when that check got added.  It was initially checking that the InsertTextAt call inside the loop inserting text succeeded.... but only the last one.  Then the loop got changed to call the void-returning InsertFromDataTransfer, and this check became _really_ useless.
Blocks: 1445410, 1445417
> wanna clean up this trailing whitespace while you're here?

Done.

> Can you mention bug 1444991 here?

Done.

> more trailing whitespace that would be awesome to fix up ^_^

Done.

> bug 1444991 ^_^

Done.

Comment 26

Last year
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e145199ebea
part 1.  Get rid of nsIDOMDataTransfer::Get/SetDropEffect.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e93ff8b7481
part 2.  Get rid of nsIDOMDataTransfer::GetMozItemCount.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ab74cd16594
part 3.  Get rid of nsIDOMDataTransfer::Get/SetMozCursor.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbe8af159b77
part 4.  Get rid of nsIDOMDataTransfer::Get/SetDropEffectInt.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/7677c75ce1c8
part 5.  Get rid of nsIDOMDataTransfer::Get/SetEffectAllowedInt.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/c10b9eb41226
part 6.  Get rid of nsIDOMDataTransfer::GetFiles.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/60e3b9bbac1e
part 7.  Get rid of unused nsIDOMDataTransfer members.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/41d99ad7144f
part 8.  Remove use of nsIDOMDataTransfer from layout/forms.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1cd8692d296
part 9.  Remove use of nsIDOMDataTransfer from nsITreeView.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ec59d2fc44a
part 10.  Remove nsIDOMDragEvent::GetDataTransfer.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/c07b1df45a01
part 11.  Remove nsIDOMDataTransfer from dragsession.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d8732d627c9
part 12.  Remove nsIDOMDataTransfer from nsIDroppedLinkHandler.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/6da907cf2ab0
part 13.  Remove remaining nsIDOMDataTransfer uses.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/608e27955682
part 14.  Remove nsIDOMDataTransfer.  r=mystor

Comment 27

Last year
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee56d519a4b0
followup.  Fix a typo that breaks compilation on Windows.
Depends on: 1471157
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.