Closed Bug 885310 Opened 7 years ago Closed 7 years ago

Remove JSHandleXXX typedefs

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jonco, Assigned: jonco)

Details

Attachments

(3 files, 1 obsolete file)

Following on from bug 884371, get rid of JSHandleXXX typedefs, replacing their use with JS::Handle<JS::Whatever> in the browser and JS::HandleWhatever in js source.
Attachment #765321 - Flags: review?(bzbarsky)
Attachment #765322 - Flags: review?(bobbyholley+bmo)
Attachment #765323 - Flags: review?(terrence)
Comment on attachment 765323 [details] [diff] [review]
Remove JSHandleXXX from js/src

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

::: js/src/jsclone.h
@@ +158,5 @@
>      bool writeTransferMap();
>  
>      bool writeString(uint32_t tag, JSString *str);
>      bool writeId(jsid id);
> +    bool writeArrayBuffer(JS::HandleObject obj);

Why JS:: ?

::: js/src/jsprvtd.h
@@ +283,5 @@
>   * A generic type for functions mapping an object to another object, or null
>   * if an error or exception was thrown on cx.
>   */
>  typedef JSObject *
> +(* JSObjectOp)(JSContext *cx, JS::HandleObject obj);

This is kinda like jsapi.h, so I would say JS::Handle<JSObject*>.

::: js/src/perf/jsperf.cpp
@@ +13,5 @@
>  using JS::PerfMeasurement;
>  
>  // You cannot forward-declare a static object in C++, so instead
>  // we have to forward-declare the helper functions that refer to it.
> +static PerfMeasurement* GetPM(JSContext* cx, JS::HandleObject obj, const char* fname);

js:: no?
Attachment #765323 - Flags: review?(terrence) → review+
Attachment #765321 - Attachment is obsolete: true
Attachment #765321 - Flags: review?(bzbarsky)
Attachment #765348 - Flags: review?(bzbarsky)
(In reply to Tom Schuster [:evilpie] from comment #4)

Thanks for the quick review!

> ::: js/src/jsclone.h
> @@ +158,5 @@
> >      bool writeTransferMap();
> >  
> >      bool writeString(uint32_t tag, JSString *str);
> >      bool writeId(jsid id);
> > +    bool writeArrayBuffer(JS::HandleObject obj);
> 
> Why JS:: ?

Well, consistency with the rest of that file.  If you think it's a big deal I can change it though.

> ::: js/src/jsprvtd.h
> This is kinda like jsapi.h, so I would say JS::Handle<JSObject*>.

Agreed, fixed.

> ::: js/src/perf/jsperf.cpp
> > +static PerfMeasurement* GetPM(JSContext* cx, JS::HandleObject obj, const char* fname);
> 
> js:: no?

I'm not sure why we would want to use the js:: namespace here?  HandleObject etc are defined in JS.
Comment on attachment 765322 [details] [diff] [review]
Remove JSHandleXXX use from js directory

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

I didn't look at this very closely, but it seems fine.
Attachment #765322 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 765348 [details] [diff] [review]
Remove JSHandleXXX use from the browser v2

r=me
Attachment #765348 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.