Closed Bug 1170771 Opened 6 years ago Closed 3 years ago

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

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bzbarsky, Assigned: adrian17)

References

Details

Attachments

(2 files)

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.
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.
Assignee: nobody → adrian.wielgosik
Status: NEW → ASSIGNED
Whoa, removing nsDOMClassInfo.cpp! That's pretty awesome. :-)
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+
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 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.
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: 3 years ago
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.