967031 - Rename DumpHeapComplete to DumpHeap

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

This will require renaming DumpHeap to something else, maybe DumpPartialHeap or something.

Comment 1

[yanisellami]

I would like to work on this bug

Comment 2

[yanisellami]

Attachment: 967031.diff

patch renaming DumpHeapComplete

Comment 3

[Nicolas B. Pierron [:nbp]]

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.

Comment 4

[Terrence Cole [: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.

Comment 5

[yanisellami]

Attachment: patch with one jsval replacement

Should I replace all jsval in js/src/builtin/TestingFunctions.cpp or just this one?

Comment 6

[Andrew McCreight [:mccr8]]

(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 7

[Terrence Cole [:terrence]]

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.

Comment 8

[yanisellami]

(In reply to Terrence Cole [:terrence] from comment #7)

> Have you filed a bug in bugzilla before?

No I don't.

Comment 9

[Terrence Cole [:terrence]]

(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.

Comment 10

[yanisellami]

(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.

Comment 11

[Terrence Cole [:terrence]]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c9961fa32c5

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c64cfd067b1e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c64cfd067b1e