1455052 - Remove nsIDOMEvent

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement)

Description

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

This will take some work, bu with bug 1444991 fixed it seems feasible.

Comment 1

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

Attachment: part 1. Switch event dispatch to working with Event, not nsIDOMEvent

MozReview-Commit-ID: 4vZgmBnTWKH

Comment 2

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

Attachment: part 2. Switch async event dispatcher to working with Event, not nsIDOMEvent

MozReview-Commit-ID: By3duB2tPpl

Comment 3

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

Attachment: part 3. Change GetEventPopupControlState to work with Event

MozReview-Commit-ID: ArEfBOa7Pha

Comment 4

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

Attachment: part 4. Stop using nsIDOMEvent in xpidl interfaces

MozReview-Commit-ID: GIs8DVfduKe

Comment 5

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

Attachment: part 5. Remove nsIDOMEvent usage from indexeddb code

MozReview-Commit-ID: 4mr6B4mYCdG

Comment 6

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

Attachment: part 6. Stop using nsIDOMEvent in editor code

MozReview-Commit-ID: JoP6kMuYQLQ

Comment 7

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

Attachment: part 7. Remove nsIDOMEvent use from layout

MozReview-Commit-ID: 2OfAXBR8G5M

Comment 8

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

Attachment: part 8. Stop using nsIDOMEvent in docshell and dom

MozReview-Commit-ID: ASkuyN3xSwB

Comment 9

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

Attachment: part 9. Stop using nsIDOMEvent in DataTransferItem

I couldn't figure out a way to do this without giving Event an IID....

MozReview-Commit-ID: LdXmsQSHeIc

Comment 10

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

Attachment: part 10. Remove various unused nsIDOMEvent bits

MozReview-Commit-ID: Fzckal7RGUv

Comment 11

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

Attachment: part 11. Remove nsIDOMEvent::InternalDOMEvent

MozReview-Commit-ID: 2ytv7CjkPGz

Comment 12

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

Attachment: part 12. Remove JS uses of nsIDOMEvent

MozReview-Commit-ID: 5g0H3rzxTXt

Comment 13

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

Attachment: part 13. Remove nsIDOMEvent

MozReview-Commit-ID: 3gmGy4URet3

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

Comment on attachment 8969535 [details] [diff] [review]
part 4.  Stop using nsIDOMEvent in xpidl interfaces

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

::: dom/base/TextInputProcessor.cpp
@@ +18,5 @@
>  #include "nsPIDOMWindow.h"
>  #include "nsPresContext.h"
>  
>  using mozilla::dom::KeyboardEvent;
> +using mozilla::dom::Event;

nit: Put this above KeyboardEvent for making the list ordered from A to Z.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

Comment on attachment 8969539 [details] [diff] [review]
part 8.  Stop using nsIDOMEvent in docshell and dom

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

::: dom/events/Event.h
@@ +398,5 @@
>  
>  inline nsISupports*
>  ToSupports(mozilla::dom::Event* e)
>  {
> +  return e;

Hmm, ToSupports() should be removed completely by one of the following patch for follow up bug.

@@ +404,5 @@
>  
>  inline nsISupports*
>  ToCanonicalSupports(mozilla::dom::Event* e)
>  {
> +  return e;

ToCanonicalSupports() too.

::: dom/xul/nsXULElement.cpp
@@ +1305,1 @@
>              NS_ENSURE_STATE(!SameCOMIdentity(event->GetOriginalTarget(),

Cannot compare Event::GetOriginalTarget() and commandElt directly now?

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

Comment on attachment 8969541 [details] [diff] [review]
part 10.  Remove various unused nsIDOMEvent bits

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

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +520,5 @@
>    if (!aVisitor.mDOMEvent->IsTrusted()) {
>      return NS_OK;
>    }
>  
>    nsString type;

nit: Cannot make this nsAutoString?

::: dom/indexedDB/ProfilerHelpers.h
@@ +264,5 @@
>      : nsAutoCString(kQuote)
>    {
>      MOZ_ASSERT(aDefault);
>  
>      nsString eventType;

nit: Cannot make this nsAutoString?

::: dom/xhr/XMLHttpRequestWorker.cpp
@@ +986,5 @@
>      NS_ERROR("Shouldn't get here!");
>      return NS_OK;
>    }
>  
>    nsString type;

nit: Cannot make this nsAutoString?

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #15)
> Comment on attachment 8969539 [details] [diff] [review]
> part 8.  Stop using nsIDOMEvent in docshell and dom
> 
> Review of attachment 8969539 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/events/Event.h
> @@ +398,5 @@
> >  
> >  inline nsISupports*
> >  ToSupports(mozilla::dom::Event* e)
> >  {
> > +  return e;
> 
> Hmm, ToSupports() should be removed completely by one of the following patch
> for follow up bug.
> 
> @@ +404,5 @@
> >  
> >  inline nsISupports*
> >  ToCanonicalSupports(mozilla::dom::Event* e)
> >  {
> > +  return e;
> 
> ToCanonicalSupports() too.

Ah, but if those methods are useful if there are some overloads, ignore these comments.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

Comment on attachment 8969543 [details] [diff] [review]
part 12.  Remove JS uses of nsIDOMEvent

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

Although, I'm not familiar with sandbox and xpconnect. But the changes must be okay since you must be familiar with them.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

Comment on attachment 8969544 [details] [diff] [review]
part 13.  Remove nsIDOMEvent

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

::: dom/events/Event.h
@@ +54,5 @@
>  #define NS_EVENT_IID                                            \
>    { 0x71139716, 0x4d91, 0x4dee,                                 \
>        { 0xba, 0xf9, 0xe3, 0x3b, 0x80, 0xc1, 0x61, 0x61 } }
>  
> +class Event : public nsISupports,

Ah, could you please move "," to below ":"?

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

I wonder, it might be better to notify other developers of removing nsIDOMEvent with "Intent to unship" mail.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

bz: When you replaces nsIDOMElement with dom::Element, could you let me know? Currently, editor has overloads of such for avoiding  using virtual method calls as far as possible. I don't want to merge them and don't want to increase virtual calls. (Although, I'm renaming a lot of non-virtual methods of editor in bug 1451672.)

Comment 22

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

> nit: Put this above KeyboardEvent for making the list ordered from A to Z.

Fixed.

> Hmm, ToSupports() should be removed completely by one of the following patch for follow up bug.

Actually, looks like bug 1452981 removed it and ToCanonicalSupports from Even this morning.  ;)

> Cannot compare Event::GetOriginalTarget() and commandElt directly now?

You're right, we can.  Fixed, like so:

            NS_ENSURE_STATE(event->GetOriginalTarget() != commandElt);

> nit: Cannot make this nsAutoString?

Can, and done, for all three callsites.

> Although, I'm not familiar with sandbox and xpconnect.

Not a problem.  I asked kmag to review this part and he said r+ on IRC.

> Ah, could you please move "," to below ":"?

Done.

> I wonder, it might be better to notify other developers of removing nsIDOMEvent with "Intent to unship"

I can do that.

> bz: When you replaces nsIDOMElement with dom::Element, could you let me know?

Yes.  Bug 1455674 filed to track that.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/070eb9701963
part 1.  Switch event dispatch to working with Event, not nsIDOMEvent.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/00dfc503972c
part 2.  Switch async event dispatcher to working with Event, not nsIDOMEvent.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2bdd520f53
part 3.  Change GetEventPopupControlState to work with Event.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/59a88dd96729
part 4.  Stop using nsIDOMEvent in xpidl interfaces.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e5bf7b5264b
part 5.  Remove nsIDOMEvent usage from indexeddb code.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/45ca32f89afe
part 6.  Stop using nsIDOMEvent in editor code.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1ec3420bc84
part 7.  Remove nsIDOMEvent use from layout.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/b35aad989a13
part 8.  Stop using nsIDOMEvent in docshell and dom.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/43abd16157ca
part 9.  Stop using nsIDOMEvent in DataTransferItem.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d824df868b7
part 10.  Remove various unused nsIDOMEvent bits.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c8bb7775296
part 11.  Remove nsIDOMEvent::InternalDOMEvent.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/b52ecaafe0df
part 12.  Remove JS uses of nsIDOMEvent.  r=masayuki,kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/27df0b6428b6
part 13.  Remove nsIDOMEvent.  r=masayuki

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cc5665abf82
followup.  Re-remove the Event ToSupports/ToCanonicalSupports methods.  r=bzbarsky

Comment 25

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/070eb9701963
https://hg.mozilla.org/mozilla-central/rev/00dfc503972c
https://hg.mozilla.org/mozilla-central/rev/7f2bdd520f53
https://hg.mozilla.org/mozilla-central/rev/59a88dd96729
https://hg.mozilla.org/mozilla-central/rev/6e5bf7b5264b
https://hg.mozilla.org/mozilla-central/rev/45ca32f89afe
https://hg.mozilla.org/mozilla-central/rev/f1ec3420bc84
https://hg.mozilla.org/mozilla-central/rev/b35aad989a13
https://hg.mozilla.org/mozilla-central/rev/43abd16157ca
https://hg.mozilla.org/mozilla-central/rev/7d824df868b7
https://hg.mozilla.org/mozilla-central/rev/9c8bb7775296
https://hg.mozilla.org/mozilla-central/rev/b52ecaafe0df
https://hg.mozilla.org/mozilla-central/rev/27df0b6428b6
https://hg.mozilla.org/mozilla-central/rev/5cc5665abf82