1033916 - Move AutoByteString to a separate header

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Per review comments in bug 966452.  When this is done, we can stop including jsapi.h in jsfriendapi.h.

Comment 1

[Nicholas Nethercote [inactive]]

> When this is done, we can stop including jsapi.h in jsfriendapi.h.

jsfriendapi.h already doesn't include jsapi.h.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> jsfriendapi.h already doesn't include jsapi.h.

It will once the patch for bug 966452 is checked in.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Patch

Whoops, thought I'd posted this earlier, guess not.  (As if it matters, the six-hour difference in posting time.  :-) )

I feel like I said this somewhere (maybe in a lost last_bzexport.txt), but assuming you didn't see it:

There's an "auto" prefix on the file name for this because "ByteString.h" is misleading.  I don't like an auto prefix for this stuff, and that's why there isn't one on StableStringChars.h.  But here, a little bad taste is actually good, IMO.

JS_EncodeString* living here is necessary for JSAutoByteString to be defined.  For lack of better place to put them, they go in this new header.

Since all of this is bad API, it is all noted to be deprecated.  Perhaps it all should be in js/public/deprecated/AutoByteString.h to make that clear, and to discourage use, but I didn't want to do the extra work here, just wanted to fix the bug.

It's dumb to have both JS_free and js_free used here, not least with them living in different headers, and also I'm not even quite sure that's exactly *right*, maybe.  But it's what we do now, so eh.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 9002179 [details] [diff] [review]
Patch

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

::: dom/base/nsJSUtils.h
@@ +226,4 @@
>  inline bool
>  AssignJSString(JSContext *cx, T &dest, JSString *s)
>  {
> +  size_t len = JS::GetStringLength(s);

This belongs in the patch for bug 1040316?

::: dom/bindings/BindingUtils.cpp
@@ +2876,4 @@
>        return false;
>      }
>    } else {
> +    length = JS::GetStringLength(s);

Same here.

::: js/src/jsfriendapi.h
@@ -13,4 @@
>  #include "mozilla/MemoryReporting.h"
>  #include "mozilla/UniquePtr.h"
>  
> -#include "jsapi.h" // For JSAutoByteString.  See bug 1033916.

Coolio.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67d5039dcbc2
Move JSAutoByteString out of jsapi.h into js/public/AutoByteString.h, incidentally breaking the jsfriendapi.h -> jsapi.h dependency.  r=jandem

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b56e88c1b06d
followup - Fix SpiderMonkey Rust bindings by including jsapi.h in wrapper.hpp instead of relying on jsfriendapi.h doing it. r=me

Comment 7

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/67d5039dcbc2

Comment 8

[Dorel Luca [:dluca]]

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