Last Comment Bug 773734 - Have a separate preference to only enable the gc/cc notifications
: Have a separate preference to only enable the gc/cc notifications
Status: VERIFIED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Terrence Cole [:terrence]
:
Mentors:
: 752861 (view as bug list)
Depends on:
Blocks: 797066
  Show dependency treegraph
 
Reported: 2012-07-13 11:25 PDT by Girish Sharma [:Optimizer]
Modified: 2012-10-04 01:18 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0 (6.80 KB, patch)
2012-07-13 14:06 PDT, Terrence Cole [:terrence]
wmccloskey: review+
Details | Diff | Splinter Review

Description Girish Sharma [:Optimizer] 2012-07-13 11:25:37 PDT
there should be a separate preference, like js.mem.notify which will only send a notification on each GC/CC and not dump into the error console.
Comment 1 Terrence Cole [:terrence] 2012-07-13 14:06:52 PDT
Created attachment 642038 [details] [diff] [review]
v0

I think this is roughly what is needed.  Bill, does this make sense?
Comment 2 Bill McCloskey (:billm) 2012-07-13 14:18:16 PDT
Comment on attachment 642038 [details] [diff] [review]
v0

This looks fine, but I think we should wait to land it until MemChaser has been updated to set the new preference. Can you talk to Henrik about this?
Comment 3 Girish Sharma [:Optimizer] 2012-07-13 22:56:02 PDT
Actually, I would also be releasing an addon, Graphical Timeline of Events, that visually displays the GC/CC. So if this could land in 16, it would be very helpful.
Comment 4 Henrik Skupin (:whimboo) 2012-07-15 10:05:31 PDT
This request has been filed originally as bug 752861. Given that an already reviewed patch is attached here, I will dupe it to this one.
Comment 5 Henrik Skupin (:whimboo) 2012-07-15 10:05:55 PDT
*** Bug 752861 has been marked as a duplicate of this bug. ***
Comment 6 Bill McCloskey (:billm) 2012-07-15 10:18:45 PDT
I talked to Henrik about this. He's going to update MemChaser to set the new preference. A few days after that (so people have time to update), we'll land this patch on trunk and Aurora.
Comment 7 Bill McCloskey (:billm) 2012-07-15 14:18:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/173b3e481297

Optimizer suggested the idea of generating the JSON if mem.log or mem.notify are set. Then MemChaser and other addons can migrate to mem.notify. At that point, we can make mem.log only generate error console output and not JSON. That sounded good, so I made those changes and landed so that this makes it into 16.
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-07-15 18:43:17 PDT
https://hg.mozilla.org/mozilla-central/rev/173b3e481297
Comment 9 Henrik Skupin (:whimboo) 2012-10-02 11:09:24 PDT
Used this pref in memchaser for a while now and everything looks fine for Nightly, Aurora, and Beta. Have we updated MDN for this change?
Comment 10 Girish Sharma [:Optimizer] 2012-10-02 11:11:46 PDT
I can also confirm, it works. There was a plan to completely stop sending notification along with the previous pref, should I file a new bug for that ?, enough time has passed..
Comment 11 Terrence Cole [:terrence] 2012-10-02 11:55:52 PDT
(In reply to Girish Sharma [:Optimizer] from comment #10)
> I can also confirm, it works. There was a plan to completely stop sending
> notification along with the previous pref, should I file a new bug for that
> ?, enough time has passed..

Please do!  We might as well get it into this release if you and Henrik are both ready.
Comment 12 Henrik Skupin (:whimboo) 2012-10-02 14:04:42 PDT
(In reply to Girish Sharma [:Optimizer] from comment #10)
> I can also confirm, it works. There was a plan to completely stop sending
> notification along with the previous pref, should I file a new bug for that
> ?, enough time has passed..

Is this really a good idea? I mean without this pref it would not be possible anymore for normal users to get the stats logged to the Error console. There are plenty of situations when such type of usage is necessary. Without it there would be no easy way anymore and you would be forced to install an extension which connects to the API. Personally I would not recommend to remove the preference.
Comment 13 Girish Sharma [:Optimizer] 2012-10-02 14:07:02 PDT
(In reply to Henrik Skupin (:whimboo) from comment #12)
> Is this really a good idea? I mean without this pref it would not be
> possible anymore for normal users to get the stats logged to the Error
> console. There are plenty of situations when such type of usage is
> necessary. Without it there would be no easy way anymore and you would be
> forced to install an extension which connects to the API. Personally I would
> not recommend to remove the preference.

there is javascript.options.mem.log for logging. Its just that it should *only* log and not send notification also, as for notification there is javascript.options.mem.notify

so to an end user, the difference will be zero.
Comment 14 Henrik Skupin (:whimboo) 2012-10-04 00:27:08 PDT
Ok, re-read comment 6 and you are right. So yes, please file a bug to get the sending of JSON data removed when mem.log has been set. Sorry for mixing it up.
Comment 15 Girish Sharma [:Optimizer] 2012-10-04 01:18:03 PDT
Did that. Fixed that. Already landed.
See bug 797066
;)

Note You need to log in before you can comment on or make changes to this bug.