Debug Preferences should handle nglayout.debug.paint_flashing_chrome

RESOLVED FIXED in seamonkey2.24

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: philip.chee, Assigned: ewong)

Tracking

Trunk
seamonkey2.24

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

5 years ago
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)
(Assignee)

Comment 1

5 years ago
Created attachment 784789 [details] [diff] [review]
Add nglayout.debug.paint_flashing_chrome pref. (v1)
Attachment #784789 - Flags: review?(iann_bugzilla)
(Reporter)

Comment 2

5 years ago
>        <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
(Assignee)

Updated

5 years ago
Attachment #784789 - Flags: review?(iann_bugzilla)
Right - that is the desired behavior.  paint_flashing everywhere, paint_flashing_chrome just on chrome.
(Assignee)

Comment 4

5 years ago
Created attachment 785325 [details] [diff] [review]
Add nglayout.debug.paint_flashing_chrome pref. (v2)
Attachment #784789 - Attachment is obsolete: true
Attachment #785325 - Flags: feedback?(philip.chee)
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
Created attachment 785327 [details] [diff] [review]
Add nglayout.debug.paint_flashing_chrome pref. (v3)

w/ pref-debug1.js
Attachment #785325 - Attachment is obsolete: true
Attachment #785325 - Flags: feedback?(philip.chee)
Attachment #785327 - Flags: feedback?(philip.chee)
(Reporter)

Comment 7

5 years ago
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"/>
Attachment #785327 - Flags: feedback?(philip.chee)
(Assignee)

Comment 8

5 years ago
Created attachment 789309 [details] [diff] [review]
Add nglayout.debug.paint_flashing_chrome pref. (v4)
Attachment #785327 - Attachment is obsolete: true
Attachment #789309 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 9

5 years ago
Created attachment 789557 [details] [diff] [review]
Add nglayout.debug.paint_flashing_chrome pref. (v5)
Attachment #789309 - Attachment is obsolete: true
Attachment #789309 - Flags: review?(iann_bugzilla)
Attachment #789557 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 10

5 years ago
Created attachment 789558 [details] [diff] [review]
Add nglayout.debug.paint_flashing_chrome pref. (v5)

Forgot to do a qrefresh.
Attachment #789557 - Attachment is obsolete: true
Attachment #789557 - Flags: review?(iann_bugzilla)
Attachment #789558 - Flags: review?(iann_bugzilla)
(Reporter)

Comment 11

5 years ago
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);
(Assignee)

Comment 12

5 years ago
Created attachment 791143 [details] [diff] [review]
Add nglayout.debug.paint_flashing_chrome pref. (v6)
Attachment #789558 - Attachment is obsolete: true
Attachment #789558 - Flags: review?(iann_bugzilla)
Attachment #791143 - Flags: review?(iann_bugzilla)
(Reporter)

Comment 13

5 years ago
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);
(Assignee)

Comment 14

5 years ago
Created attachment 791617 [details] [diff] [review]
Add nglayout.debug.paint_flashing_chrome pref. (v7)
Attachment #791143 - Attachment is obsolete: true
Attachment #791143 - Flags: review?(iann_bugzilla)
Attachment #791617 - Flags: review?(iann_bugzilla)
(Reporter)

Comment 15

5 years ago
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

5 years ago
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
Attachment #791617 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 17

5 years ago
Created attachment 813437 [details] [diff] [review]
Add nglayout.debug.paint_flashing_chrome pref. (v8) r=IanN
Attachment #791617 - Attachment is obsolete: true
Attachment #813437 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Reporter)

Updated

5 years ago
Attachment #813437 - Attachment description: Add nglayout.debug.paint_flashing_chrome pref. (v8) → Add nglayout.debug.paint_flashing_chrome pref. (v8) r=IanN
https://hg.mozilla.org/comm-central/rev/666c0c26a02b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.24
You need to log in before you can comment on or make changes to this bug.