Last Comment Bug 884750 - Persistent logs options should apply instantly
: Persistent logs options should apply instantly
Status: RESOLVED FIXED
[good first bug][lang=js][mentor=msucan]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: Trunk
: All All
: P3 enhancement (vote)
: Firefox 25
Assigned To: dorian jaminais
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-19 03:45 PDT by Girish Sharma [:Optimizer]
Modified: 2013-06-26 09:59 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
apply the log persistance immediately (1.25 KB, patch)
2013-06-23 09:11 PDT, dorian jaminais
paul: review+
Details | Diff | Review

Description Girish Sharma [:Optimizer] 2013-06-19 03:45:22 PDT
Right now, when you change the "Enable persistent logs" options from options panel, it does not get applied until toolbox is reopened. This can be made instant if someone is listening to "pref-changed" event on gDevTools and acting accordingly.

See http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/theme-switching.js#70
Comment 1 dorian jaminais 2013-06-23 09:11:23 PDT
Created attachment 766453 [details] [diff] [review]
apply the log persistance immediately

This patch fixes the problem.

I did not find where and how to unit test this. Can you provide me some guidance on how to do this ?
Comment 2 Girish Sharma [:Optimizer] 2013-06-23 09:20:47 PDT
Comment on attachment 766453 [details] [diff] [review]
apply the log persistance immediately

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

Hi Dorian, thanks for taking this bug. The patch does what is intended, but things can be optimized here :)

Instead of querying the prefs each time, why not convert the getter to a normal property and update its value whenever the value gets changed via options panel.

That can be done via the event listener gDevTools.on("pref-changed", callback) and changing the value of _persistLog.

Take a look at hos its done at one place already : http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/theme-switching.js#56
Comment 3 dorian jaminais 2013-06-23 09:49:51 PDT
I did it that way because the property is only used in this method. Yet you are right that this can be optimised because the method is called each time a tab is refreshed.

I'll update my patch as soon as possible and re-submit.

Thanks
Comment 4 Paul Rouget [:paul] 2013-06-23 10:43:53 PDT
Comment on attachment 766453 [details] [diff] [review]
apply the log persistance immediately

There's no point of caching this pref. It's only accessed on page reload. Let's keep it simple.
Comment 5 Paul Rouget [:paul] 2013-06-23 10:47:12 PDT
Dorian, once this is green: https://tbpl.mozilla.org/?tree=Try&rev=c32ab4b1c7cd add [land-in-fx-team] in the whiteboard, and update your patch to reflect the review (r=paul - and BTW, it's r=, not ref=).
Comment 6 dorian jaminais 2013-06-24 11:14:24 PDT
I have an orange on Win7 opt (bc).
Unfortunately I don't understand how the log relate to my changes. Have I break something ?
Comment 7 Girish Sharma [:Optimizer] 2013-06-24 11:18:44 PDT
(In reply to dorian jaminais from comment #6)
> I have an orange on Win7 opt (bc).
> Unfortunately I don't understand how the log relate to my changes. Have I
> break something ?

Nope it is not related. All god to go here.
Comment 8 Girish Sharma [:Optimizer] 2013-06-24 11:19:28 PDT
s/god/good
Comment 9 tchatha 2013-06-24 19:16:44 PDT
I'm new to programming, and I want to help contribute to opensource projects. How can I help?
Comment 10 Rob Campbell [:rc] (:robcee) 2013-06-25 07:37:20 PDT
https://hg.mozilla.org/integration/fx-team/rev/708a370466ff
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-06-26 09:59:50 PDT
https://hg.mozilla.org/mozilla-central/rev/708a370466ff

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