Closed Bug 909623 Opened 7 years ago Closed 7 years ago

Make jsfriendapi.h not depend on jsapi.h

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: [js:t])

Attachments

(5 files, 3 obsolete files)

This is like bug 909597, but for jsfriendapi.h.

The tricky parts here are the following things which are defined in jsapi.h and used in jsfriendapi.h.

- IsAcceptableThis and NativeImpl.  I see two options: (a) move the typedefs to js/CallArg.h, or (b) move them, along with CallNonGenericMethod() and CallMethodIfWrapped, into a new public header.  The latter is more work but less grotty.

- JSTraceDataOp.  Probably the easiest thing is to move JS_SetGrayGCRootsTracer() from jsfriendapi.h to jsapi.h, next to JS_{Add,Remove}ExtraGCRootsTracer().

- JS_GetPrototype(), used in GetObjectProto().  If the path it's on is not hit often, we could move it into a separate function in jsfriendapi.cpp.  Otherwise, I'm not sure.

- JS_ReallocateArrayBufferContents() and JS_NewArrayBufferWithContents().  These are both used within ArrayBufferBuilder methods.  If those methods aren't too hot, they could easily be moved into jsfriendapi.cpp.  Alternatively, ArrayBufferBuilder's only use is in nsXMLHttpRequest.{h,cpp};  it could just be reimplemented there.
Are there many uses of jsfriendapi.h that do not already depend on jsapi.h?  I always thought jsfriendapi.h was sort of like jsapi.h++.
> Are there many uses of jsfriendapi.h that do not already depend on jsapi.h? 

There are two important ones.

DOMJSClass.h is one.  It uses GetGlobalForObjectCrossCompartment(), GetObjectClass(), and GetReservedSlot().

jsproxy.h is the other.  And I need to make jsproxy.h not depend on jsapi.h because it's used all over the place in Gecko, and doing that once jsfriendapi.h doesn't depend on jsapi.h is easy.  Alternatively, that could be achieved by moving *ProxyClassPtr from jsfriendapi to jsproxy... except that would also require moving that call to JS_GetPrototype() from jsfriendapi.h to jsproxy.h.  Doable, but unless DOMJSClass.h can be handled in some other way, we need this bug.

> I always thought jsfriendapi.h was sort of like jsapi.h++.

Well, for 1700+ lines of code, jsfriendapi.h doesn't depend very much on jsapi.h.  I think of it sort of as an under-the-covers alternative to jsapi.h :)
Ah, good to know, thanks.
> - JS_GetPrototype(), used in GetObjectProto().  If the path it's on is not
> hit often, we could move it into a separate function in jsfriendapi.cpp. 
> Otherwise, I'm not sure.

The "hit" path is taken most of the time, so this isn't an option.  But it may be ok to just move GetObjectProto() into jsfriendapi.cpp.  I started up and browsed to three webpages (about 30 seconds wroth) and only got ~30,000 hits, which is few enough that inlining probably doesn't matter.
I don't think this function is hot enough for its inlining to matter.
Attachment #796219 - Flags: review?(luke)
If you're wondering about the removal of the JS_HAS_CTYPES guard in
jsfriendapi.h -- it's conceptually correct, but the problem is that
JS_HAS_CTYPES is defined (when defined) in js-config.h, which hasn't been
included at this point in the file!  It's bug 909576 biting again.
Attachment #796226 - Flags: review?(luke)
I added a comment about the JS_HAS_CTYPES thingy.
Attachment #796228 - Flags: review?(luke)
Comment on attachment 796219 [details] [diff] [review]
(part 2) - Move GetObjectProto() from jsfriendapi.h to jsfriendapi.cpp.

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

::: js/src/jsproxy.h
@@ +266,5 @@
> +// These are equal to |&{Function,Object,OuterWindow}ProxyObject::class_|.  Use
> +// them in places where you don't want to #include vm/ProxyObject.h.
> +extern js::Class* const FunctionProxyClassPtr;
> +extern js::Class* const ObjectProxyClassPtr;
> +extern js::Class* const OuterWindowProxyClassPtr;

So, presumably these would go away when we cleave jsproxy.h into a public/ProxyHandler.h and then, from the now-private internal header, we could #include ProxyObject.h and use FunctionProxyObject::class_ directly?
Attachment #796219 - Flags: review?(luke) → review+
Attachment #796226 - Attachment is obsolete: true
Attachment #796226 - Flags: review?(luke)
(In reply to Nicholas Nethercote [:njn] from comment #7)
ArrayBufferBuilder doesn't seem particularly friend-y.  How about a js/public/ArrayBufferBuilder.h?  That would avoid even needing to #include jsfriendapi.h.
Comment on attachment 796222 [details] [diff] [review]
(part 4) - Create js/CallNonGenericMethod.h and vm/CallNonGenericMethod.cpp.

I'm not thrilled about a proliferation of single-function .cpps.  It's only 2 so far, but if we start to see more, perhaps we can find some better logical grouping.
Attachment #796222 - Flags: review?(luke) → review+
Comment on attachment 796228 [details] [diff] [review]
(part 5) - Remove |#include "jsapi.h"| from jsfriendapi.h.

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

::: js/src/jsfriendapi.h
@@ +9,5 @@
>  
> +// XXX: MemoryReporting.h is only needed in JS_HAS_CTYPES builds.  But we can't
> +// make this include statement conditional because JS_HAS_CTYPES is defined
> +// (when defined) in js-config.h, which hasn't been included yet!
> +// If bug 909576 is fixed, we could.

MemoryReporting.h is really small so this doesn't seem worthwhile to bother everyone with a big flashy (yellow-colored, by my editor) XXX label (or even a comment, really).
Attachment #796228 - Flags: review?(luke) → review+
Attachment #796219 - Flags: review?(bzbarsky)
(In reply to Nicholas Nethercote [:njn] from comment #5)
I don't really like moving JS_FRIEND_API stuff into jsapi.h.  Is it just for the JSTraceDataOp typedef?  Seems like that could be hoisted: we appear to already hoist jsapi.h/jsfriendapi.h-shared definitions (including other callback typdefs) to jspubtd.h.  I know we don't want to balloon jspubtd.h but this is one typedef and it seems like we could refactor jspubtd.h more if necessary later.
Comment on attachment 796219 [details] [diff] [review]
(part 2) - Move GetObjectProto() from jsfriendapi.h to jsfriendapi.cpp.

Yeah, this is probably ok.
Attachment #796219 - Flags: review?(bzbarsky) → review+
> > +extern js::Class* const FunctionProxyClassPtr;
> > +extern js::Class* const ObjectProxyClassPtr;
> > +extern js::Class* const OuterWindowProxyClassPtr;
> 
> So, presumably these would go away when we cleave jsproxy.h into a
> public/ProxyHandler.h and then, from the now-private internal header, we
> could #include ProxyObject.h and use FunctionProxyObject::class_ directly?

They're needed for IsObjectProxyClass() and IsFunctionProxyClass(), which are used by Gecko.  So I don't think they could be made private like that.

> ArrayBufferBuilder doesn't seem particularly friend-y.  How about a
> js/public/ArrayBufferBuilder.h?  That would avoid even needing to #include
> jsfriendapi.h.

It only has one user in Gecko.  I'd be more inclined to move it there, which I think can be done.

> I don't really like moving JS_FRIEND_API stuff into jsapi.h.  Is it just for
> the JSTraceDataOp typedef?  Seems like that could be hoisted: we appear to
> already hoist jsapi.h/jsfriendapi.h-shared definitions (including other
> callback typdefs) to jspubtd.h.  I know we don't want to balloon jspubtd.h
> but this is one typedef and it seems like we could refactor jspubtd.h more
> if necessary later.

It is just for the JSTraceDataOp typedef.  I figured it was ok because there is already FRIEND stuff in jsapi.h.  But I'm ok to delay solving the problem properly to make progress now.
(In reply to Nicholas Nethercote [:njn] from comment #17)
> They're needed for IsObjectProxyClass() and IsFunctionProxyClass(), which
> are used by Gecko.  So I don't think they could be made private like that.

Oh, I assumed IsFunctionProxyClass et al weren't used; it seems like you'd need JS_PUBLIC_DATA for FunctionProxyClassPtr, then.

> > ArrayBufferBuilder doesn't seem particularly friend-y.  How about a
> > js/public/ArrayBufferBuilder.h?  That would avoid even needing to #include
> > jsfriendapi.h.
> 
> It only has one user in Gecko.  I'd be more inclined to move it there, which
> I think can be done.

Yes, moving it to the use site, if it operations in terms of jsapi.h sounds like what should have been done in the first place.

> It is just for the JSTraceDataOp typedef.  I figured it was ok because there
> is already FRIEND stuff in jsapi.h.  But I'm ok to delay solving the problem
> properly to make progress now.

There are only 2 which probably should be moved to jsfriendapi.h.
> Oh, I assumed IsFunctionProxyClass et al weren't used; it seems like you'd
> need JS_PUBLIC_DATA for FunctionProxyClassPtr, then.

The Windows try builds agree with you.  I'll put the JS_FRIEND_DATA back.
As requested.

My favourite part of doing this patch was changing the JS style to Gecko style.
That was really enjoyable.
Attachment #796345 - Flags: review?(luke)
Attachment #796345 - Flags: review?(bzbarsky)
Attachment #796220 - Attachment is obsolete: true
Attachment #796220 - Flags: review?(luke)
Attachment #796214 - Attachment is obsolete: true
Attachment #796214 - Flags: review?(luke)
Comment on attachment 796345 [details] [diff] [review]
(part 3) - Move ArrayBufferBuilder from jsfriendapi.{h,cpp} to nsXMLHttpRequest.{h,cpp}.

A few remainin style nits:

1)  s/NULL/nullptr/

2)  Single-line ifs are braced in DOM code.  Either way on this one.

3)  length() and capacity() can probably stay inline.

4)  "  public:" should not be indented.  And the members should be two-space
    indented, not 4-space.

r=me with those dealt with, and thank you for putting up with the hassle....
Attachment #796345 - Flags: review?(bzbarsky) → review+
Comment on attachment 796345 [details] [diff] [review]
(part 3) - Move ArrayBufferBuilder from jsfriendapi.{h,cpp} to nsXMLHttpRequest.{h,cpp}.

Great, thanks!
Attachment #796345 - Flags: review?(luke) → review+
(In reply to Nicholas Nethercote [:njn] from comment #20)
Hehe, I'm sorry, I hadn't considered that.
Attachment #796349 - Flags: review?(luke) → review+
Comment on attachment 796345 [details] [diff] [review]
(part 3) - Move ArrayBufferBuilder from jsfriendapi.{h,cpp} to nsXMLHttpRequest.{h,cpp}.

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

::: content/base/src/nsXMLHttpRequest.h
@@ +89,5 @@
> +    uint32_t capacity();
> +
> +    JSObject* getArrayBuffer(JSContext* aCx);
> +
> +protected:  // njn?

This seems to have made it into inbound. I guess you wanted to check something before landing?
> > +protected:  // njn?
> 
> This seems to have made it into inbound. I guess you wanted to check
> something before landing?

Yes!  Thanks for catching this.  Fixed here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87f9d5cff71a
You need to log in before you can comment on or make changes to this bug.