Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(336)

Issue 79082: Add bitmap manipulation functions in advance of themes. (Closed)

Created:
11 years, 8 months ago by Glen Murphy
Modified:
9 years, 5 months ago
Reviewers:
brettw
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Add bitmap manipulation functions in advance of themes. HSLToSkColor premultiplies its output, should this instead live in the ImageOperations bitmap munging code? Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=14712

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Total comments: 9

Patch Set 6 : '' #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+653 lines, -5 lines) Patch
M skia/ext/image_operations.h View 1 2 3 4 5 2 chunks +35 lines, -1 line 1 comment Download
M skia/ext/image_operations.cc View 1 2 3 4 5 4 chunks +206 lines, -4 lines 1 comment Download
M skia/ext/image_operations_unittest.cc View 1 2 3 4 5 2 chunks +254 lines, -0 lines 0 comments Download
M skia/ext/skia_utils.h View 1 2 3 4 5 1 chunk +6 lines, -0 lines 1 comment Download
M skia/ext/skia_utils.cc View 1 2 3 4 5 2 chunks +100 lines, -0 lines 0 comments Download
A skia/ext/skia_utils_unittest.cc View 1 chunk +47 lines, -0 lines 0 comments Download
M webkit/tools/test_shell/test_shell.gyp View 1 chunk +1 line, -0 lines 0 comments Download
M webkit/tools/test_shell/test_shell_tests.vcproj View 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
Glen Murphy
11 years, 8 months ago (2009-04-18 06:20:23 UTC) #1
brettw
http://codereview.chromium.org/79082/diff/32/34 File skia/ext/image_operations.cc (right): http://codereview.chromium.org/79082/diff/32/34#newcode420 Line 420: for (int y = 0; y < bitmap.height(); ...
11 years, 8 months ago (2009-04-20 06:30:48 UTC) #2
Glen Murphy
Done. Can you verify that premultiplying the HSLToSkColor output is correct (vs. doing it as ...
11 years, 8 months ago (2009-04-23 02:09:49 UTC) #3
brettw
11 years, 8 months ago (2009-04-28 02:49:43 UTC) #4
LGTM with these fixes.

http://codereview.chromium.org/79082/diff/2001/3002
File skia/ext/image_operations.cc (right):

http://codereview.chromium.org/79082/diff/2001/3002#newcode532
Line 532: SkBitmap ImageOperations::CreateTiledBitmap(const SkBitmap& source,
FWIW this could have been done by making a tiled bitmap shader and just drawing
the bitmap to the canvas using Skia. But your approach is fine.

http://codereview.chromium.org/79082/diff/2001/3003
File skia/ext/image_operations.h (right):

http://codereview.chromium.org/79082/diff/2001/3003#newcode58
Line 58: // in the alpha bitmap.
Can you add in this comment whether the images can be different sizes and what
will happen in this case?

http://codereview.chromium.org/79082/diff/2001/3005
File skia/ext/skia_utils.h (right):

http://codereview.chromium.org/79082/diff/2001/3005#newcode25
Line 25: void SkColorToHSL(SkColor c, float hsl[3]);
Since the colors are premultiplied, you should use SkPMColor here, below, and in
image_operations internally when you read the values. That way the
premultiplication aspect of it (which is fine) is self-documenting and
type-checked.

Powered by Google App Engine
This is Rietveld 408576698