Closed Bug 1455052 Opened 7 years ago Closed 7 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.

Attachment

General

Created:
Updated:
Size: