849273 - Investigate splitting the js and JS namespaces

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

At the moment, the JS namespace is included in the js namespace by a using declaration in js/public/Utility.h.

For bug 781070 we want to have separate js::NullPtr and JS::NullPtr classes, so this bug is to investigate removing the inclusion.

Comment 1

[Luke Wagner [:luke]]

Splitting the namespaces sounds fine to me on its face and it would be more consistent with what we do for namespace mozilla.  I see two (solvable) problems though:
 - internal headers would have to explicitly qualify super-common names like Value
 - we couldn't put 'using namespace JS;' at the top of our .cpps, like we do with 'js' and 'mozilla', since this would cause a naming ambiguity between js::NullPtr and JS::NullPtr.  That means *all* code would have to explicitly qualify names like Value.

There are a couple of options, but the best I can see atm is to:
 1. never have 'using namespace JS;' anywhere
 2. in some central *internal* header we explicitly import all the JS:: names that we want to make unqualified reference to via 'using JS::Value;'.

IIUC, that'll give us short names internally and no ambiguity for NullPtr.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: 1 - Change js:: to JS:: for things that are in JS namespace

Comment 3

[Jon Coppeard (:jonco)]

Attachment: 2 - Use RootedThing where possible instead of Rooted<Thing *>

Comment 4

[Jon Coppeard (:jonco)]

Attachment: 3 - Remove inclusion of JS in js namspace

This adds JS:: prefixes where necessary in header files, and adds "using namespace JS" in .cpp files.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: 4 - Remove temporary aliases to Value/Rooted/Handles from js namespace and fix references

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Luke Wagner [:luke] from comment #1)

Oh hey, I just posted my work in progress patches for this before seeing your comment.  I prefer the idea of having a bunch of names we import into js::, that would be much less cumbersome.

Comment 7

[Luke Wagner [:luke]]

(In reply to Jon Coppeard (:jonco) from comment #6)
Great!  IIUC, if we import all the non-ambiguous names, the only js::/JS:: qualification we'd have to add would be for NullPtr in headers.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: Split namespaces but import frequently used JS:: names into js::

Simpler approach as per Luke's suggestion.

This splits the namespaces but explicitly imports frequently used names back into js::.  jsapi.h turned out to be the most convenient place to do this, since it needs to happen after declaration but before use.

Comment 9

[Terrence Cole [:terrence]]

Comment on attachment 726258 [details] [diff] [review]
Split namespaces but import frequently used JS:: names into js::

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

Excellent. The external changes appear to be actual fixes for js:: usage that should never have slipped outside the engine. The internal changes also seem reasonable.

::: js/public/Utility.h
@@ +30,5 @@
>  /* The mozilla-shared reusable template/utility namespace. */
>  namespace mozilla {}
>  
>  /* The private JS engine namespace. */
> +namespace js {}

\o/

::: js/src/jsapi.cpp
@@ +6261,5 @@
>      AssertHeapIsIdle(cx);
>      CHECK_REQUEST(cx);
>  
>      RootedValue reviver(cx, reviverArg), value(cx);
> +    if (!ParseJSONWithReviver(cx, JS::StableCharPtr(chars, len), len, reviver, &value))

StableCharPtr and RootedValue are both imported into js:: in jsapi.h. Why are the StableCharPtr changes required here?

Comment 10

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #9)
> Comment on attachment 726258 [details] [diff] [review]

> StableCharPtr and RootedValue are both imported into js:: in jsapi.h. Why
> are the StableCharPtr changes required here?

They're not.  Fixed.

Comment 11

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa78767a3e78

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fa78767a3e78