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

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
6 years ago
3 months ago

People

(Reporter: bzbarsky, Assigned: mccr8)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

I get the feeling bug 886110 will need this.
Need to figure out what the right API here is... I guess I can just do chromeonly _clearCachedFooValue things on the object....
Blake, are you still doing cached dictionaries in bug 886110?  Trying to prioritize things....
Flags: needinfo?(mrbkap)
I don't think so. We ended up using an interface.
Flags: needinfo?(mrbkap)
Blocks: 899322
Could we expose the ClearCachedXXXValue on the __DOM_IMPL__ object? Or is it what bz means in comment 1?
That is what bz meant in comment 1.
Flags: needinfo?(bzbarsky)
Assignee

Comment 6

5 years ago
Posted patch basic tests. (obsolete) — Splinter Review
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.
Assignee

Comment 7

5 years ago
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?
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.
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bzbarsky)
Assignee

Comment 9

5 years ago
I have a crude patch-in-progress that sort of works.
Assignee: bzbarsky → continuation
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Assignee

Comment 12

5 years ago
This uses existing codegen stuff to pull out the "this" object, which required moving code around.
Attachment #8473070 - Attachment is obsolete: true
Assignee

Comment 13

5 years ago
Attachment #8473264 - Attachment is obsolete: true
Assignee

Comment 14

5 years ago
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...
Attachment #8473343 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8473069 - Flags: review?(bzbarsky)
Assignee

Updated

5 years ago
Attachment #8473902 - Flags: review?(bzbarsky)
Comment on attachment 8473069 [details] [diff] [review]
part 1 - Split out MethodDefiner's JS implemented case into its own thing.

r=me
Attachment #8473069 - Flags: review?(bzbarsky) → review+
Flags: needinfo?(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.
Attachment #8473902 - Flags: review?(bzbarsky) → review+
Assignee

Updated

5 years ago
Keywords: dev-doc-needed
Assignee

Comment 17

5 years ago
<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.")
Assignee

Comment 18

5 years ago
(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
>+         bindingNamespace=toBindingNamespace(self.descriptor.interface.identifier.name),

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

Comment 20

5 years ago
(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.
Depends on: 1117131
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.