Move AutoByteString to a separate header

RESOLVED FIXED in Firefox 63

Status

()

defect
RESOLVED FIXED
5 years ago
11 months ago

People

(Reporter: bzbarsky, Assigned: Waldo)

Tracking

unspecified
mozilla63
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

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

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

It will once the patch for bug 966452 is checked in.
Depends on: 1484385
Depends on: 1484386
Depends on: 1484389
Posted patch PatchSplinter Review
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.
Attachment #9002179 - Flags: review?(jdemooij)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
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.
Attachment #9002179 - Flags: review?(jdemooij) → review+
Pushed by jwalden@mit.edu:
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
Pushed by jandemooij@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/67d5039dcbc2
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.