900422 - Debug Preferences should handle nglayout.debug.paint_flashing_chrome

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Philip Chee]

Go Edit->Preferences->Debug->Events

We already have a checkbox for "nglayout.debug.paint_flashing"
"Paint Flashing (Caps-Lock toggles)"
Bug 895003 introduced a new preference for chrome only: "nglayout.debug.paint_flashing_chrome"
I propose that we add this to our UI (pref-debug1.xul/pref-debug1.dtd)

Comment 1

[Edmund Wong (:ewong)]

Attachment: Add nglayout.debug.paint_flashing_chrome pref. (v1)

Comment 2

[Philip Chee]

>        <checkbox id="nglayoutDebugPaintFlashing" label="&debugPaintFlashing.label;"
>                   preference="nglayout.debug.paint_flashing"/>
>+        <checkbox id="nglayoutDebugPaintFlashingChrome"
>+                  label="&debugPaintFlashingChrome.label;"
>+                  preference="nglayout.debug.paint_flashing_chrome"/>

A complication is that the original pref "nglayout.debug.paint_flashing turns on paint flashing for both content and chrome. The new one only turns it on for chrome.

So paint flashing in chrome occurs when either of the preferences are turned on.

See:
http://hg.mozilla.org/mozilla-central/annotate/2ba2c2534fdb/modules/libpref/src/init/all.js#l478

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Right - that is the desired behavior.  paint_flashing everywhere, paint_flashing_chrome just on chrome.

Comment 4

[Edmund Wong (:ewong)]

Attachment: Add nglayout.debug.paint_flashing_chrome pref. (v2)

Comment 5

[Edmund Wong (:ewong)]

Comment on attachment 785325 [details] [diff] [review]
Add nglayout.debug.paint_flashing_chrome pref. (v2)

When clicking on the painting flash, it ticks both the flash and the flash chrome.  I close the preference, and open it again, those preferences aren't ticked.  What loads the preferences to the checkboxes?  I thought by default
when I check it, the preferences are saved, so when I close the panel and open 
it again, the checkboxes are checked(or not checked, depending on what I saved).
Need some feedback on this.  Thanks.

Comment 6

[Edmund Wong (:ewong)]

Attachment: Add nglayout.debug.paint_flashing_chrome pref. (v3)

w/ pref-debug1.js

Comment 7

[Philip Chee]

Comment on attachment 785327 [details] [diff] [review]
Add nglayout.debug.paint_flashing_chrome pref. (v3)

From https://bugzilla.mozilla.org/show_bug.cgi?id=895003#c8
> 
> Introduce a second preference to control the chrome paint flashing - 
> nglayout.debug.paint_flashing_chrome. The original pref still controls
> content and chrome when set. The preference is now live, for content and
> chrome, the change will be seen on the next refresh. The cached value is
> now in nsPresContext, rather than nsRefreshDriver.

Here's my take on nglayout.debug.paint_flashing vs nglayout.debug.paint_flashing_chrome

Chrome will flash if either of the two preferences are set.
Content will only flash if "nglayout.debug.paint_flashing" is set.

So:
If <checkbox id="nglayoutDebugPaintFlashing"> is checked, you should disable
<checkbox id="nglayoutDebugPaintFlashingChrome"> but don't change the checked state itself.

If <checkbox id="nglayoutDebugPaintFlashing"> is not checked, enable the chrome only preference.
................


>        <preference id="nglayout.debug.paint_flashing"
Add an
  onchange="EnableChrome(this.value)"
here to enable/disable the chrome checkbox.

>                    name="nglayout.debug.paint_flashing" type="bool"/>
> +      <preference id="nglayout.debug.paint_flashing_chrome"
> +                  name="nglayout.debug.paint_flashing_chrome" type="bool"/>

Comment 8

[Edmund Wong (:ewong)]

Attachment: Add nglayout.debug.paint_flashing_chrome pref. (v4)

Comment 9

[Edmund Wong (:ewong)]

Attachment: Add nglayout.debug.paint_flashing_chrome pref. (v5)

Comment 10

[Edmund Wong (:ewong)]

Attachment: Add nglayout.debug.paint_flashing_chrome pref. (v5)

Forgot to do a qrefresh.

Comment 11

[Philip Chee]

Comment on attachment 789558 [details] [diff] [review]
Add nglayout.debug.paint_flashing_chrome pref. (v5)

> -                  name="nglayout.debug.paint_flashing" type="bool"/>
> +                  name="nglayout.debug.paint_flashing" type="bool"
> +                  onchange="enableChrome(this.checked);"/>
This should be |this.value| not |this.checked|.

Startup() [Note capitalisation] is still needed to set the initial enabled/disabled state or the chrome paint flashing checkbox. Probably something like:

var val = document.getElementById("nglayout.debug.paint_flashing").value;
enableChrome(val);

Comment 12

[Edmund Wong (:ewong)]

Attachment: Add nglayout.debug.paint_flashing_chrome pref. (v6)

Comment 13

[Philip Chee]

Comment on attachment 791143 [details] [diff] [review]
Add nglayout.debug.paint_flashing_chrome pref. (v6)

> +function StartUp()
Caps still wrong. Only initial character is upper case.
> Startup() [Note capitalisation] is still needed ...

> +  var paintFlashingVal = document.getElementById("nglayoutDebugPaintFlashing").value;
> +  enableChrome(paintFlashingVal);

Earlier I said:
> var val = document.getElementById("nglayout.debug.paint_flashing").value;
> enableChrome(val);

Comment 14

[Edmund Wong (:ewong)]

Attachment: Add nglayout.debug.paint_flashing_chrome pref. (v7)

Comment 15

[Philip Chee]

Comment on attachment 791617 [details] [diff] [review]
Add nglayout.debug.paint_flashing_chrome pref. (v7)

> +            onload="Startup();">
Oops. |Startup| is magical. The SeaMonkey prefwindow.xml will run this at the appropriate time. See:

http://hg.mozilla.org/comm-central/annotate/0b7a62366012/suite/common/bindings/prefwindow.xml#l540

But see what else IanN wants nitted before spinning up a new patch.

Comment 16

[Ian Neal]

Comment on attachment 791617 [details] [diff] [review]
Add nglayout.debug.paint_flashing_chrome pref. (v7)

r=me with the onload removed as per Ratty's comment

Comment 17

[Edmund Wong (:ewong)]

Attachment: Add nglayout.debug.paint_flashing_chrome pref. (v8) r=IanN

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/666c0c26a02b