Bug 89130 - avoid min and max check in Color constructor when needed.
Summary: avoid min and max check in Color constructor when needed.
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-14 15:24 PDT by arno.
Modified: 2012-06-14 16:58 PDT (History)
1 user (show)

See Also:


Attachments
patch proposal (4.76 KB, patch)
2012-06-14 15:35 PDT, arno.
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description arno. 2012-06-14 15:24:30 PDT
Hi,

Color constructor accepts integers arguments and checks that those integers are between 0 and 255.
But sometimes, Color constructor is called with unsigned char, and those checks are therefore unneeded (for example in cairo ImageBuffer::putByteArray)

So, I'm thinking a specialized constructor for Color accepting unsigned char, and not performing min/max checks would improve performance of Color initialization in those cases.
Comment 1 arno. 2012-06-14 15:35:27 PDT
Created attachment 147662 [details]
patch proposal

patch proposal. I get a 5% to 15% speedup in CanvasRenderingContext2D.putImageData in the gtk port. Color constructor does not seem to be called with signed char nor unsigned int, but makeRGBA is called with unsigned int. So, I still added Color constructor for char, unsigned char and unsigned int, just in case
Comment 2 WebKit Review Bot 2012-06-14 15:39:26 PDT
Attachment 147662 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/Color.h:63:  The parameter name "r" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/Color.h:63:  The parameter name "g" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/Color.h:64:  The parameter name "r" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/Color.h:64:  The parameter name "g" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/Color.h:64:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/Color.h:65:  The parameter name "r" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/Color.h:66:  The parameter name "r" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/Color.h:66:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/Color.h:67:  The parameter name "g" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/Color.h:68:  The parameter name "g" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 10 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 arno. 2012-06-14 15:45:07 PDT
(In reply to comment #2)
> Attachment 147662 [details] did not pass style-queue:

I've used this style to match RGBA32 makeRGB(int r, int g, int b) and RGBA32 makeRGBA(int r, int g, int b, int a) declarations.
Comment 4 Build Bot 2012-06-14 15:59:08 PDT
Comment on attachment 147662 [details]
patch proposal

Attachment 147662 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12951763
Comment 5 Build Bot 2012-06-14 16:00:23 PDT
Comment on attachment 147662 [details]
patch proposal

Attachment 147662 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12964316
Comment 6 arno. 2012-06-14 16:52:05 PDT
Actually, I need either to add more function overloads, or modify code in many places to use makeRGBA32FromFloats instead of makeRGBA(float, float, float, float).

Alternatively, as cairo putByteArray would be the main beneficiary of the patch, I can let Color.h and Color.cpp untouched and make changes in cairo port only. So, closing this bug for now.
Comment 7 arno. 2012-06-14 16:58:35 PDT
For reference, I have opened bug #89138 for a cairo-only version of this bug.