Cache interned ids for JS-implemented WebIDL methods (and other callbacks?)

RESOLVED FIXED in mozilla32

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bz, Assigned: froydnj)

Tracking

unspecified
mozilla32
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

We'd need to add structs to GeneratedAtomList in Codegen.py for storing the actual jsids for all JS-implemented WebIDL (and probably for callback interfaces in general), then change the codegen to intern the ids as needed and then use the "Id" version of JS_GetProperty/JS_SetProperty/etc.
(Assignee)

Comment 1

4 years ago
Created attachment 8432938 [details] [diff] [review]
part 1 - factor out initIdsClassMethod from CGDictionary

We need to create this method for more than just dictionaries, so we need a
function that will work for interfaces, too.
Attachment #8432938 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

4 years ago
Created attachment 8432939 [details] [diff] [review]
part 2 - create atom caches for JS-implemented interfaces

...and for callback interfaces, too.
Attachment #8432939 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

4 years ago
Created attachment 8432940 [details] [diff] [review]
part 3 - use ids to access JS-implemented interface properties

This patch is plugging in the access to the atoms cache that we created in part
2...and making a bunch of other small changes to make that work: includes,
forward declarations, etc.  This is the part that I'm less sure about, but
everything compiles at least.
Attachment #8432940 - Flags: review?(bzbarsky)
Comment on attachment 8432938 [details] [diff] [review]
part 1 - factor out initIdsClassMethod from CGDictionary

r=me
Attachment #8432938 - Flags: review?(bzbarsky) → review+
Comment on attachment 8432939 [details] [diff] [review]
part 2 - create atom caches for JS-implemented interfaces

>+            return ClassMember(CGDictionary.makeIdName(m.identifier.name),

Don't we want the correct binaryname for the member?

>+        for d in config.getDescriptors(isJSImplemented=True):
>+        for d in config.getDescriptors(isCallback=True):

  for d in (config.getDescriptors(isJSImplemented=True) +
            config.getDescriptors(isCallback=True)):

>+            attributes = [m if m.isAttr() for m in d.interface.members]

This fails to run under python 2.7.5, afaict.  It wants:

  attributes = [m for m in d.interface.members if m.isAttr()]

But also, why are we excluding methods?  We want those too.

Note that we'll need to do something with the special __init thing on JS-implemented interfaces with a ctor. 

r=me with the above fixed.
Attachment #8432939 - Flags: review?(bzbarsky) → review+
Comment on attachment 8432940 [details] [diff] [review]
part 3 - use ids to access JS-implemented interface properties

>-            if not method.needThisHandling:
>+            if method.static or not method.needThisHandling:

Why this change?

>+        if any(m.isAttr() for m in descriptor.interface.members):

Again, we should include methods and __init here.

>+            methods.append(initIdsClassMethod([m.identifier.name

That _definitely_ needs the correct binary name.  Look at the codegen for the attributeRenamedFrom/To bits in TestJSImplGen before and after this change.

>+            if (!*reinterpret_cast<jsid**>(atomsCache) && !InitIds(cx, atomsCache)
>+                ? !JS_GetProperty(cx, callback, "${attrName}", &rval)

Shouldn't we just throw if !InitIds?  So something like:

  if (!(*reinterpret_cast<jsid**>(atomsCache) && !InitIds(cx, atomsCache)) ||
      !JS_GetPropertyById(cx, callback, atomsCache->${attrAtomName}, &rval)) {

>+            attrAtomName=CGDictionary.makeIdName(self.attrName),

Again, need the right binaryname thing.  See what we do for attrName.
Attachment #8432940 - Flags: review?(bzbarsky) → review-
Created attachment 8433049 [details] [diff] [review]
Some hackery to sorta make methods limp
(Assignee)

Comment 8

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #5)
> Comment on attachment 8432939 [details] [diff] [review]
> part 2 - create atom caches for JS-implemented interfaces
> 
> >+            return ClassMember(CGDictionary.makeIdName(m.identifier.name),
> 
> Don't we want the correct binaryname for the member?

Um.  Probably?  I am unfamiliar with binaryname.

> >+        for d in config.getDescriptors(isJSImplemented=True):
> >+        for d in config.getDescriptors(isCallback=True):
> 
>   for d in (config.getDescriptors(isJSImplemented=True) +
>             config.getDescriptors(isCallback=True)):

Gah, I knew there must be a better way to do this, I just couldn't think of it.

> >+            attributes = [m if m.isAttr() for m in d.interface.members]
> 
> This fails to run under python 2.7.5, afaict.  It wants:
> 
>   attributes = [m for m in d.interface.members if m.isAttr()]

I apparently couldn't read the Python language grammar closely enough to get this to work.  Will fix.

> But also, why are we excluding methods?  We want those too.

I hadn't looked far enough to see that we needed methods.  I'll try doing that.

(In reply to Boris Zbarsky [:bz] from comment #6)
> Comment on attachment 8432940 [details] [diff] [review]
> part 3 - use ids to access JS-implemented interface properties
> 
> >-            if not method.needThisHandling:
> >+            if method.static or not method.needThisHandling:
> 
> Why this change?

Because ClassMethod doesn't have a needThisHandling attribute, so you get Python errors if you don't check for method.static.  I think this is clunky, but I'm not sure what the right thing here is.  (The whole callback/jsimplemented stuff sharing code but not clearly delineated in the code/CGFoo hierarchy is kind of a mess, IMHO.)

> >+            if (!*reinterpret_cast<jsid**>(atomsCache) && !InitIds(cx, atomsCache)
> >+                ? !JS_GetProperty(cx, callback, "${attrName}", &rval)
> 
> Shouldn't we just throw if !InitIds?  So something like:
> 
>   if (!(*reinterpret_cast<jsid**>(atomsCache) && !InitIds(cx, atomsCache)) ||
>       !JS_GetPropertyById(cx, callback, atomsCache->${attrAtomName}, &rval))
> {

Throwing on property accesses because of the JS engine's inability to intern things seems bad.  Though I guess if we can't intern things, the world is about to fall over anyway...
> Um.  Probably?  I am unfamiliar with binaryname.

It's the thing that lets us have C++ names not matching the JS ones.  For JS-implemented stuff, the binaryname is what's used on the chrome side.

> Because ClassMethod doesn't have a needThisHandling attribute

Oh, I see.  Yeah, I agree this setup is a bit of an annoyance.

How about just a isinstance(CallbackMember) test, since that's what we really want?

> Though I guess if we can't intern things, the world is about to fall over anyway

Exactly.  Plus, the failure to intern already threw an exception on cx, so we _really_ don't want to be entering script at that point.
(Assignee)

Comment 10

4 years ago
Created attachment 8435165 [details] [diff] [review]
part 1 - factor out initIdsClassMethod from CGDictionary
Attachment #8432938 - Attachment is obsolete: true
Attachment #8435165 - Flags: review+
(Assignee)

Comment 11

4 years ago
Created attachment 8435167 [details] [diff] [review]
part 2 - separate out some Fake* class changes
Attachment #8435167 - Flags: review?(bzbarsky)
(Assignee)

Comment 12

4 years ago
Created attachment 8435168 [details] [diff] [review]
part 3 - create atom caches for JS-implemented interfaces

I know you r+'d this as part 2 earlier, but it'd probably be good to have
another look at it.
Attachment #8432939 - Attachment is obsolete: true
Attachment #8435168 - Flags: review?(bzbarsky)
(Assignee)

Comment 13

4 years ago
Created attachment 8435170 [details] [diff] [review]
part 4 - use jsids to access JS-implemented interface properties

Methods, getters, and setters...I think I got everything this time.
Attachment #8432940 - Attachment is obsolete: true
Attachment #8433049 - Attachment is obsolete: true
Attachment #8435170 - Flags: review?(bzbarsky)
Comment on attachment 8435167 [details] [diff] [review]
part 2 - separate out some Fake* class changes

r=me, I guess, though I sorta wrote this bit... ;)

I assume your leaving it as-is counts as review from you.
Attachment #8435167 - Flags: review?(bzbarsky) → review+
Comment on attachment 8435168 [details] [diff] [review]
part 3 - create atom caches for JS-implemented interfaces

>+        def buildAtomCacheStructure(d, binaryNameFor, members):

That first argument could use a better name than "d".  Maybe IDLObject?

>+            attributes = [m for m in d.interface.members if m.isAttr() or m.isMethod()]

Maybe "members" instead of "attributes", since it includes methods?

r=me
Attachment #8435168 - Flags: review?(bzbarsky) → review+
Comment on attachment 8435170 [details] [diff] [review]
part 4 - use jsids to access JS-implemented interface properties

>+            if (!(*reinterpret_cast<jsid**>(atomsCache) && !InitIds(cx, atomsCache)) ||
>+               !GetCallableProperty(cx, atomsCache->${methodAtomName}, &callable)) {

This doesn't look right to me.  If *reinterpret_cast<jsid**>(atomsCache) is null (which is is to start with) this condition will test true.  I think that first '!' needs to move inside the paren before '*'.

Also, fix the indent of the second line, please.

r=me with that fixed.  Thank you for doing this!
Attachment #8435170 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

4 years ago
Assignee: nobody → nfroyd
Oh, I wasn't worrying about the credit, more the reviewing-code-I-wrote-myself being a semi-useless exercise.  ;)
You need to log in before you can comment on or make changes to this bug.