Closed
Bug 1170771
Opened 9 years ago
Closed 7 years ago
Get rid of the this translator (SetFunctionThisTranslator and all the things that it uses, GetThisTranslatorMap, etc)
Categories
(Core :: XPConnect, defect)
Core
XPConnect
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.
Comment 1•7 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)
Comment 2•7 years ago
|
||
(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)
Comment 3•7 years ago
|
||
That sounds accurate, yes.
Assignee | ||
Comment 4•7 years ago
|
||
It looks like it's actionable now, so I made some patches.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → adrian.wielgosik
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•7 years ago
|
||
Comment 8•7 years ago
|
||
Whoa, removing nsDOMClassInfo.cpp! That's pretty awesome. :-)
Reporter | ||
Comment 9•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5e6526a99bc9
https://hg.mozilla.org/mozilla-central/rev/2ae59181b9de
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 16•7 years ago
|
||
Doesn't this change the behaviour of event listeners added through nsIEventListenerService? And what about nsISessionStoreUtils::createDynamicFrameEventFilter?
Flags: needinfo?(adrian.wielgosik)
Reporter | ||
Comment 17•7 years ago
|
||
> 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.
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(adrian.wielgosik)
Assignee | ||
Comment 18•7 years ago
|
||
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 :(
Reporter | ||
Comment 19•7 years ago
|
||
I filed bug 1453132 on dealing with that.
Updated•6 years ago
|
status-firefox41:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•