Bug 13842 - Simplify Color::light() and Color::dark()
Summary: Simplify Color::light() and Color::dark()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P3 Trivial
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-05-23 07:10 PDT by mitz
Modified: 2007-05-26 07:00 PDT (History)
0 users

See Also:


Attachments
Simpler implementation (4.28 KB, patch)
2007-05-23 07:11 PDT, mitz
no flags Details | Formatted Diff | Diff
Simpler implementation (4.40 KB, patch)
2007-05-23 07:40 PDT, mitz
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2007-05-23 07:10:16 PDT
The RGB->HSV->RGB conversion Color::light() and Color::dark() do is an overkill. I don't think those functions are hot but what they do can be described and implemented in simpler terms.
Comment 1 mitz 2007-05-23 07:11:25 PDT
Created attachment 14683 [details]
Simpler implementation
Comment 2 mitz 2007-05-23 07:40:01 PDT
Created attachment 14684 [details]
Simpler implementation
Comment 3 Darin Adler 2007-05-23 07:57:12 PDT
Comment on attachment 14684 [details]
Simpler implementation

Does this really give the same values as before? If so, seems fine. But I was under the impression that scaling value didn't affect the 3 channels equally.

I don't think dark() properly handles the case where all three of r, g, and b are zero. So review- because of that. Otherwise, would be r=me.
Comment 4 mitz 2007-05-23 09:09:22 PDT
Comment on attachment 14684 [details]
Simpler implementation

(In reply to comment #3)
> (From update of attachment 14684 [details] [edit])
> Does this really give the same values as before? If so, seems fine. But I was
> under the impression that scaling value didn't affect the 3 channels equally.

If you look at convertHSVToRGB() you'll see that its result can be expressed as "v times some vector x whose components do not depend on v", so indeed for given h and s components, scaling v affects the 3 channels equally. (IIRC in the HSL space, L doesn't affect all channels equally, since L=1.0 maps to white).

> I don't think dark() properly handles the case where all three of r, g, and b
> are zero.

I verified that in that case (v - 0.33f) / v is -inf and therefore the multiplier is 0, so the result is <0,0,0,alpha>, which is what I would expect.
Comment 5 Darin Adler 2007-05-23 09:12:22 PDT
Comment on attachment 14684 [details]
Simpler implementation

r=me
Comment 6 Mark Rowe (bdash) 2007-05-26 07:00:59 PDT
Landed in r21798.