909178 - Make jsclass.h not depend on jsapi.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Making jsclass.h not depend on jsapi.h is critical for reducing the amount of Gecko code that depends on jsapi.h.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Move |jsid| from jsapi.h into js/Id.h.

This is a necessary precursor to the next patch, and a whole lot that will
follow in other bugs.

The need for IdForward.h is explained in a comment.  Annoying, but I don't see
a better way to do it.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Make jsclass.h not depend on jsapi.h, and rename it js/Class.h.

This patch moves a bunch of class-related stuff from jsapi.h to jsclass.h, and
renames jsclass.h as js/Class.h.

This allows jsapi.h to now include jsclass.h, rather than the other way around.
This is critical for reducing the number of Gecko files that depend on jsapi.h,
because heaps of Gecko code (esp. DOMJSClass.h) needs class-related stuff but
not other jsapi.h stuff.  Also, it's better to have a big header depend on a
small header than the other way around.

It also makes a lot of sense -- it was weird having half of the class-related
stuff in jsapi.h and half in jsclass.h.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 795241 [details] [diff] [review]
(part 1) - Move |jsid| from jsapi.h into js/Id.h.

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

::: js/src/jsapi.cpp
@@ -125,5 @@
>  
> -#ifdef JS_USE_JSID_STRUCT_TYPES
> -const jsid JSID_VOID  = { size_t(JSID_TYPE_VOID) };
> -const jsid JSID_EMPTY = { size_t(JSID_TYPE_OBJECT) };
> -#endif

I'd kinda rather just leave it here rather than make a whole new Id.cpp, unless later patches add to Id.cpp?

Comment 4

[Nicholas Nethercote [inactive]]

> I'd kinda rather just leave it here rather than make a whole new Id.cpp,
> unless later patches add to Id.cpp?

They don't.  But I like having .cpp files that correspond to js/*.h files, because they provide a place where the .h file is included before any others, and so guarantees that the .h file is self-sufficient.  See bug 905507.

Comment 5

[Nicholas Nethercote [inactive]]

Also, imagine a glorious future in which jsapi.h has been entirely broken up into smaller headers.  Will jsapi.cpp still remain to hold the code that we didn't want to bother creating separate .cpp files for?

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 795243 [details] [diff] [review]
(part 2) - Make jsclass.h not depend on jsapi.h, and rename it js/Class.h.

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

I didn't look too closely at the contents of js/public/Class.h after this, to see if what was in jsclass.h that wasn't in context (or wasn't touched) should really sensibly be there.  Ditto, in reverse somewhat, for jsapi.h.  Probably it's mostly reasonable as-is, didn't seem worth fine-toothed-combing it.

::: js/public/Class.h
@@ +3,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #ifndef jsclass_h

Please add an overview comment

/* JSClass definition and its component types, plus related interfaces. */

between license block and include-guard.  That'll show up on http://mxr.mozilla.org/mozilla-central/source/js/public/ and help people reading our code understand where things are, slightly.

@@ +267,5 @@
> +typedef void
> +(* JSFinalizeOp)(JSFreeOp *fop, JSObject *obj);
> +
> +// Finalizes external strings created by JS_NewExternalString.
> +typedef struct JSStringFinalizer JSStringFinalizer;

Remove the typedef, we're C++ now.

Comment 7

[Jeff Walden [:Waldo]]

For what it's worth, Id.cpp for those library externs seems fine to me.

Comment 8

[Luke Wagner [:luke]]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> But I like having .cpp files that correspond to js/*.h files,
> because they provide a place where the .h file is included before any
> others, and so guarantees that the .h file is self-sufficient.

Oh, I forgot about that; that's a good reason.

Comment 9

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f8e99aec954
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8fec7c369d1

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7f8e99aec954
https://hg.mozilla.org/mozilla-central/rev/f8fec7c369d1