884750 - Persistent logs options should apply instantly

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P3)

Description

[Girish Sharma [:Optimizer]]

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]

Attachment: 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]]

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]

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

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

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]

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

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

s/god/good

Comment 9

[tchatha]

I'm new to programming, and I want to help contribute to opensource projects. How can I help?

Comment 10

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/708a370466ff

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/708a370466ff