910109 - Make jsproxy.h and jswrapper.h not depend on jsapi.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Like jsfriendapi.h and jsdbgapi.h, jsproxy.h and jswrapper.h need not depend on jsapi.h.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Make jsproxy.h not depend on jsapi.h.

The most notable feature of this patch is the addition of lots of JS::
qualifiers, because jsapi.h's "inject tons of JS:: types into the js::
namespace" hack is no longer visible in jsproxy.h.  I view this as a good
thing -- I don't much like that hack, *especially* in public header files.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Make jswrapper.h not depend on jsapi.h.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 796500 [details] [diff] [review]
(part 1) - Make jsproxy.h not depend on jsapi.h.

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

::: js/src/jsproxy.h
@@ +84,5 @@
>      virtual bool isOuterWindow() {
>          return false;
>      }
>  
> +    virtual bool finalizeInBackground(JS::Value priv) {

Instead of JS:: prefixing everything, can you put
  using JS::Value;
et al after the 'namespace js {' above?

It seems like the goal for this file is for most of this to be hoisted into a public/ProxyHandler.h and then all of these definitions will be in namespace JS anyway.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 796502 [details] [diff] [review]
(part 2) - Make jswrapper.h not depend on jsapi.h.

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

::: js/src/jswrapper.h
@@ +74,5 @@
>      CrossCompartmentWrapper(unsigned flags, bool hasPrototype = false);
>  
>      virtual ~CrossCompartmentWrapper();
>  
> +    virtual bool finalizeInBackground(JS::Value priv) MOZ_OVERRIDE;

Same comment as previous patch.

Comment 5

[Nicholas Nethercote [inactive]]

> Instead of JS:: prefixing everything, can you put
>   using JS::Value;
> et al after the 'namespace js {' above?

I didn't do that because I thought we had a rule against using |using| in header files.  But maybe that just relates to |using namespace|, rather than |using <name>|?  And the fact that it's inside the |namespace js| makes it less objectionable than being at the top-level.

Comment 6

[Luke Wagner [:luke]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
Yes and agreed.  (Note jsapi.h is already 'using <name>' inside namespace js for all the popular JS:: names.  This replaced 'namespace js { using namespace JS; }'.)

Comment 7

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73bad4a03382
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7f48284d66a

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/73bad4a03382
https://hg.mozilla.org/mozilla-central/rev/d7f48284d66a