Closed Bug 528792 Opened 10 years ago Closed 10 years ago

:-moz-lwtheme incorrectly depends on the lwthemetextcolor attribute

Categories

(Core :: CSS Parsing and Computation, defect)

1.9.2 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta4-fixed

People

(Reporter: dao, Assigned: Unfocused)

References

Details

Attachments

(1 file)

For bug 519486, we're considering not setting the lwthemetextcolor attribute if we can't decide whether a color should be considered bright or dark. But apparently GetDocumentLWTheme from <http://mxr.mozilla.org/mozilla-central/source/content/xul/document/src/nsXULDocument.cpp#4763> makes :-moz-lwtheme depend on the lwthemetextcolor attribute. It should only depend on the lwtheme attribute, as described in bug 513461 comment 0.
taking. Does this have to block 1.9.2 or is this for future updates?
Assignee: nobody → highmind63
Attached patch Patch v1Splinter Review
Assignee: highmind63 → bmcbride
Status: NEW → ASSIGNED
Attachment #412516 - Flags: review?(dbaron)
Whoops - sorry, I missed comment 1. You get to flay me if you wish :)
No prob (although I'll take you up on the flaying) ;)
This *might* block 1.9.2 - depends on if we end up needing it for bug 519486 (which I'm working on right now). Either way, it'd be great to have on 1.9.2 and is a low-risk fix.
Comment on attachment 412516 [details] [diff] [review]
Patch v1

r=dbaron

(I thought the original idea was that the lwthemetextcolor attribute was required, but I guess that's not true anymore.)
Attachment #412516 - Flags: review?(dbaron) → review+
(In reply to comment #6)
> (I thought the original idea was that the lwthemetextcolor attribute was
> required,

No, that must have been a misunderstanding.
http://hg.mozilla.org/mozilla-central/rev/1b13998cbe17
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Version: Trunk → 1.9.2 Branch
Attachment #412516 - Flags: approval1.9.2?
This may have caused a 0.31% Tp4 regression on Linux:

<http://graphs.mozilla.org/graph.html#tests=[{"machine":188,"test":38,"branch":1},{"machine":189,"test":38,"branch":1},{"machine":190,"test":38,"branch":1},{"machine":191,"test":38,"branch":1},{"machine":192,"test":38,"branch":1},{"machine":193,"test":38,"branch":1},{"machine":194,"test":38,"branch":1},{"machine":195,"test":38,"branch":1},{"machine":196,"test":38,"branch":1},{"machine":197,"test":38,"branch":1},{"machine":198,"test":38,"branch":1},{"machine":199,"test":38,"branch":1},{"machine":200,"test":38,"branch":1},{"machine":201,"test":38,"branch":1},{"machine":202,"test":38,"branch":1},{"machine":203,"test":38,"branch":1},{"machine":204,"test":38,"branch":1},{"machine":205,"test":38,"branch":1}]&amp;sel=1258277040,1258449840>

The graph looks noisy, so this might not be a real regression after all.
(In reply to comment #9)
> This may have caused a 0.31% Tp4 regression on Linux:
> 
> <http://graphs.mozilla.org/graph.html#tests=[{"machine":188,"test":38,"branch":1},{"machine":189,"test":38,"branch":1},{"machine":190,"test":38,"branch":1},{"machine":191,"test":38,"branch":1},{"machine":192,"test":38,"branch":1},{"machine":193,"test":38,"branch":1},{"machine":194,"test":38,"branch":1},{"machine":195,"test":38,"branch":1},{"machine":196,"test":38,"branch":1},{"machine":197,"test":38,"branch":1},{"machine":198,"test":38,"branch":1},{"machine":199,"test":38,"branch":1},{"machine":200,"test":38,"branch":1},{"machine":201,"test":38,"branch":1},{"machine":202,"test":38,"branch":1},{"machine":203,"test":38,"branch":1},{"machine":204,"test":38,"branch":1},{"machine":205,"test":38,"branch":1}]&amp;sel=1258277040,1258449840>
> 
> The graph looks noisy, so this might not be a real regression after all.

Yea, I'd put it down to noise (or something else) - the only real code change here is a single enum assignment.
Blocks: 529129
Comment on attachment 412516 [details] [diff] [review]
Patch v1

Approved fragility fix.
Attachment #412516 - Flags: approval1.9.2? → approval1.9.2+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.