Get rid of the this translator (SetFunctionThisTranslator and all the things that it uses, GetThisTranslatorMap, etc)

RESOLVED FIXED in Firefox 61

Status

()

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: bzbarsky, Assigned: adrian17)

Tracking

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments)

We only use this for nsIDOMEventListener.  And I'm pretty sure that's currently only used for the non-webidl event targets, and then only via Xrays, I think (because the non-Xray case uses webidl quickstubs).

Once we fix bug 888591 we can kill this stuff off completely.

Comment 1

2 years ago
What's the status here? Looking at Bug 888591 the only one that's left is Bug 888600. Is that one required to fix this bug?
Flags: needinfo?(overholt)
(In reply to Florian Bender from comment #1)
> What's the status here?

I have no idea. Please ask Boris when his queue re-opens.
Flags: needinfo?(overholt)
That sounds accurate, yes.
It looks like it's actionable now, so I made some patches.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

Last year
Assignee: nobody → adrian.wielgosik
Status: NEW → ASSIGNED
Whoa, removing nsDOMClassInfo.cpp! That's pretty awesome. :-)
Reporter

Comment 9

Last year
mozreview-review
Comment on attachment 8965487 [details]
Bug 1170771 - Remove ThisTranslator and support code.

https://reviewboard.mozilla.org/r/234252/#review239934

This is awesome.  Thank you!

::: js/xpconnect/src/XPCWrappedJSClass.cpp:1073
(Diff revision 1)
>          // and try to make the call. If it turns out the named property is not
>          // a callable object then the JS engine will throw an error and we'll
>          // pass this along to the caller as an exception/result code.
>  
>          fval = ObjectValue(*obj);
> -        if (isFunction &&
> +        if (!(isFunction && JS_TypeOfValue(ccx, fval) == JSTYPE_FUNCTION)) {

I think this would be more readable as:

    if (!isFunction || !JS_TypeOfValue(...))
Attachment #8965487 - Flags: review?(bzbarsky) → review+
Reporter

Comment 10

Last year
mozreview-review
Comment on attachment 8965488 [details]
Bug 1170771 - Remove now-empty nsDOMClassInfo.

https://reviewboard.mozilla.org/r/234254/#review239936

Excellent.  ;)

::: dom/bindings/BindingUtils.cpp:3145
(Diff revision 1)
>  nsresult
>  RegisterDOMNames()

This can be changed to return void now, right?
Attachment #8965488 - Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 13

Last year
mozreview-review-reply
Comment on attachment 8965488 [details]
Bug 1170771 - Remove now-empty nsDOMClassInfo.

https://reviewboard.mozilla.org/r/234254/#review239936

> This can be changed to return void now, right?

Yup. Made it static, too.

Comment 14

Last year
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e6526a99bc9
Remove ThisTranslator and support code. r=bz
https://hg.mozilla.org/integration/autoland/rev/2ae59181b9de
Remove now-empty nsDOMClassInfo. r=bz
https://hg.mozilla.org/mozilla-central/rev/5e6526a99bc9
https://hg.mozilla.org/mozilla-central/rev/2ae59181b9de
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Doesn't this change the behaviour of event listeners added through nsIEventListenerService? And what about nsISessionStoreUtils::createDynamicFrameEventFilter?
Flags: needinfo?(adrian.wielgosik)
> Doesn't this change the behaviour of event listeners added through nsIEventListenerService?

Hmm... So it does.

Looking at the consumers of addListenerForAllEvents:

* The use in devtools/server/actors/thread.js passes a bound function, so it's fine.
* The use in browser/base/content/test/performance/head.js does not use "this".
* The uses in dom/events/test/test_bug448602.html don't use "this".

So those are all OK.  For addSystemEventListener (some of the files have multiple uses; I did look at all of them):

* browser/base/content/browser.js: Doesn't use "this".
* browser/base/content/content.js: Uses a non-function so doesn't need this translator.
* browser/base/content/tabbrowser.js: Uses a non-function.
* browser/components/customizableui/CustomizableUI.jsm: Uses a non-function.
* browser/components/syncedtabs/TabListView.js: Uses a non-function.
* browser/extensions/mortar/host/common/ppapi-runtime.jsm: Uses a bound function.
* browser/modules/ContextMenu.jsm: Uses a bound function.
* dom/browser-element/BrowserElementChildPreload.js: Uses a non-function.
* dom/browser-element/BrowserElementParent.js: Uses a bound function.
* toolkit/components/viewsource/content/viewSource-content.js: Uses a non-function.
* toolkit/content/browser-content.js: Uses a non-function.
* toolkit/content/widgets/tabbox.xml: Uses a non-function.
* browser/components/resistfingerprinting/test/mochitest/test_keyboard_event.html: Doesn't use "this".
* dom/events/test/pointerevents/test_bug1315862.html: Doesn't use "this".
* dom/events/test/test_dom_wheel_event.html: Doesn't use "this".
* editor/libeditor/tests/test_bug569988.html: Uses a non-function.
* editor/libeditor/tests/test_bug674770-1.html: Doesn't use "this".
* editor/libeditor/tests/test_contenteditable_text_input_handling.html: Uses a non-function.
* editor/libeditor/tests/test_htmleditor_keyevent_handling.html: Uses a non-function.
* editor/libeditor/tests/test_texteditor_keyevent_handling.html: Uses a non-function.
* layout/forms/test/test_bug935876.html: Doesn't use "this".
* testing/mochitest/tests/SimpleTest/EventUtils.js: Doesn't use "this".
* toolkit/components/satchel/test/test_popup_enter_event.html: Doesn't use "this".
* toolkit/content/tests/chrome/test_autocomplete_mac_caret.xul: Doesn't use "this".
* toolkit/content/tests/mochitest/test_autocomplete_change_after_focus.html: Doesn't use "this".
* widget/tests/test_keycodes.xul: Doesn't use "this".

So we're good on all those too.  For future ones people add, behavior will be a bit different from DOM addEventListener, but I think that's OK.  And if we care, we can work on switching the event listener service API to taking a jsval and creating a dom::EventListener from it...  Maybe that's a good idea anyway.

> And what about nsISessionStoreUtils::createDynamicFrameEventFilter?

All the callers (all are in browser/components/sessionstore/content/content-sessionStore.js) pass a non-function.  So we're good there too.
Flags: needinfo?(adrian.wielgosik)
Ah, I missed this indeed.

> For future ones people add, behavior will be a bit different from DOM addEventListener, but I think that's OK.

It seems like a potentially obscure footgun :(
I filed bug 1453132 on dealing with that.
You need to log in before you can comment on or make changes to this bug.