963382 - Add a way to clear cached attribute values for JS-implemented webidl

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I get the feeling bug 886110 will need this.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Need to figure out what the right API here is... I guess I can just do chromeonly _clearCachedFooValue things on the object....

Comment 2

[Boris Zbarsky [:bzbarsky]]

Blake, are you still doing cached dictionaries in bug 886110?  Trying to prioritize things....

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

I don't think so. We ended up using an interface.

Comment 4

[[:fabrice] Fabrice Desré]

Could we expose the ClearCachedXXXValue on the __DOM_IMPL__ object? Or is it what bz means in comment 1?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

That is what bz meant in comment 1.

Comment 6

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: basic tests.

Basic test for cached values with JS-implemented WebIDL.  Of course, the "not implemented" part needs to be replaced with an implementation of cache clearing when one exists.

Comment 7

[Andrew McCreight [:mccr8] (PTO until 12-2)]

I can look at this but I'm not entirely sure the right way to proceed.  Do I add an entry to the classinfo of the __DOM_IMPL__ object for the clearCache() method and somehow mark it chrome only?

Comment 8

[Boris Zbarsky [:bzbarsky]]

You can crib what we do for the _create static method over in MethodDefiner.__init__, but put it in the non-static branch of that "if not static" statement (but still add to self.chrome, so it ends up chromeonly).  And of course you have to codegen the actual JSNative it will call, probably in the same place as we do _Create.

We _could_ do that last by giving ClearCached*Value the signature of JSNative but with default values for the args, but that would not play well with JSNative moving to CallArgs in the future, so better to codegen a separate JSNative that calls the relevant ClearCached*Value bit.

Comment 9

[Andrew McCreight [:mccr8] (PTO until 12-2)]

I have a crude patch-in-progress that sort of works.

Comment 10

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: part 1 - Split out MethodDefiner's JS implemented case into its own thing.

Comment 11

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: part 2 - Add chrome JS function for clearing cache. WIP.

Comment 12

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: part 2 - Add chrome JS function for clearing cache.

This uses existing codegen stuff to pull out the "this" object, which required moving code around.

Comment 13

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: part 2 - Add chrome JS function for clearing cache.

Comment 14

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: part 2 - Add chrome JS function for clearing cache.

try run looks good: https://tbpl.mozilla.org/?tree=Try&rev=acd9403e05a8

I'll probably just wait until bz gets back and get him to review it, though if somebody else wants to feel free...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8473069 [details] [diff] [review]
part 1 - Split out MethodDefiner's JS implemented case into its own thing.

r=me

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8473902 [details] [diff] [review]
part 2 - Add chrome JS function for clearing cache.

>+def clearableCachedAttrs(descriptor):

Does this really need to return a list, or can it return a generator (as in, just replace [] with ())?

>+            ${ifaceName}Binding::${fnName}(self);

You probably want toBindingNamespace(self.descriptor.name) instead of "${ifaceName}Binding".  Though in practice it's the same thing, since these are all guaranteed non-worker...

You also want to pass the cx if the member is [StoreInSlot], no?  See the signature CGClearCachedValueMethod produces.  And in that case you want to return the return value of ${fnName}, not always true.  Add a test using [StoreInSlot], please.

>+    def deps(self):

Why is this needed?  Who invokes deps() on this CGThing?

r=me with those fixed and the deps() bit removed or explained.

Comment 17

[Andrew McCreight [:mccr8] (PTO until 12-2)]

<bz_away> Basically, you should be able to add support for StoreInSlot bits there, just not test it easily...
<bz_away> So maybe it's not worth it and instead just add an assert that !StoreInSlot
<bz_away> in case we ever make it OK to StoreInSlot on throwing things, or to have non-throwing jsimpl thing
<bz_away> er, things
<mccr8> sounds good.  I can file a followup bug that people can complain about if it becomes needed or something.
<bz_away> With a comment about why we're asserting that
<bz_away> Sounds like a plan

I filed bug 1056325 for supporting StoreInSlot with JS-implemented WebIDL. We hit an assert in codegen even without my patch.  Rather than an assert, I'm going to make it throw an exception in CGJSimplClearCachedValueMethod:
        if attr.getExtendedAttribute("StoreInSlot"):
            raise TypeError("[StoreInSlot] is not supported for JS-implemented WebIDL. See bug 1056325.")

Comment 18

[Andrew McCreight [:mccr8] (PTO until 12-2)]

(In reply to Boris Zbarsky [:bz] from comment #16)
> Does this really need to return a list, or can it return a generator (as in,
> just replace [] with ())?
Ah, I didn't know that was a thing. Fixed.

> You probably want toBindingNamespace(self.descriptor.name) instead of
> "${ifaceName}Binding".
Fixed.

> You also want to pass the cx if the member is [StoreInSlot], no?
As discussed, I made it throw an exception that references a bug for supporting StoreInSlot.

> Why is this needed?  Who invokes deps() on this CGThing?
Oops, looks like it isn't.  At some point it was needed, but I guess that was before I changed where it was generated.  Removed.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c9ce133b845e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/019faa34f194

Comment 19

[Boris Zbarsky [:bzbarsky]]

>+         bindingNamespace=toBindingNamespace(self.descriptor.interface.identifier.name),

No, you really did want toBindingNamespace(self.descriptor.name), imo.

Comment 20

[Andrew McCreight [:mccr8] (PTO until 12-2)]

(In reply to Boris Zbarsky [:bz] from comment #19)
> No, you really did want toBindingNamespace(self.descriptor.name), imo.
Oops.  Fixed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/896985c272e0

I added a sentence to the section on [Cached] that mentions how to clear the cache for JS-implemented WebIDL:
  https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Cached

Thanks for laying out how to fix this, Boris.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c9ce133b845e
https://hg.mozilla.org/mozilla-central/rev/019faa34f194
https://hg.mozilla.org/mozilla-central/rev/896985c272e0