Closed Bug 967031 Opened 6 years ago Closed 4 years ago

Rename DumpHeapComplete to DumpHeap

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: mccr8, Assigned: yanisellami)

Details

Attachments

(1 file, 1 obsolete file)

This will require renaming DumpHeap to something else, maybe DumpPartialHeap or something.
I would like to work on this bug
Attached patch 967031.diff (obsolete) — Splinter Review
patch renaming DumpHeapComplete
Attachment #8608687 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8608687 [details] [diff] [review]
967031.diff

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

This sounds good to me, but you might need a GC person for this review.
Attachment #8608687 - Flags: review?(nicolas.b.pierron) → review?(terrence)
Comment on attachment 8608687 [details] [diff] [review]
967031.diff

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

Great! Thanks for the patch!

::: js/src/builtin/TestingFunctions.cpp
@@ -1181,4 @@
>  }
>  
>  static bool
> -DumpHeapComplete(JSContext* cx, unsigned argc, jsval* vp)

s/jsval/Value/ while you are here.
Attachment #8608687 - Flags: review?(terrence) → review+
Should I replace all jsval in js/src/builtin/TestingFunctions.cpp or just this one?
Attachment #8608687 - Attachment is obsolete: true
Attachment #8609277 - Flags: review?(terrence)
(In reply to yanisellami from comment #5)
> Should I replace all jsval in js/src/builtin/TestingFunctions.cpp or just
> this one?

Terrence probably means just that one, because you are modifying the line anyways. (That is what is meant by the "while you are here" part.)
Comment on attachment 8609277 [details] [diff] [review]
patch with one jsval replacement

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

Sorry for the confusion! Andrew is correct that I meant to fix the one instance on the same line that you were touching. That said, fixing up the rest of the jsval in TestingFunctions would be an excellent next step! It should be done in a separate bug and with a separate patch, however. Have you filed a bug in bugzilla before?

::: js/src/builtin/TestingFunctions.cpp
@@ +1181,4 @@
>  }
>  
>  static bool
> +DumpHeap(JSContext* cx, unsigned argc, Value* vp)

Exactly right.
Attachment #8609277 - Flags: review?(terrence) → review+
(In reply to Terrence Cole [:terrence] from comment #7)

> Have you filed a bug in bugzilla before?

No I don't.
(In reply to yanisellami from comment #8)
> (In reply to Terrence Cole [:terrence] from comment #7)
> 
> > Have you filed a bug in bugzilla before?
> 
> No I don't.

It's not nearly as intimidating as it looks. There are lots of fields, but all you really need is to ensure that you file as Product "Core" and Component "Javascript" with a reasonable Summary and Description. You can safely ignore all the other fields.
(In reply to Terrence Cole [:terrence] from comment #9)
> 
> It's not nearly as intimidating as it looks. There are lots of fields, but
> all you really need is to ensure that you file as Product "Core" and
> Component "Javascript" with a reasonable Summary and Description. You can
> safely ignore all the other fields.

Ok, I'll do that. The previous patch should be ok for this bug.
https://hg.mozilla.org/mozilla-central/rev/c64cfd067b1e
Assignee: nobody → yanisellami
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.