Closed Bug 1282267 Opened 3 years ago Closed 3 years ago

Window Frame Color detection routine can lead to incorrect results

Categories

(Firefox :: Theme, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox48 --- wontfix
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: mark, Assigned: dao)

Details

Attachments

(1 file)

Specifically, it occurred to me Windows8WindowFrameColor.jsm has some ambiguous checks that can give unpredictable results for specific values.

I'd have linked to mxr but it's down at the moment.

e.g.:
https://hg.mozilla.org/mozilla-central/file/tip/browser/modules/Windows8WindowFrameColor.jsm#l35

If the colorbalance is 0 (all the way desaturated), this check incorrectly assumes it's undefined, and will use the || 78 value instead of the (intended) 0.
Similarly, if the colorizationcolor reg value above would be 0-hex (black + 00 for extra flag) the code would assume the value isn't present in the registry, and give back the hard-coded [158,158,158] for colorization color, and use the wrong FG/text color.

The if(!value) should be rewritten to e.g. if(typeof value === "undefined") for checking nonexisting reg keys.
Priority: -- → P3
Attached patch patchSplinter Review
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Attachment #8768749 - Flags: review?(jaws)
Attachment #8768749 - Flags: review?(jaws) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/e1007823c2e7
Properly handle 0 as a possible registry key value when determining the window frame color. r=jaws
https://hg.mozilla.org/mozilla-central/rev/e1007823c2e7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You're welcome.
Comment on attachment 8768749 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: not a regression
[User impact if declined]: Firefox doesn't integrate as well as it should on Windows 8.1 / 10
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: trivial correctness fix, low risk
[String/UUID change made/needed]: none
Attachment #8768749 - Flags: approval-mozilla-aurora?
(In reply to Mark Straver from comment #4)
> You're welcome.

Belated thank you!
Comment on attachment 8768749 [details] [diff] [review]
patch

Improve windows support, taking it
Attachment #8768749 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.