Closed
Bug 1033916
Opened 10 years ago
Closed 6 years ago
Move AutoByteString to a separate header
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: Waldo)
References
Details
Attachments
(1 file)
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
Per review comments in bug 966452. When this is done, we can stop including jsapi.h in jsfriendapi.h.
Comment 1•10 years ago
|
||
> When this is done, we can stop including jsapi.h in jsfriendapi.h.
jsfriendapi.h already doesn't include jsapi.h.
Reporter | ||
Comment 2•10 years ago
|
||
> jsfriendapi.h already doesn't include jsapi.h.
It will once the patch for bug 966452 is checked in.
Assignee | ||
Comment 3•6 years ago
|
||
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 | ||
Updated•6 years ago
|
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Comment 4•6 years ago
|
||
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
Comment 7•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 8•6 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•