Closed
Bug 1444686
Opened 7 years ago
Closed 7 years ago
Get rid of nsIDOMDataTransfer
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 2 open bugs)
Details
Attachments
(14 files)
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•7 years ago
|
||
MozReview-Commit-ID: 6Kn9uuaQYI0
Attachment #8958239 -
Flags: review?(nika)
Assignee | ||
Comment 2•7 years ago
|
||
MozReview-Commit-ID: 1eo6czER8Qw
Attachment #8958240 -
Flags: review?(nika)
Assignee | ||
Comment 3•7 years ago
|
||
MozReview-Commit-ID: G7vuh1uuWGv
Attachment #8958241 -
Flags: review?(nika)
Assignee | ||
Comment 4•7 years ago
|
||
MozReview-Commit-ID: EQ8KXMf4mnR
Attachment #8958242 -
Flags: review?(nika)
Assignee | ||
Comment 5•7 years ago
|
||
MozReview-Commit-ID: GIzIU7nWP5j
Attachment #8958243 -
Flags: review?(nika)
Assignee | ||
Comment 6•7 years ago
|
||
MozReview-Commit-ID: 3gAWLI2DyyU
Attachment #8958244 -
Flags: review?(nika)
Assignee | ||
Comment 7•7 years ago
|
||
MozReview-Commit-ID: 53ShdRZHlC9
Attachment #8958245 -
Flags: review?(nika)
Assignee | ||
Comment 8•7 years ago
|
||
MozReview-Commit-ID: 2MR9MVbdOPP
Attachment #8958246 -
Flags: review?(nika)
Assignee | ||
Comment 9•7 years ago
|
||
MozReview-Commit-ID: Dpn7YSZpDsc
Attachment #8958247 -
Flags: review?(nika)
Assignee | ||
Comment 10•7 years ago
|
||
MozReview-Commit-ID: LwKqWBGXVcN
Attachment #8958248 -
Flags: review?(nika)
Assignee | ||
Comment 11•7 years ago
|
||
MozReview-Commit-ID: CRmiSTSN8af
Attachment #8958249 -
Flags: review?(nika)
Assignee | ||
Comment 12•7 years ago
|
||
MozReview-Commit-ID: 7hseR1dpfWG
Attachment #8958250 -
Flags: review?(nika)
Assignee | ||
Comment 13•7 years ago
|
||
MozReview-Commit-ID: EFauqLMGz5S
Attachment #8958251 -
Flags: review?(nika)
Assignee | ||
Comment 14•7 years ago
|
||
MozReview-Commit-ID: BLi4w10clkP
Attachment #8958252 -
Flags: review?(nika)
Updated•7 years ago
|
Attachment #8958239 -
Flags: review?(nika) → review+
Updated•7 years ago
|
Attachment #8958240 -
Flags: review?(nika) → review+
Comment 15•7 years ago
|
||
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 16•7 years ago
|
||
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+
Comment 17•7 years ago
|
||
I should note that for ^ IMO we should MOZ_RELEASE_ASSERT it because it's important for correctness.
Updated•7 years ago
|
Attachment #8958243 -
Flags: review?(nika) → review+
Comment 18•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8958245 -
Flags: review?(nika) → review+
Updated•7 years ago
|
Attachment #8958246 -
Flags: review?(nika) → review+
Comment 19•7 years ago
|
||
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 20•7 years ago
|
||
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 21•7 years ago
|
||
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 22•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8958251 -
Flags: review?(nika) → review+
Comment 23•7 years ago
|
||
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+
Assignee | ||
Comment 24•7 years ago
|
||
> 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.
Assignee | ||
Comment 25•7 years ago
|
||
> 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•7 years ago
|
||
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•7 years ago
|
||
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee56d519a4b0
followup. Fix a typo that breaks compilation on Windows.
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9e145199ebea
https://hg.mozilla.org/mozilla-central/rev/9e93ff8b7481
https://hg.mozilla.org/mozilla-central/rev/5ab74cd16594
https://hg.mozilla.org/mozilla-central/rev/dbe8af159b77
https://hg.mozilla.org/mozilla-central/rev/7677c75ce1c8
https://hg.mozilla.org/mozilla-central/rev/c10b9eb41226
https://hg.mozilla.org/mozilla-central/rev/60e3b9bbac1e
https://hg.mozilla.org/mozilla-central/rev/41d99ad7144f
https://hg.mozilla.org/mozilla-central/rev/e1cd8692d296
https://hg.mozilla.org/mozilla-central/rev/0ec59d2fc44a
https://hg.mozilla.org/mozilla-central/rev/c07b1df45a01
https://hg.mozilla.org/mozilla-central/rev/3d8732d627c9
https://hg.mozilla.org/mozilla-central/rev/6da907cf2ab0
https://hg.mozilla.org/mozilla-central/rev/608e27955682
https://hg.mozilla.org/mozilla-central/rev/ee56d519a4b0
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•