Opened 12 years ago

Closed 12 years ago

#3667 closed defect (fixed)

dojo.Color code could be shorter

Reported by: Adam Peller Owned by: Adam Peller
Priority: high Milestone: 0.9
Component: General Version: 0.9
Keywords: Cc:
Blocked By: Blocking:

Description

machines prefer binary arithmetic. 20x faster on FF and we save bytes in _base

Attachments (1)

color.patch (2.6 KB) - added by Adam Peller 12 years ago.
Replace string processing in hex2rgb, remove complex signature in rgb2hex, add inline docs.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 12 years ago by Eugene Lazutkin

Much better! Two small comments:

1) We can replace this code (line 112) in the patch:

x += x * 16;

with this:

x *= 17;

I think it will be even faster than the old school trick:

x += (x << 4);

And it is slightly shorter.

2) Another thing I noticed that dojo.rgb2hex() signature is changed. Is it OK?

Changed 12 years ago by Adam Peller

Attachment: color.patch added

Replace string processing in hex2rgb, remove complex signature in rgb2hex, add inline docs.

comment:2 Changed 12 years ago by Adam Peller

Thanks, Eugene. Patch updated. So I removed the int, int, int signature overlay because it wasn't used and seemed to conflict with 0.9 design goals. So right now, we support array plus the variety of string formats that CSS accepts. As it is, that's a lot. It might be nice to do the same for the dojo.Color constructor. Unless someone speaks up, I'll consider this reviewed and check it in :)

comment:3 Changed 12 years ago by Eugene Lazutkin

Ask Alex if he's okay with changes. I see that it is possible to improve/simplify some other stuff in dojo.Color, but it will be easier to do after your patch is committed.

comment:4 Changed 12 years ago by Adam Peller

Resolution: fixed
Status: newclosed

(In [9564]) More efficient color conversion, plus inline docs. Fixes #3667

comment:5 Changed 12 years ago by bill

Resolution: fixed
Status: closedreopened

comment:6 Changed 12 years ago by bill

Owner: changed from alex to Adam Peller
Status: reopenednew

Can you take a look at this checkin again? It breaks my recently checked in util/test_focusWidget (the border colors don't change anymore). Or if there's something wrong w/my test let me know.

comment:7 Changed 12 years ago by Adam Peller

Resolution: fixed
Status: newclosed

(In [9586]) Fixed rgb2hex so that it returns 3 octets, not 4. But I still don't think animateProperty is working right for colors. Fixes #3667

Note: See TracTickets for help on using tickets.