909623 - Make jsfriendapi.h not depend on jsapi.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Luke Wagner [:luke]]

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++.

Comment 2

[Nicholas Nethercote [inactive]]

> 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 :)

Comment 3

[Luke Wagner [:luke]]

Ah, good to know, thanks.

Comment 4

[Nicholas Nethercote [inactive]]

> - 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.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Move JS_SetGrayGCRootsTracer from jsfriendapi.{h,cpp} to jsapi.{h,cpp}.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Move GetObjectProto() from jsfriendapi.h to jsfriendapi.cpp.

I don't think this function is hot enough for its inlining to matter.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Move two ArrayBufferBuilder methods from jsfriendapi.{h,cpp} to jsapi.{h,cpp}.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Create js/CallNonGenericMethod.h and vm/CallNonGenericMethod.cpp.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Remove |#include "jsapi.h"| from jsfriendapi.h.

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.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Remove |#include "jsapi.h"| from jsfriendapi.h.

I added a comment about the JS_HAS_CTYPES thingy.

Comment 11

[Luke Wagner [: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?

Comment 12

[Luke Wagner [: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 13

[Luke Wagner [:luke]]

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.

Comment 14

[Luke Wagner [:luke]]

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).

Comment 15

[Luke Wagner [:luke]]

(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 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 796219 [details] [diff] [review]
(part 2) - Move GetObjectProto() from jsfriendapi.h to jsfriendapi.cpp.

Yeah, this is probably ok.

Comment 17

[Nicholas Nethercote [inactive]]

> > +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.

Comment 18

[Luke Wagner [:luke]]

(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.

Comment 19

[Nicholas Nethercote [inactive]]

> 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.

Comment 20

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Move ArrayBufferBuilder from jsfriendapi.{h,cpp} to nsXMLHttpRequest.{h,cpp}.

As requested.

My favourite part of doing this patch was changing the JS style to Gecko style.
That was really enjoyable.

Comment 21

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Move JS_TraceDataOp from jsapi.h to jspubtd.h.

As requested.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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....

Comment 23

[Luke Wagner [:luke]]

Comment on attachment 796345 [details] [diff] [review]
(part 3) - Move ArrayBufferBuilder from jsfriendapi.{h,cpp} to nsXMLHttpRequest.{h,cpp}.

Great, thanks!

Comment 24

[Luke Wagner [:luke]]

(In reply to Nicholas Nethercote [:njn] from comment #20)
Hehe, I'm sorry, I hadn't considered that.

Comment 25

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c05d093c8024
https://hg.mozilla.org/integration/mozilla-inbound/rev/df669fe7875b
https://hg.mozilla.org/integration/mozilla-inbound/rev/597530d998cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f686383d807
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cc68dacc74c

Comment 26

[Till Schneidereit [:till]]

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?

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c05d093c8024
https://hg.mozilla.org/mozilla-central/rev/df669fe7875b
https://hg.mozilla.org/mozilla-central/rev/597530d998cc
https://hg.mozilla.org/mozilla-central/rev/3f686383d807
https://hg.mozilla.org/mozilla-central/rev/9cc68dacc74c

Comment 28

[Nicholas Nethercote [inactive]]

> > +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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87f9d5cff71a