Closed
Bug 1446711
Opened 6 years ago
Closed 6 years ago
Get rid of nsIDOMMouseEvent
Categories
(Core :: DOM: Events, enhancement, P2)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(8 files)
22.12 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
28.07 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
4.82 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
4.97 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
15.75 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
6.87 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
104.23 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
31.21 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•6 years ago
|
Priority: -- → P2
Assignee | ||
Comment 1•6 years ago
|
||
MozReview-Commit-ID: 9Y61WHTDVvF
Attachment #8960314 -
Flags: review?(kyle)
Assignee | ||
Comment 2•6 years ago
|
||
MozReview-Commit-ID: DqMhK4wajH5
Attachment #8960315 -
Flags: review?(kyle)
Assignee | ||
Comment 3•6 years ago
|
||
MozReview-Commit-ID: 4cwIPNPzolI
Attachment #8960316 -
Flags: review?(kyle)
Assignee | ||
Comment 4•6 years ago
|
||
MozReview-Commit-ID: 1H2FzUHf55n
Attachment #8960317 -
Flags: review?(kyle)
Assignee | ||
Comment 5•6 years ago
|
||
MozReview-Commit-ID: AZWzObh01uI
Attachment #8960318 -
Flags: review?(kyle)
Assignee | ||
Comment 6•6 years ago
|
||
MozReview-Commit-ID: BmO0Ik3B8bO
Attachment #8960319 -
Flags: review?(kyle)
Assignee | ||
Comment 7•6 years ago
|
||
We can't include MouseEventBinding.h in MouseEvents.h because that produces this include loop: MouseEventBinding.h -> UIEventBinding.h -> nsGlobalWindow.h -> nsGlobalWindowInner.h -> nsRefreshDriver.h -> AnimationEventDispatcher.h -> AnimationComparator.h -> Animation.h -> EffectCompositor.h -> PseudoElementHashEntry.h -> Element.h -> PointerEventHandler.h -> MouseEvents.h -> MouseEventBinding.h MozReview-Commit-ID: 6FNksGil7uD
Attachment #8960320 -
Flags: review?(kyle)
Assignee | ||
Comment 8•6 years ago
|
||
MozReview-Commit-ID: 2FK1MA4LGZj
Attachment #8960321 -
Flags: review?(kyle)
Updated•6 years ago
|
Attachment #8960314 -
Flags: review?(kyle) → review+
Comment 9•6 years ago
|
||
Comment on attachment 8960315 [details] [diff] [review] part 2. Get rid of nsIDOMMouseEvent::GetClientX/Y Review of attachment 8960315 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/ImageDocument.cpp @@ +640,2 @@ > if (event) { > + x = event->ClientX(); nit: Might be worth simplifying the math in this block (i.e. x = event->ClientX() - img->OffsetLeft()), since it's no longer an XPCOM style getter? Up to you though.
Attachment #8960315 -
Flags: review?(kyle) → review+
Comment 10•6 years ago
|
||
Comment on attachment 8960316 [details] [diff] [review] part 3. Get rid of nsIDOMMouseEvent::GetMozInputSource Review of attachment 8960316 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/xul/nsXULPopupListener.cpp @@ +190,5 @@ > } > > if (mIsContext) { > #ifndef NS_CONTEXT_MENU_IS_MOUSEUP > + uint16_t inputSource = mouseEvent->MozInputSource(); nit: Do we even need the assignment anymore, or can we just use mouseEvent->MozInputSource() in the boolean check below?
Attachment #8960316 -
Flags: review?(kyle) → review+
Updated•6 years ago
|
Attachment #8960317 -
Flags: review?(kyle) → review+
Comment 11•6 years ago
|
||
Comment on attachment 8960318 [details] [diff] [review] part 5. Get rid of nsIDOMMouseEvent::GetButton Review of attachment 8960318 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/xbl/nsXBLPrototypeHandler.cpp @@ +732,5 @@ > { > if (mDetail == -1 && mMisc == 0 && (mKeyMask & cAllModifiers) == 0) > return true; // No filters set up. It's generic. > > + int16_t button = aMouseEvent->Button(); nit: temporary only used once, can move to check statement below @@ +740,2 @@ > > + int32_t clickcount = aMouseEvent->Detail(); nit: temporary only used once, can move to check statement below ::: extensions/spellcheck/src/mozInlineSpellChecker.cpp @@ +1858,5 @@ > NS_ENSURE_TRUE(mouseEvent, NS_OK); > > // ignore any errors from HandleNavigationEvent as we don't want to prevent > // anyone else from seeing this event. > + int16_t button = mouseEvent->Button(); nit: temporary only used once, can move to check statement below ::: layout/xul/nsSplitterFrame.cpp @@ +632,2 @@ > > + int16_t button = mouseEvent->Button(); nit: button isn't used after the if statement, so could probably just move getter to check below.
Attachment #8960318 -
Flags: review?(kyle) → review+
Updated•6 years ago
|
Attachment #8960319 -
Flags: review?(kyle) → review+
Updated•6 years ago
|
Attachment #8960320 -
Flags: review?(kyle) → review+
Updated•6 years ago
|
Attachment #8960321 -
Flags: review?(kyle) → review+
Assignee | ||
Comment 12•6 years ago
|
||
> nit: Might be worth simplifying the math in this block Good catch, fixed. > nit: Do we even need the assignment anymore, or can we just use mouseEvent->MozInputSource() in the boolean check below? We can do the latter, but we end up with really long lines. And wrapping it is less readable than having the temporary. >nit: temporary only used once, can move to check statement below >nit: temporary only used once, can move to check statement below >nit: temporary only used once, can move to check statement below >nit: button isn't used after the if statement, so could probably just move getter to check below. All fixed.
Comment 13•6 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b65f0a0b2e8e part 1. Get rid of nsIDOMMouseEvent::GetScreenX/Y. r=qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/4672662f16c2 part 2. Get rid of nsIDOMMouseEvent::GetClientX/Y. r=qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/d88f75968a29 part 3. Get rid of nsIDOMMouseEvent::GetMozInputSource. r=qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/3833386688c1 part 4. Get rid of nsIDOMMouseEvent::GetCtrl/Shift/Alt/MetaKey. r=qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/8f49c8b5c2d4 part 5. Get rid of nsIDOMMouseEvent::GetButton. r=qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/1e34d0184ad6 part 6. Get rid of various unused nsIDOMMouseEvent bits. r=qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/3073cf6d1124 part 7. Switch the nsIDOMMouseEvent::MOZ_SOURCE_* constants over to MouseEventBinding. r=qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/7a1e55aa1a6b part 8. Get rid of nsIDOMMouseEvent. r=qdot
Comment 14•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b65f0a0b2e8e https://hg.mozilla.org/mozilla-central/rev/4672662f16c2 https://hg.mozilla.org/mozilla-central/rev/d88f75968a29 https://hg.mozilla.org/mozilla-central/rev/3833386688c1 https://hg.mozilla.org/mozilla-central/rev/8f49c8b5c2d4 https://hg.mozilla.org/mozilla-central/rev/1e34d0184ad6 https://hg.mozilla.org/mozilla-central/rev/3073cf6d1124 https://hg.mozilla.org/mozilla-central/rev/7a1e55aa1a6b
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1447232
You need to log in
before you can comment on or make changes to this bug.
Description
•