MozReview-Commit-ID: 6Kn9uuaQYI0

MozReview-Commit-ID: 1eo6czER8Qw

MozReview-Commit-ID: G7vuh1uuWGv

MozReview-Commit-ID: EQ8KXMf4mnR

MozReview-Commit-ID: GIzIU7nWP5j

MozReview-Commit-ID: 3gAWLI2DyyU

MozReview-Commit-ID: 53ShdRZHlC9

MozReview-Commit-ID: 2MR9MVbdOPP

MozReview-Commit-ID: Dpn7YSZpDsc

MozReview-Commit-ID: LwKqWBGXVcN

MozReview-Commit-ID: CRmiSTSN8af

MozReview-Commit-ID: 7hseR1dpfWG

MozReview-Commit-ID: EFauqLMGz5S

MozReview-Commit-ID: BLi4w10clkP

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?

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.

I should note that for ^ IMO we should MOZ_RELEASE_ASSERT it because it's important for correctness.

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

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?

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?

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 ^_^

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 ^_^

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/

> 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.

> 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.

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

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ee56d519a4b0 followup. Fix a typo that breaks compilation on Windows.

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