Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(26)

Issue 22889020: Refactor SkPDFImage (Closed)

Created:
6 years, 2 months ago by ducky
Modified:
6 years, 2 months ago
CC:
skia-review_googlegroups.com
Visibility:
Public.

Description

Patch Set 1 #

Patch Set 2 : Fixes #

Patch Set 3 : Make JPEG mode not crash #

Total comments: 2

Patch Set 4 : Cleanup transparency tests #

Patch Set 5 : Simplify comparison in 4-bit case #

Total comments: 26

Patch Set 6 : Merge back image and alpha extraction, other minor fixes #

Patch Set 7 : More style changes #

Patch Set 8 : Refactor to delay alpha stream generation #

Patch Set 9 : A slightly different style #

Total comments: 34

Patch Set 10 : Style improvements from review comments #

Patch Set 11 : Further style improvements #

Total comments: 10

Patch Set 12 : More style fixes #

Total comments: 2

Patch Set 13 : Fix some nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+408 lines, -341 lines) Patch
M gyp/pdf.gypi View 1 chunk +0 lines, -2 lines 0 comments Download
M src/pdf/SkPDFImage.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +34 lines, -8 lines 0 comments Download
M src/pdf/SkPDFImage.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +374 lines, -199 lines 0 comments Download
D src/pdf/SkPDFImageStream.h View 1 chunk +0 lines, -53 lines 0 comments Download
D src/pdf/SkPDFImageStream.cpp View 1 chunk +0 lines, -79 lines 0 comments Download

Messages

Total messages: 17 (0 generated)
ducky
This refactors SkPDFImage to: - Combine SkPDFImage and SkPDFImageStream. The artificial separation layer between those ...
6 years, 2 months ago (2013-08-21 18:26:20 UTC) #1
edisonn
https://codereview.chromium.org/22889020/diff/8001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/8001/src/pdf/SkPDFImage.cpp#newcode167 src/pdf/SkPDFImage.cpp:167: if (alphaDst[0] != 0xFF) { alphaDst[0] != 0xFF if ...
6 years, 2 months ago (2013-08-21 18:40:51 UTC) #2
ducky
Fixed! https://codereview.chromium.org/22889020/diff/8001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/8001/src/pdf/SkPDFImage.cpp#newcode167 src/pdf/SkPDFImage.cpp:167: if (alphaDst[0] != 0xFF) { On 2013/08/21 18:40:51, ...
6 years, 2 months ago (2013-08-21 19:27:53 UTC) #3
vandebo (ex-Chrome)
https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (left): https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp#oldcode22 src/pdf/SkPDFImage.cpp:22: SkStream** imageData, SkStream** alphaData) { We talked about the ...
6 years, 2 months ago (2013-08-21 22:37:17 UTC) #4
ducky
Addresses most of the stylistic issues, and adds some more fixes. https://codereview.chromium.org/22889020/diff/19001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (left): ...
6 years, 2 months ago (2013-08-22 03:48:05 UTC) #5
ducky
Here's a different style, using SkBitmap::isOpaque to determine whether or not to add a soft ...
6 years, 2 months ago (2013-08-22 05:49:41 UTC) #6
ducky
Ok, this looks prettier. I think.
6 years, 2 months ago (2013-08-22 20:59:00 UTC) #7
vandebo (ex-Chrome)
https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#newcode55 src/pdf/SkPDFImage.cpp:55: return get_row_bytes(bitmap, srcRect) * srcRect.height(); Here and extract_index8_image are ...
6 years, 2 months ago (2013-08-23 05:25:20 UTC) #8
vandebo (ex-Chrome)
https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#newcode379 src/pdf/SkPDFImage.cpp:379: SkAutoTUnref<SkStream> alphaData( nit: if bitmap.isOpaque() is set, then you ...
6 years, 2 months ago (2013-08-23 05:45:10 UTC) #9
ducky
Fixed! https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#newcode55 src/pdf/SkPDFImage.cpp:55: return get_row_bytes(bitmap, srcRect) * srcRect.height(); On 2013/08/23 05:25:20, ...
6 years, 2 months ago (2013-08-23 06:59:08 UTC) #10
vandebo (ex-Chrome)
https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#newcode82 src/pdf/SkPDFImage.cpp:82: bool* hasAlpha, On 2013/08/23 06:59:08, ducky wrote: > On ...
6 years, 2 months ago (2013-08-23 15:45:35 UTC) #11
vandebo (ex-Chrome)
https://codereview.chromium.org/22889020/diff/48001/src/pdf/SkPDFImage.h File src/pdf/SkPDFImage.h (right): https://codereview.chromium.org/22889020/diff/48001/src/pdf/SkPDFImage.h#newcode96 src/pdf/SkPDFImage.h:96: void initImageParams(bool isAlpha); Remove.
6 years, 2 months ago (2013-08-23 15:50:18 UTC) #12
ducky
Another style fix patchset. https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/38001/src/pdf/SkPDFImage.cpp#newcode82 src/pdf/SkPDFImage.cpp:82: bool* hasAlpha, On 2013/08/23 15:45:35, ...
6 years, 2 months ago (2013-08-23 17:59:50 UTC) #13
vandebo (ex-Chrome)
LGTM https://codereview.chromium.org/22889020/diff/24012/src/pdf/SkPDFImage.cpp File src/pdf/SkPDFImage.cpp (right): https://codereview.chromium.org/22889020/diff/24012/src/pdf/SkPDFImage.cpp#newcode263 src/pdf/SkPDFImage.cpp:263: bool localTransparent = true; nit: localTransparent -> transparent ...
6 years, 2 months ago (2013-08-23 18:22:52 UTC) #14
ducky
Fixed. Will commit when trybots go green.
6 years, 2 months ago (2013-08-23 18:29:57 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/22889020/57001
6 years, 2 months ago (2013-08-23 19:02:19 UTC) #16
commit-bot: I haz the power
6 years, 2 months ago (2013-08-23 19:07:01 UTC) #17
Message was sent while issue was closed.
Change committed as 10896

Powered by Google App Engine
This is Rietveld 408576698