Closed
Bug 1455052
Opened 7 years ago
Closed 7 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•7 years ago
|
||
MozReview-Commit-ID: 4vZgmBnTWKH
Attachment #8969532 -
Flags: review?(masayuki)
![]() |
Assignee | |
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 2•7 years ago
|
||
MozReview-Commit-ID: By3duB2tPpl
Attachment #8969533 -
Flags: review?(masayuki)
![]() |
Assignee | |
Comment 3•7 years ago
|
||
MozReview-Commit-ID: ArEfBOa7Pha
Attachment #8969534 -
Flags: review?(masayuki)
![]() |
Assignee | |
Comment 4•7 years ago
|
||
MozReview-Commit-ID: GIs8DVfduKe
Attachment #8969535 -
Flags: review?(masayuki)
![]() |
Assignee | |
Comment 5•7 years ago
|
||
MozReview-Commit-ID: 4mr6B4mYCdG
Attachment #8969536 -
Flags: review?(masayuki)
![]() |
Assignee | |
Comment 6•7 years ago
|
||
MozReview-Commit-ID: JoP6kMuYQLQ
Attachment #8969537 -
Flags: review?(masayuki)
![]() |
Assignee | |
Comment 7•7 years ago
|
||
MozReview-Commit-ID: 2OfAXBR8G5M
Attachment #8969538 -
Flags: review?(masayuki)
![]() |
Assignee | |
Comment 8•7 years ago
|
||
MozReview-Commit-ID: ASkuyN3xSwB
Attachment #8969539 -
Flags: review?(masayuki)
![]() |
Assignee | |
Comment 9•7 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•7 years ago
|
||
MozReview-Commit-ID: Fzckal7RGUv
Attachment #8969541 -
Flags: review?(masayuki)
![]() |
Assignee | |
Comment 11•7 years ago
|
||
MozReview-Commit-ID: 2ytv7CjkPGz
Attachment #8969542 -
Flags: review?(masayuki)
![]() |
Assignee | |
Comment 12•7 years ago
|
||
MozReview-Commit-ID: 5g0H3rzxTXt
Attachment #8969543 -
Flags: review?(masayuki)
![]() |
Assignee | |
Comment 13•7 years ago
|
||
MozReview-Commit-ID: 3gmGy4URet3
Attachment #8969544 -
Flags: review?(masayuki)
Updated•7 years ago
|
Attachment #8969532 -
Flags: review?(masayuki) → review+
Updated•7 years ago
|
Attachment #8969533 -
Flags: review?(masayuki) → review+
Updated•7 years ago
|
Attachment #8969534 -
Flags: review?(masayuki) → review+
Comment 14•7 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•7 years ago
|
Attachment #8969536 -
Flags: review?(masayuki) → review+
Updated•7 years ago
|
Attachment #8969537 -
Flags: review?(masayuki) → review+
Updated•7 years ago
|
Attachment #8969538 -
Flags: review?(masayuki) → review+
Comment 15•7 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•7 years ago
|
Attachment #8969540 -
Flags: review?(masayuki) → review+
Comment 16•7 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•7 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•7 years ago
|
Attachment #8969542 -
Flags: review?(masayuki) → review+
Comment 18•7 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•7 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•7 years ago
|
||
I wonder, it might be better to notify other developers of removing nsIDOMEvent with "Intent to unship" mail.
Comment 21•7 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•7 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•7 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•7 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•7 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: 7 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
•