Remove some XPCOM event interfaces

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(16 attachments, 1 obsolete attachment)

8.32 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
9.47 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
7.44 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
2.37 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
26.01 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
32.64 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
14.13 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
8.96 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
5.59 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
641.30 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
87.14 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
32.44 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
64.41 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
8.37 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
36.44 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
21.42 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
No description provided.
Olli, are you OK with me removing "empty" nsISupports impls from events?

So for example, instead of having:

  NS_INTERFACE_MAP_BEGIN(AnimationEvent)
  NS_INTERFACE_MAP_END_INHERITING(Event)

  NS_IMPL_ADDREF_INHERITED(AnimationEvent, Event)
  NS_IMPL_RELEASE_INHERITED(AnimationEvent, Event)

with no interfaces on AnimationEvent that are not on Event, to just remove all that and the NS_DECL_ISUPPORTS_INHERITED?  Saves some function calls, at the cost of worse refcount logging...
Flags: needinfo?(bugs)
Masayuki, is it generally OK to use the KeyboardEvent.DOM_VK_* constants everywhere we currently do Ci.nsDOMKeyEvent.DOM_VK_* (assuming KeyboardEvent exists in those scopes, of course)?  If not, is there a better option?
Flags: needinfo?(masayuki)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #2)
> Masayuki, is it generally OK to use the KeyboardEvent.DOM_VK_* constants
> everywhere we currently do Ci.nsDOMKeyEvent.DOM_VK_* (assuming KeyboardEvent
> exists in those scopes, of course)?  If not, is there a better option?

Yeah, of course, I think so. We should get rid of nsIDOMKeyEvent.DOM_VK_* from everywhere.
Flags: needinfo?(masayuki)
And similar for C++; can I just replace nsIDOMKeyEvent::DOM_VK_* with KeyboardEventBinding::DOM_VK_*?  Or should I use KeyEventBinding::DOM_VK_*?
Difficult to say. I think that C++ code shouldn't use dom::*Event basically. If it's possible, each handler should use Widget*Event or Internal*Event with nsIDOMEvent::WidgetEventPtr()->As*Event().  Then, we can save virtual call cost for accessing each attribute and QI cost (bug 1374921, bug 1331879).  Additionally, each keyboard event handler shouldn't use keyCode anymore. They should use .key (bug 1136539).
Hmm.  OK, that sounds like a much more complicated transformation than what I have so far.  I'll put up what I have and see what you think.  It's no worse than what we have now, at least...
MozReview-Commit-ID: GRZzt1xGGDc
Attachment #8949608 - Flags: review?(masayuki)
MozReview-Commit-ID: 9svlTELngmi
Attachment #8949609 - Flags: review?(masayuki)
We never use initCommandEvent anywhere on a CommandEvent, only on
XULCommandEvents.

MozReview-Commit-ID: 8QHYnlPdDvx
Attachment #8949610 - Flags: review?(masayuki)
MozReview-Commit-ID: Hx9NYaslnCM
Attachment #8949611 - Flags: review?(masayuki)
MozReview-Commit-ID: COqUWh5xjfH
Attachment #8949612 - Flags: review?(masayuki)
MozReview-Commit-ID: Cp4krHgxXzQ
Attachment #8949613 - Flags: review?(masayuki)
The various event header changes are to avoid forcing random places to include nsGlobalWindowInner.h

MozReview-Commit-ID: 4THIjj6kIXv
Attachment #8949614 - Flags: review?(masayuki)
MozReview-Commit-ID: 57xpY8vNfW2
Attachment #8949615 - Flags: review?(masayuki)
MozReview-Commit-ID: JXCCrbaMcn
Attachment #8949616 - Flags: review?(masayuki)
MozReview-Commit-ID: GGciORX62Yh
Attachment #8949617 - Flags: review?(masayuki)
MozReview-Commit-ID: Honw0NrVMuV
Attachment #8949618 - Flags: review?(masayuki)
MozReview-Commit-ID: 8giqG5iHiIf
Attachment #8949619 - Flags: review?(masayuki)
MozReview-Commit-ID: Anl5QJZknJL
Attachment #8949620 - Flags: review?(masayuki)
MozReview-Commit-ID: Gf59kFSIuaK
Attachment #8949621 - Flags: review?(masayuki)
MozReview-Commit-ID: EWWqk9HAwqp
Attachment #8949622 - Flags: review?(masayuki)
MozReview-Commit-ID: 8JDovhthKtx
Attachment #8949623 - Flags: review?(masayuki)
In part 10, the long bit in widget/tests/test_keycodes.xul was just pure search-and-replace.  In fact, most of part 10 was pure search-and-replace, as was much of part 11...
Blocks: 1436902
> Olli, are you OK with me removing "empty" nsISupports impls from events?

I decided to have my cake and eat it too.  See bug 1436902.
Flags: needinfo?(bugs)
MozReview-Commit-ID: Gf59kFSIuaK
Attachment #8949628 - Flags: review?(masayuki)
Attachment #8949621 - Attachment is obsolete: true
Attachment #8949621 - Flags: review?(masayuki)
Comment on attachment 8949609 [details] [diff] [review]
part 2.  Remove nsIDOMBeforeUnloadEvent

Review of attachment 8949609 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/Event.h
@@ +113,5 @@
>  
>    virtual TimeEvent* AsTimeEvent() { return nullptr; }
>  
> +  // BeforeUnloadEvent is not autogenerated because it has a setter.
> +  virtual BeforeUnloadEvent* AsBeforeUnloadEvent()

I think that there should be |const| version too. However, after all, perhaps, we should implement this kind of methods with macro and a header file listing all event classes. So, not adding |const| version in this bug is fine.
Attachment #8949609 - Flags: review?(masayuki) → review+
Comment on attachment 8949612 [details] [diff] [review]
part 5.  Switch xbl from nsIDOMKeyEvent to KeyboardEvent

Review of attachment 8949612 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, this must improve real performance of key input.
Attachment #8949612 - Flags: review?(masayuki) → review+
Comment on attachment 8949613 [details] [diff] [review]
part 6.  Switch layout/xul from nsIDOMKeyEvent to KeyboardEvent

Review of attachment 8949613 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/xul/nsMenuBarListener.h
@@ +72,5 @@
>    nsresult Fullscreen(nsIDOMEvent* aEvent);
>  
>    static void InitAccessKey();
>  
> +  static mozilla::Modifiers GetModifiersForAccessKey(mozilla::dom::KeyboardEvent* event);

nit: too long line. please break this line before landing.

::: layout/xul/nsMenuPopupFrame.h
@@ +344,5 @@
>    // then the menu's action should be carried out, as if the user had pressed
>    // the Enter key. If doAction is false, the menu should just be highlighted.
>    // This method also handles incremental searching in menus so the user can
>    // type the first few letters of an item/s name to select it.
> +  nsMenuFrame* FindMenuWithShortcut(mozilla::dom::KeyboardEvent* aKeyEvent, bool& doAction);

nit: too long line.

::: layout/xul/nsXULPopupManager.cpp
@@ +2706,5 @@
> +                  aKeyEvent->AltKey());
> +      bool shift = (menuAccessKey != nsIDOMKeyEvent::DOM_VK_SHIFT &&
> +                    aKeyEvent->ShiftKey());
> +      bool meta = (menuAccessKey != nsIDOMKeyEvent::DOM_VK_META &&
> +                   aKeyEvent->MetaKey());

Still using nsIDOMKeyEvent::DOM_VK_* here, but I guess that this will be fixed in one of the following patches.
Attachment #8949613 - Flags: review?(masayuki) → review+
Comment on attachment 8949614 [details] [diff] [review]
part 7.  Remove nsIDOMKeyEvent usage from formfill and spellcheck

Review of attachment 8949614 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/KeyboardEvent.cpp
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "mozilla/dom/KeyboardEvent.h"
> +#include "mozilla/dom/KeyboardEventBinding.h"

If you have some special reason to reduce dependency of KeyboardEventBinding.h, please write it in the commit message.

::: dom/events/KeyboardEvent.h
@@ +44,5 @@
>                                             const KeyboardEventInit& aParam,
>                                             ErrorResult& aRv);
>  
> +  virtual JSObject* WrapObjectInternal(JSContext* aCx,
> +                                       JS::Handle<JSObject*> aGivenProto) override;

nit: Too long line. Perhaps, breaking after|virtual JSObject*| makes them shorter easily.

::: dom/events/UIEvent.h
@@ +49,5 @@
>                                                 const UIEventInit& aParam,
>                                                 ErrorResult& aRv);
>  
> +  virtual JSObject* WrapObjectInternal(JSContext* aCx,
> +                                       JS::Handle<JSObject*> aGivenProto) override;

nit: Too long line.
Attachment #8949614 - Flags: review?(masayuki) → review+
Comment on attachment 8949616 [details] [diff] [review]
part 9.  Remove nsIDOMKeyEvent::DOM_KEY* constants

Review of attachment 8949616 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/resistfingerprinting/nsRFPService.cpp
@@ +620,5 @@
>    WidgetKeyboardEvent::GetDOMCodeName(keyCodeInfo.mCode, aOut);
>  
>    // We need to change the 'Left' with 'Right' if the location indicates
>    // it's a right key.
> +  if (aKeyboardEvent->mLocation == dom::KeyboardEventBinding::DOM_KEY_LOCATION_RIGHT &&

nit: too long line.
Attachment #8949616 - Flags: review?(masayuki) → review+
Comment on attachment 8949617 [details] [diff] [review]
part 10.  Remove use of nsIDOMKeyEvent in JS

Review of attachment 8949617 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/downloads/content/downloads.js
@@ +415,5 @@
>      // If the user has pressed the tab, up, or down cursor key, start keyboard
>      // navigation, thus enabling focusrings in the panel.  Keyboard navigation
>      // is automatically disabled if the user moves the mouse on the panel, or
>      // if the panel is closed.
> +    if ((aEvent.keyCode == aEvent.DOM_VK_TAB ||

I guess that here may not have global object, win.

::: browser/extensions/formautofill/FormAutofillContent.jsm
@@ +619,5 @@
>    _onKeyDown(e) {
>      let lastAutoCompleteResult = ProfileAutocomplete.lastProfileAutoCompleteResult;
>      let focusedInput = FormAutofillContent.activeInput;
>  
> +    if (e.keyCode != e.DOM_VK_RETURN || !lastAutoCompleteResult ||

ditto...

::: devtools/server/actors/highlighters.js
@@ +306,5 @@
>         * ESC/CTRL+SHIFT+C: Cancels picker, picks currentNode
>         */
>        switch (event.keyCode) {
>          // Wider.
> +        case event.DOM_VK_LEFT:

ditto...

::: testing/marionette/event.js
@@ +361,5 @@
>      return 0;
>    }
>  
> +  let KeyboardEvent = getKeyboardEvent_(win);
> +    

nit: only whitespaces line.

::: toolkit/components/autocomplete/tests/unit/test_autofillSelectedPopupIndex.js
@@ +56,5 @@
>  
>    // Key down to select the second match in the popup.
> +  // Hardcode KeyboardEvent.DOM_VK_DOWN, because we can't easily
> +  // include KeyboardEvent here.
> +  controller.handleKeyNavigation(0x28 /* KeyboardEvent.DOM_VK_DOWN */);

Yeah, and perhaps, we should make it taking key name instead of keyCode value. (of course, not scope of this bug.)
Attachment #8949617 - Flags: review?(masayuki) → review+
Comment on attachment 8949618 [details] [diff] [review]
part 11.  Remove the use of the nsIDOMKeyEvent::DOM_VK* constants in C++

Review of attachment 8949618 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp
@@ +1876,5 @@
>    switch (keyCode)
>    {
> +    case KeyboardEventBinding::DOM_VK_RIGHT:
> +    case KeyboardEventBinding::DOM_VK_LEFT:
> +      HandleNavigationEvent(false, keyCode == KeyboardEventBinding::DOM_VK_RIGHT ? 1 : -1);

nit: Too long line.

::: layout/xul/nsMenuBarListener.cpp
@@ +154,5 @@
>  #endif
>  
>    // Get the menu access key value from prefs, overriding the default:
>    mAccessKey = Preferences::GetInt("ui.key.menuAccessKey", mAccessKey);
> +  if (mAccessKey == dom::KeyboardEventBinding::DOM_VK_SHIFT)

Looks like that this if/else if blocks can be rewritten with switch statement.

::: layout/xul/nsXULPopupManager.cpp
@@ +52,5 @@
> +static_assert(KeyboardEventBinding::DOM_VK_HOME  == KeyboardEventBinding::DOM_VK_END + 1 &&
> +              KeyboardEventBinding::DOM_VK_LEFT  == KeyboardEventBinding::DOM_VK_END + 2 &&
> +              KeyboardEventBinding::DOM_VK_UP    == KeyboardEventBinding::DOM_VK_END + 3 &&
> +              KeyboardEventBinding::DOM_VK_RIGHT == KeyboardEventBinding::DOM_VK_END + 4 &&
> +              KeyboardEventBinding::DOM_VK_DOWN  == KeyboardEventBinding::DOM_VK_END + 5,

nit: Too long lines. Perhaps, should break after "=="?

@@ +2374,2 @@
>        if (aTopVisibleMenuItem) {
> +        aTopVisibleMenuItem->Frame()->ChangeByPage(keyCode == KeyboardEventBinding::DOM_VK_PAGE_UP);

nit: Too long line.
Attachment #8949618 - Flags: review?(masayuki) → review+
Comment on attachment 8949619 [details] [diff] [review]
part 12.  Remove nsIDOMKeyEvent

Review of attachment 8949619 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/TextInputProcessor.cpp
@@ +606,5 @@
>  
> +  RefPtr<KeyboardEvent> keyEvent;
> +  if (aDOMKeyEvent) {
> +    keyEvent = aDOMKeyEvent->InternalDOMEvent()->AsKeyboardEvent();
> +    NS_ENSURE_TRUE(keyEvent, NS_ERROR_INVALID_ARG);

Please use

if (NS_WARN_IF(!keyEvent)) {
  return NS_ERROR_INVALID_ARG;
}

@@ +709,5 @@
>  
> +  RefPtr<KeyboardEvent> keyEvent;
> +  if (aDOMKeyEvent) {
> +    keyEvent = aDOMKeyEvent->InternalDOMEvent()->AsKeyboardEvent();
> +    NS_ENSURE_TRUE(keyEvent, NS_ERROR_INVALID_ARG);

ditto.

@@ +757,5 @@
>  
> +  RefPtr<KeyboardEvent> keyEvent;
> +  if (aDOMKeyEvent) {
> +    keyEvent = aDOMKeyEvent->InternalDOMEvent()->AsKeyboardEvent();
> +    NS_ENSURE_TRUE(keyEvent, NS_ERROR_INVALID_ARG);

ditto.

@@ +784,5 @@
>  
> +  RefPtr<KeyboardEvent> keyEvent;
> +  if (aDOMKeyEvent) {
> +    keyEvent = aDOMKeyEvent->InternalDOMEvent()->AsKeyboardEvent();
> +    NS_ENSURE_TRUE(keyEvent, NS_ERROR_INVALID_ARG);

ditto.

@@ +852,5 @@
>  
> +  RefPtr<KeyboardEvent> keyEvent;
> +  if (aDOMKeyEvent) {
> +    keyEvent = aDOMKeyEvent->InternalDOMEvent()->AsKeyboardEvent();
> +    NS_ENSURE_TRUE(keyEvent, NS_ERROR_INVALID_ARG);

ditto.
Attachment #8949619 - Flags: review?(masayuki) → review+
Comment on attachment 8949623 [details] [diff] [review]
part 16.  Remove nsIDOMSimpleGestureEvent

Review of attachment 8949623 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/SimpleGestureEvent.cpp
@@ +40,4 @@
>  NS_INTERFACE_MAP_END_INHERITING(MouseEvent)
>  
>  uint32_t
> +SimpleGestureEvent::AllowedDirections() const

Yeah, we should use |const| more.

::: dom/webidl/SimpleGestureEvent.webidl
@@ +4,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +/**
> + * The SimpleGestureEvent interface is the datatype for all

Nice to move the explanation from the removing idl!

::: widget/cocoa/SwipeTracker.mm
@@ +70,5 @@
>  
>  double
>  SwipeTracker::SwipeSuccessTargetValue() const
>  {
> +  return (mSwipeDirection == dom::SimpleGestureEventBinding::DIRECTION_RIGHT) ? -1.0 : 1.0;

nit: too long line.

@@ +79,5 @@
>  {
>    // gestureAmount needs to stay between -1 and 0 when swiping right and
>    // between 0 and 1 when swiping left.
> +  double min = (mSwipeDirection == dom::SimpleGestureEventBinding::DIRECTION_RIGHT) ? -1.0 : 0.0;
> +  double max = (mSwipeDirection == dom::SimpleGestureEventBinding::DIRECTION_LEFT) ? 1.0 : 0.0;

nit: too long lines.

::: widget/cocoa/nsChildView.mm
@@ +4323,5 @@
>    WidgetSimpleGestureEvent geckoEvent(true, msg, mGeckoChild);
>    [self convertCocoaMouseEvent:anEvent toGeckoEvent:&geckoEvent];
>    geckoEvent.mDelta = -rotation;
>    if (rotation > 0.0) {
> +    geckoEvent.mDirection = dom::SimpleGestureEventBinding::ROTATION_COUNTERCLOCKWISE;

nit: too long line.

@@ +4415,5 @@
>        WidgetSimpleGestureEvent geckoEvent(true, eRotateGesture, mGeckoChild);
>        [self convertCocoaMouseEvent:anEvent toGeckoEvent:&geckoEvent];
>        geckoEvent.mDelta = -mCumulativeRotation;
>        if (mCumulativeRotation > 0.0) {
> +        geckoEvent.mDirection = dom::SimpleGestureEventBinding::ROTATION_COUNTERCLOCKWISE;

nit: too long line.

::: widget/windows/nsWinGesture.cpp
@@ +207,5 @@
>        evt.mDelta = degrees - mRotateIntermediate;
>        mRotateIntermediate = degrees;
>  
>        if (evt.mDelta > 0)
> +        evt.mDirection = dom::SimpleGestureEventBinding::ROTATION_COUNTERCLOCKWISE;

nit: too long line.
Attachment #8949623 - Flags: review?(masayuki) → review+
> perhaps, we should implement this kind of methods with macro and a header file listing all event classes.

We already do that for all generated events, effectively...  Ideally we'd move toward more generated events.

> nit: too long line. please break this line before landing.

Done, and similar for the other long line comments.

> If you have some special reason to reduce dependency of KeyboardEventBinding.h,
> please write it in the commit message.

I tried to.  But actually, I ended up having to deal with that issue in later patches (by exporting the global window headers) anyway, so I will pull that back to this patch and just undo the #include changes.

> I guess that here may not have global object, win.

Right, it's unclear whether we do.  And since we _do_ have an event, this is the easy and safe way to go.

> nit: only whitespaces line.

Fixed.

> Looks like that this if/else if blocks can be rewritten with switch statement.

OK, done.

> if (NS_WARN_IF(!keyEvent)) {

Done, all 5 places.

Comment 36

a year ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4db36d5d4945
part 1.  Remove nsIDOMAnimationEvent.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e6feaec3d6a
part 2.  Remove nsIDOMBeforeUnloadEvent.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc87507a729b
part 3.  Remove nsIDOMCommandEvent.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/51d688aa1c80
part 4.  Remove some XPOM goop from ExtendableMessageEvent.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e2f9290346a
part 5.  Switch xbl from nsIDOMKeyEvent to KeyboardEvent.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1f2b10c62e7
part 6.  Switch layout/xul from nsIDOMKeyEvent to KeyboardEvent.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee44459b72f6
part 7.  Remove nsIDOMKeyEvent usage from formfill and spellcheck.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/745e6034684b
part 8.  Remove unused nsIDOMKeyEvent members.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/498d0e9ef3da
part 9.  Remove nsIDOMKeyEvent::DOM_KEY* constants.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/b01bd36c3899
part 10.  Remove use of nsIDOMKeyEvent in JS.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ae4bf25badf
part 11.  Remove the use of the nsIDOMKeyEvent::DOM_VK* constants in C++.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/f91374248200
part 12.  Remove nsIDOMKeyEvent.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fcaebaeee9f
part 13.  Remove nsIDOMMutationEvent constants.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/15f68311633e
part 14.  Remove nsIDOMMutationEvent.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c5e16dfd42e
part 15.  Remove nsIDOMTransitionEvent.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/f882418b2d05
part 16.  Remove nsIDOMSimpleGestureEvent.  r=masayuki
Depends on: 1437135
No longer depends on: 1437135

Updated

a year ago
Depends on: 1437269
Depends on: 1437353

Updated

a year ago
Depends on: 1439974
Depends on: 1441241
You need to log in before you can comment on or make changes to this bug.