Closed
Bug 1310971
Opened 8 years ago
Closed 7 years ago
HandleValueArray constructor should take a HandleValue rather than a RootedValue&
Categories
(Core :: JavaScript: GC, defect, P3)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: jonco, Assigned: rofael)
Details
(Keywords: good-first-bug, triage-deferred)
Attachments
(1 file, 3 obsolete files)
1.12 KB,
patch
|
Details | Diff | Splinter Review |
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.
Updated•7 years ago
|
Keywords: triage-deferred
Priority: -- → P3
Assignee | ||
Comment 1•7 years ago
|
||
Would this be a worthwhile change? If so, could I be assigned to it?
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8918614 -
Flags: review?(jcoppeard)
Reporter | ||
Comment 3•7 years 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•7 years ago
|
||
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•7 years 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•7 years 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•7 years 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•7 years ago
|
Attachment #8919537 -
Flags: checkin?
Assignee | ||
Updated•7 years ago
|
Attachment #8919537 -
Flags: checkin?
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 8•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
Attachment #8919537 -
Attachment is obsolete: true
Comment 14•7 years 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
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/99f3aa7d125f
Status: ASSIGNED → RESOLVED
Closed: 7 years 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.
Description
•