Closed
Bug 1455052
Opened 6 years ago
Closed 6 years ago
Remove nsIDOMEvent
Categories
(Core :: DOM: Events, enhancement)
Core
DOM: Events
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.
Assignee | ||
Comment 1•6 years ago
|
||
MozReview-Commit-ID: 4vZgmBnTWKH
Attachment #8969532 -
Flags: review?(masayuki)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•6 years ago
|
||
MozReview-Commit-ID: By3duB2tPpl
Attachment #8969533 -
Flags: review?(masayuki)
Assignee | ||
Comment 3•6 years ago
|
||
MozReview-Commit-ID: ArEfBOa7Pha
Attachment #8969534 -
Flags: review?(masayuki)
Assignee | ||
Comment 4•6 years ago
|
||
MozReview-Commit-ID: GIs8DVfduKe
Attachment #8969535 -
Flags: review?(masayuki)
Assignee | ||
Comment 5•6 years ago
|
||
MozReview-Commit-ID: 4mr6B4mYCdG
Attachment #8969536 -
Flags: review?(masayuki)
Assignee | ||
Comment 6•6 years ago
|
||
MozReview-Commit-ID: JoP6kMuYQLQ
Attachment #8969537 -
Flags: review?(masayuki)
Assignee | ||
Comment 7•6 years ago
|
||
MozReview-Commit-ID: 2OfAXBR8G5M
Attachment #8969538 -
Flags: review?(masayuki)
Assignee | ||
Comment 8•6 years ago
|
||
MozReview-Commit-ID: ASkuyN3xSwB
Attachment #8969539 -
Flags: review?(masayuki)
Assignee | ||
Comment 9•6 years ago
|
||
I couldn't figure out a way to do this without giving Event an IID.... MozReview-Commit-ID: LdXmsQSHeIc
Attachment #8969540 -
Flags: review?(masayuki)
Assignee | ||
Comment 10•6 years ago
|
||
MozReview-Commit-ID: Fzckal7RGUv
Attachment #8969541 -
Flags: review?(masayuki)
Assignee | ||
Comment 11•6 years ago
|
||
MozReview-Commit-ID: 2ytv7CjkPGz
Attachment #8969542 -
Flags: review?(masayuki)
Assignee | ||
Comment 12•6 years ago
|
||
MozReview-Commit-ID: 5g0H3rzxTXt
Attachment #8969543 -
Flags: review?(masayuki)
Assignee | ||
Comment 13•6 years ago
|
||
MozReview-Commit-ID: 3gmGy4URet3
Attachment #8969544 -
Flags: review?(masayuki)
Updated•6 years ago
|
Attachment #8969532 -
Flags: review?(masayuki) → review+
Updated•6 years ago
|
Attachment #8969533 -
Flags: review?(masayuki) → review+
Updated•6 years ago
|
Attachment #8969534 -
Flags: review?(masayuki) → review+
Comment 14•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8969536 -
Flags: review?(masayuki) → review+
Updated•6 years ago
|
Attachment #8969537 -
Flags: review?(masayuki) → review+
Updated•6 years ago
|
Attachment #8969538 -
Flags: review?(masayuki) → review+
Comment 15•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8969540 -
Flags: review?(masayuki) → review+
Comment 16•6 years ago
|
||
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+
Comment 17•6 years ago
|
||
(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.
Updated•6 years ago
|
Attachment #8969542 -
Flags: review?(masayuki) → review+
Comment 18•6 years ago
|
||
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 19•6 years ago
|
||
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+
Comment 20•6 years ago
|
||
I wonder, it might be better to notify other developers of removing nsIDOMEvent with "Intent to unship" mail.
Comment 21•6 years ago
|
||
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.)
Assignee | ||
Comment 22•6 years ago
|
||
> 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•6 years ago
|
||
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
Comment 24•6 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5cc5665abf82 followup. Re-remove the Event ToSupports/ToCanonicalSupports methods. r=bzbarsky
Comment 25•6 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•