Closed Bug 910109 Opened 7 years ago Closed 7 years ago

Make jsproxy.h and jswrapper.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

(2 files)

Like jsfriendapi.h and jsdbgapi.h, jsproxy.h and jswrapper.h need 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.
Attachment #796500 - Flags: review?(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.
Attachment #796500 - Flags: review?(luke) → review+
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.
Attachment #796502 - Flags: review?(luke) → review+
> 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.
(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; }'.)
https://hg.mozilla.org/mozilla-central/rev/73bad4a03382
https://hg.mozilla.org/mozilla-central/rev/d7f48284d66a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.