528792 - :-moz-lwtheme incorrectly depends on the lwthemetextcolor attribute

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Dão Gottwald [:dao]]

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.

Comment 1

[Nochum Sossonko [:Natch]]

taking. Does this have to block 1.9.2 or is this for future updates?

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Whoops - sorry, I missed comment 1. You get to flay me if you wish :)

Comment 4

[Nochum Sossonko [:Natch]]

No prob (although I'll take you up on the flaying) ;)

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 7

[Dão Gottwald [:dao]]

(In reply to comment #6)
> (I thought the original idea was that the lwthemetextcolor attribute was
> required,

No, that must have been a misunderstanding.

Comment 8

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/1b13998cbe17

Comment 9

[(no longer active)]

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.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 11

[Johnathan Nightingale [:johnath]]

Comment on attachment 412516 [details] [diff] [review]
Patch v1

Approved fragility fix.

Comment 12

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ea95dc06e291