Closed Bug 1455052 Opened 6 years ago Closed 6 years ago

Remove nsIDOMEvent

Categories

(Core :: DOM: Events, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(13 files)

17.76 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
6.17 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
2.01 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
28.27 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
7.87 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
7.24 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
15.55 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
44.82 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
3.90 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
24.72 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
5.32 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
24.11 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
7.70 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
This will take some work, bu with bug 1444991 fixed it seems feasible.
Depends on: 1455055
Blocks: 1132934
MozReview-Commit-ID: 4vZgmBnTWKH
Attachment #8969532 - Flags: review?(masayuki)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
MozReview-Commit-ID: By3duB2tPpl
Attachment #8969533 - Flags: review?(masayuki)
MozReview-Commit-ID: ArEfBOa7Pha
Attachment #8969534 - Flags: review?(masayuki)
MozReview-Commit-ID: GIs8DVfduKe
Attachment #8969535 - Flags: review?(masayuki)
MozReview-Commit-ID: 4mr6B4mYCdG
Attachment #8969536 - Flags: review?(masayuki)
MozReview-Commit-ID: JoP6kMuYQLQ
Attachment #8969537 - Flags: review?(masayuki)
MozReview-Commit-ID: 2OfAXBR8G5M
Attachment #8969538 - Flags: review?(masayuki)
MozReview-Commit-ID: ASkuyN3xSwB
Attachment #8969539 - Flags: review?(masayuki)
I couldn't figure out a way to do this without giving Event an IID....

MozReview-Commit-ID: LdXmsQSHeIc
Attachment #8969540 - Flags: review?(masayuki)
MozReview-Commit-ID: Fzckal7RGUv
Attachment #8969541 - Flags: review?(masayuki)
MozReview-Commit-ID: 2ytv7CjkPGz
Attachment #8969542 - Flags: review?(masayuki)
MozReview-Commit-ID: 5g0H3rzxTXt
Attachment #8969543 - Flags: review?(masayuki)
MozReview-Commit-ID: 3gmGy4URet3
Attachment #8969544 - Flags: review?(masayuki)
Attachment #8969532 - Flags: review?(masayuki) → review+
Attachment #8969533 - Flags: review?(masayuki) → review+
Attachment #8969534 - Flags: review?(masayuki) → review+
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.
Attachment #8969535 - Flags: review?(masayuki) → review+
Attachment #8969536 - Flags: review?(masayuki) → review+
Attachment #8969537 - Flags: review?(masayuki) → review+
Attachment #8969538 - Flags: review?(masayuki) → review+
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?
Attachment #8969539 - Flags: review?(masayuki) → review+
Attachment #8969540 - Flags: review?(masayuki) → review+
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?
Attachment #8969541 - Flags: review?(masayuki) → review+
(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.
Attachment #8969542 - Flags: review?(masayuki) → review+
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.
Attachment #8969543 - Flags: review?(masayuki) → review+
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 ":"?
Attachment #8969544 - Flags: review?(masayuki) → review+
I wonder, it might be better to notify other developers of removing nsIDOMEvent with "Intent to unship" mail.
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.)
> 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.
Pushed by bzbarsky@mozilla.com:
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
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cc5665abf82
followup.  Re-remove the Event ToSupports/ToCanonicalSupports methods.  r=bzbarsky
You need to log in before you can comment on or make changes to this bug.