1473492 - Possibly dead code in Codegen.py

Status: RESOLVED FIXED

(Core :: DOM: Bindings (WebIDL), enhancement)

Description

[Jan de Mooij [:jandem]]

The code snippets in this branch are not emitted for any bindings, according to searchfox:

https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/dom/bindings/Codegen.py#8515-8517

Is this expected? Should we remove/keep?

Comment 1

[Jan de Mooij [:jandem]]

I'm interested in this because there's a js::GetGlobalForObjectCrossCompartment call there. Easy to convert but if we can remove it that's even better :)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Looks like this code became dead in bug 1451516.

Specifically, the only way to reach this code is for someone to call CGAbstractBindingMethod.__init__ without passing in a value for getThisObj, or passing an explicit None.  We have calls to CGAbstractBindingMethod.__init__ from the following places: CGLegacyCallHook, CGEnumerateHook, CGResolveOwnPropertyViaResolve, CGEnumerateOwnPropertiesViaGetOwnPropertyNames, CGJSImplClearCachedValueMethod.

All of these except CGJSImplClearCachedValueMethod pass a value for getThisObj.  And CGJSImplClearCachedValueMethod never appears on a global's proto chain, because we never have globals inheriting from JS-implemented interfaces.

That said, CGJSImplClearCachedValueMethod is dead code outside of tests anyway; it was initially added for JS-implemented APIs with cached attributes, but all of those were b2g-specific and have since been removed.

Given that JS-implemented webidl is deprecated, I'm going to just go ahead and remove the complexity.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Remove support for clearing cached attributes on JS-implemented webidl object

This more or less backs out bug 963382.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Disallow [Cached] on JS-implemented interfaces

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Make the getThisObj argument of CGAbstractBindingMethod required

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8990094 [details] [diff] [review]
part 2.  Disallow [Cached] on JS-implemented interfaces

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

::: dom/bindings/test/TestJSImplGen.webidl
@@ +40,5 @@
>    void passOptionalByteWithDefaultBeforeRequired(optional byte arg1 = 0, byte arg2);
>    void passNullableByte(byte? arg);
>    void passOptionalNullableByte(optional byte? arg);
>    void passVariadicByte(byte... arg);
> +  // [Cached] not supported in JS-implemented WebIDL.

Should these comments be "is not" instead of "not"?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Should these comments be "is not" instead of "not"?

Sure.  Fixed.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ebaffb961ac
part 1.  Remove support for clearing cached attributes on JS-implemented webidl object.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/6231278eb5bd
part 2.  Disallow [Cached] on JS-implemented interfaces.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/02d78ee0198c
part 3.  Make the getThisObj argument of CGAbstractBindingMethod required.  r=mccr8

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/1ebaffb961ac
https://hg.mozilla.org/mozilla-central/rev/6231278eb5bd
https://hg.mozilla.org/mozilla-central/rev/02d78ee0198c