HandleValueArray constructor should take a HandleValue rather than a RootedValue&

RESOLVED FIXED in Firefox 58

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jonco, Assigned: rofael)

Tracking

({good-first-bug, triage-deferred})

unspecified
mozilla58
good-first-bug, triage-deferred
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
The constructor |HandleValueArray::HandleValueArray(const RootedValue& value)| should probably take a HandleValue instead.  This would allow you to construct one from MutableHandleValue and PersistentRootedValue, etc.
Keywords: triage-deferred
Priority: -- → P3
(Assignee)

Comment 1

a year ago
Would this be a worthwhile change? If so, could I be assigned to it?
(Assignee)

Comment 2

a year ago
Posted patch 1310971.patch (obsolete) — Splinter Review
(Assignee)

Updated

a year ago
Attachment #8918614 - Flags: review?(jcoppeard)
(Reporter)

Comment 3

a year ago
Comment on attachment 8918614 [details] [diff] [review]
1310971.patch

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

Thanks for the patch.  Yes this is good thing to do.  Just one comment:

::: js/src/jsapi.h
@@ +492,5 @@
>  
>      HandleValueArray(size_t len, const Value* elements) : length_(len), elements_(elements) {}
>  
>    public:
> +    explicit HandleValueArray(const HandleValue& value) : length_(1), elements_(value.address()) {}

HandleValue can be passed by value because it's really just a pointer.  Can you change this to just take |HandleValue value|?
Attachment #8918614 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 4

a year ago
Posted patch 1310971-2.patch (obsolete) — Splinter Review
Does this look good? I got rid of the const as well.
Attachment #8918614 - Attachment is obsolete: true
Attachment #8919537 - Flags: review?(jcoppeard)
(Reporter)

Comment 5

a year ago
Comment on attachment 8919537 [details] [diff] [review]
1310971-2.patch

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

That looks great.  Thanks for the patch!
Attachment #8919537 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 6

a year ago
Cool! What else do I need to do? And if I'm done, could you point me to any other bugs?(In reply to Jon Coppeard (:jonco) from comment #5)
> Comment on attachment 8919537 [details] [diff] [review]
> 1310971-2.patch
> 
> Review of attachment 8919537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That looks great.  Thanks for the patch!

Cool! What else do I need to do? And if I'm done, could you point me to some other bugs I could work on?
(Reporter)

Comment 7

a year ago
(In reply to Rofael Aleezada from comment #6)
Next you need to edit the bug and add the "checkin-needed" keyword.  Then someone will check in your patch.
Assignee: nobody → me
(Assignee)

Updated

a year ago
Attachment #8919537 - Flags: checkin?
(Assignee)

Updated

a year ago
Attachment #8919537 - Flags: checkin?
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
(Reporter)

Comment 8

a year ago
Oh hey, I forgot that nowadays you need to do a try build before requesting checkin.  Full details are here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Since I'm not sure you have try access, I'll kick one off for you:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7467be02a077c6210acb8e6f081e2f2174bbcbe
(Reporter)

Comment 9

a year ago
And another thing I forgot to say: you should update the commit message on the patch to be like "Bug 123456 - Description r=jonco".  More details here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment
(Assignee)

Comment 10

a year ago
Comment on attachment 8919537 [details] [diff] [review]
1310971-2.patch

>changeset:   386274:963b24ed3290
>tag:         tip
>user:        Rofael Aleezada <me@rofael.com>
>date:        Tue Oct 17 18:57:51 2017 -0500
>summary:     Bug 1310971: Changed |HandleValueArray(const RootedValue& value)| to |HandleValueArray(HandleValue value)| r=jonco
>
>diff --git a/js/src/jsapi.h b/js/src/jsapi.h
>--- a/js/src/jsapi.h
>+++ b/js/src/jsapi.h
>@@ -488,17 +488,17 @@ class MOZ_RAII JS_PUBLIC_API(CustomAutoR
> class HandleValueArray
> {
>     const size_t length_;
>     const Value * const elements_;
> 
>     HandleValueArray(size_t len, const Value* elements) : length_(len), elements_(elements) {}
> 
>   public:
>-    explicit HandleValueArray(const RootedValue& value) : length_(1), elements_(value.address()) {}
>+    explicit HandleValueArray(HandleValue value) : length_(1), elements_(value.address()) {}
> 
>     MOZ_IMPLICIT HandleValueArray(const AutoValueVector& values)
>       : length_(values.length()), elements_(values.begin()) {}
> 
>     template <size_t N>
>     MOZ_IMPLICIT HandleValueArray(const AutoValueArray<N>& values) : length_(N), elements_(values.begin()) {}
> 
>     /** CallArgs must already be rooted somewhere up the stack. */
>
(Assignee)

Comment 11

a year ago
Comment on attachment 8919537 [details] [diff] [review]
1310971-2.patch

>changeset:   386274:963b24ed3290
>tag:         tip
>user:        Rofael Aleezada <me@rofael.com>
>date:        Tue Oct 17 18:57:51 2017 -0500
>summary:     Bug 1310971: Changed |HandleValueArray(const RootedValue& value)| to |HandleValueArray(HandleValue value)| r=jonco
>
>diff --git a/js/src/jsapi.h b/js/src/jsapi.h
>--- a/js/src/jsapi.h
>+++ b/js/src/jsapi.h
>@@ -488,17 +488,17 @@ class MOZ_RAII JS_PUBLIC_API(CustomAutoR
> class HandleValueArray
> {
>     const size_t length_;
>     const Value * const elements_;
> 
>     HandleValueArray(size_t len, const Value* elements) : length_(len), elements_(elements) {}
> 
>   public:
>-    explicit HandleValueArray(const RootedValue& value) : length_(1), elements_(value.address()) {}
>+    explicit HandleValueArray(HandleValue value) : length_(1), elements_(value.address()) {}
> 
>     MOZ_IMPLICIT HandleValueArray(const AutoValueVector& values)
>       : length_(values.length()), elements_(values.begin()) {}
> 
>     template <size_t N>
>     MOZ_IMPLICIT HandleValueArray(const AutoValueArray<N>& values) : length_(N), elements_(values.begin()) {}
> 
>     /** CallArgs must already be rooted somewhere up the stack. */
>
(Assignee)

Comment 12

a year ago
Posted patch 1310971.patch (obsolete) — Splinter Review
Attachment #8919537 - Attachment is obsolete: true
(Assignee)

Comment 13

a year ago
Posted patch 1310971.patchSplinter Review
Formatting fix
Attachment #8919856 - Attachment is obsolete: true

Comment 14

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99f3aa7d125f
Change |HandleValueArray(const RootedValue& value)| to |HandleValueArray(HandleValue value)|. r=jonco
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/99f3aa7d125f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.