Comment on attachment 699945 [details] [diff] [review] Patch v1 Review of attachment 699945 [details] [diff] [review]: ----------------------------------------------------------------- Nicely done. I'd just like a bit more code motion to help statically ensure that ToLocaleHelper and date_format don't make use of args[0]; details inline. ::: js/src/jsdate.cpp @@ +767,5 @@ > * TZD = time zone designator (Z or +hh:mm or -hh:mm or missing for local) > */ > > static JSBool > +date_parseISOString(Handle<JSLinearString*> str, double *result, DateTimeInfo *dtInfo) The ForwardDeclare macros automatically give you HandleLinearString and RootedLinearString. We have reached concensus (sans Waldo) on using the typedefs. @@ +906,5 @@ > #undef NEED_NDIGITS > } > > static JSBool > +date_parseString(Handle<JSLinearString*> str, double *result, DateTimeInfo *dtInfo) Ditto. @@ +1191,4 @@ > if (!str) > + return false; > + > + Rooted<JSLinearString*> linearStr(cx, str->ensureLinear(cx)); Ditto. @@ +2867,5 @@ > JSAutoByteString fmtbytes(cx, fmt); > if (!fmtbytes) > return false; > > return ToLocaleHelper(cx, args, thisObj, fmtbytes.ptr()); Please change ToLocaleHelper and date_format to take MutableHandleValue instead of CallReceiver. They only use the rval so we might as well pass it directly using CallReceiver::rval.

(In reply to :Ms2ger from comment #0) > Created attachment 699945 [details] [diff] [review] > Patch v1 Sweet! Thanks for helping push the rooting along!

(In reply to Terrence Cole [:terrence] from comment #1) > Comment on attachment 699945 [details] [diff] [review] > Patch v1 > > Review of attachment 699945 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nicely done. I'd just like a bit more code motion to help statically ensure > that ToLocaleHelper and date_format don't make use of args[0]; details > inline. > > ::: js/src/jsdate.cpp > @@ +767,5 @@ > > * TZD = time zone designator (Z or +hh:mm or -hh:mm or missing for local) > > */ > > > > static JSBool > > +date_parseISOString(Handle<JSLinearString*> str, double *result, DateTimeInfo *dtInfo) > > The ForwardDeclare macros automatically give you HandleLinearString and > RootedLinearString. We have reached concensus (sans Waldo) on using the > typedefs. That's not actually true; the macro only defined Raw* and Unrooted*. If you tell me where I should add the Handle/Rooted typedefs, I can do that.

This got big enough that I made it a separate patch.

Comment on attachment 700229 [details] [diff] [review] Part b: Stop passing CallReceiver Review of attachment 700229 [details] [diff] [review]: ----------------------------------------------------------------- Yup, that's exactly it.

https://hg.mozilla.org/mozilla-central/rev/2978e786bb2c https://hg.mozilla.org/mozilla-central/rev/80d614cfb1f8