Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16094020:
Fixed a bug with linear gradient PDF matrices and added test cases (Closed)
|
Created:
7 years, 6 months ago by ducky Modified:
7 years, 6 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionFixed a bug with linear gradient PDF matrices and added test cases
Committed: http://code.google.com/p/skia/source/detail?r=9553
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressing code review comments #
Total comments: 12
Patch Set 3 : More fixes addressing code review comments #
Total comments: 26
Patch Set 4 : More fixes from code review comments #Patch Set 5 : Another minor style change #Patch Set 6 : More style nits. #
Total comments: 2
Patch Set 7 : Dynamically generate shader matrix from rectangle #Patch Set 8 : Attempt at fixing Mac build #
Messages
Total messages: 18 (0 generated)
Not bad for your first code review. I'm happy to answer questions or discuss any of these comments. https://codereview.chromium.org/16094020/diff/1/gm/gradient_matrix.cpp File gm/gradient_matrix.cpp (right): https://codereview.chromium.org/16094020/diff/1/gm/gradient_matrix.cpp#newcode17 gm/gradient_matrix.cpp:17: static SkShader* SimpleGradient(const SkPoint pts[2]) { static helper functions are named in hacker case: simple_gradient() https://codereview.chromium.org/16094020/diff/1/gm/gradient_matrix.cpp#newcode19 gm/gradient_matrix.cpp:19: 2, SkShader::kClamp_TileMode, NULL); nit: put "2" on the previous line. https://codereview.chromium.org/16094020/diff/1/gm/gradient_matrix.cpp#newcode35 gm/gradient_matrix.cpp:35: virtual SkISize onISize() { return make_isize(800, 400); } line wrap. https://codereview.chromium.org/16094020/diff/1/gm/gradient_matrix.cpp#newcode38 gm/gradient_matrix.cpp:38: const int TESTGRID_X = 200; Should be SkScalar instead of int (applies in multiple places). https://codereview.chromium.org/16094020/diff/1/gm/gradient_matrix.cpp#newcode64 gm/gradient_matrix.cpp:64: virtual void simpleGrad(SkCanvas* canvas) { This method is unused, remove. https://codereview.chromium.org/16094020/diff/1/gm/gradient_matrix.cpp#newcode84 gm/gradient_matrix.cpp:84: virtual void testGrad(SkCanvas* canvas, int x1, int y1, int x2, int y2) { not virtual. In fact, it can be a static helper function. https://codereview.chromium.org/16094020/diff/1/gm/gradient_matrix.cpp#newcode85 gm/gradient_matrix.cpp:85: SkPoint pts[2] = { why not just pass SkPoints[2] ? https://codereview.chromium.org/16094020/diff/1/gm/gradient_matrix.cpp#newcode98 gm/gradient_matrix.cpp:98: paint.setAntiAlias(true); remove - not needed for this test.
Addresses issues in code review comments. Based on code from https://codereview.chromium.org/16467002/ minus radial gradient specific stuff. https://codereview.chromium.org/16094020/diff/1/gm/gradient_matrix.cpp File gm/gradient_matrix.cpp (right): https://codereview.chromium.org/16094020/diff/1/gm/gradient_matrix.cpp#newcode17 gm/gradient_matrix.cpp:17: static SkShader* SimpleGradient(const SkPoint pts[2]) { On 2013/06/11 05:15:10, vandebo wrote: > static helper functions are named in hacker case: simple_gradient() Done. https://codereview.chromium.org/16094020/diff/1/gm/gradient_matrix.cpp#newcode19 gm/gradient_matrix.cpp:19: 2, SkShader::kClamp_TileMode, NULL); On 2013/06/11 05:15:10, vandebo wrote: > nit: put "2" on the previous line. Done. https://codereview.chromium.org/16094020/diff/1/gm/gradient_matrix.cpp#newcode35 gm/gradient_matrix.cpp:35: virtual SkISize onISize() { return make_isize(800, 400); } On 2013/06/11 05:15:10, vandebo wrote: > line wrap. Done. https://codereview.chromium.org/16094020/diff/1/gm/gradient_matrix.cpp#newcode38 gm/gradient_matrix.cpp:38: const int TESTGRID_X = 200; On 2013/06/11 05:15:10, vandebo wrote: > Should be SkScalar instead of int (applies in multiple places). Done. https://codereview.chromium.org/16094020/diff/1/gm/gradient_matrix.cpp#newcode64 gm/gradient_matrix.cpp:64: virtual void simpleGrad(SkCanvas* canvas) { On 2013/06/11 05:15:10, vandebo wrote: > This method is unused, remove. Done. https://codereview.chromium.org/16094020/diff/1/gm/gradient_matrix.cpp#newcode84 gm/gradient_matrix.cpp:84: virtual void testGrad(SkCanvas* canvas, int x1, int y1, int x2, int y2) { On 2013/06/11 05:15:10, vandebo wrote: > not virtual. In fact, it can be a static helper function. Done. https://codereview.chromium.org/16094020/diff/1/gm/gradient_matrix.cpp#newcode85 gm/gradient_matrix.cpp:85: SkPoint pts[2] = { On 2013/06/11 05:15:10, vandebo wrote: > why not just pass SkPoints[2] ? Done. https://codereview.chromium.org/16094020/diff/1/gm/gradient_matrix.cpp#newcode98 gm/gradient_matrix.cpp:98: paint.setAntiAlias(true); On 2013/06/11 05:15:10, vandebo wrote: > remove - not needed for this test. Done.
https://codereview.chromium.org/16094020/diff/5001/gm/gradient_matrix.cpp File gm/gradient_matrix.cpp (right): https://codereview.chromium.org/16094020/diff/5001/gm/gradient_matrix.cpp#new... gm/gradient_matrix.cpp:39: SkScalar* ptsArray, int numImages) { nit: indent https://codereview.chromium.org/16094020/diff/5001/gm/gradient_matrix.cpp#new... gm/gradient_matrix.cpp:49: SkPoint pts[2] = { You shouldn't need this bit. https://codereview.chromium.org/16094020/diff/5001/gm/gradient_matrix.cpp#new... gm/gradient_matrix.cpp:55: SkMatrix shaderMat = SkMatrix(); Pull this stuff above the loop. https://codereview.chromium.org/16094020/diff/5001/gm/gradient_matrix.cpp#new... gm/gradient_matrix.cpp:61: SkShader* shader = makeShader(pts); use SkAutoTUnref https://codereview.chromium.org/16094020/diff/5001/gm/gradient_matrix.cpp#new... gm/gradient_matrix.cpp:93: canvas->save(); Do the save restore around the for loop instead of here. https://codereview.chromium.org/16094020/diff/5001/gm/gradient_matrix.cpp#new... gm/gradient_matrix.cpp:96: linearPts, sizeof(linearPts) / sizeof(linearPts[0]) / 4); SK_ARRAY_COUNT()
More fixes! https://codereview.chromium.org/16094020/diff/5001/gm/gradient_matrix.cpp File gm/gradient_matrix.cpp (right): https://codereview.chromium.org/16094020/diff/5001/gm/gradient_matrix.cpp#new... gm/gradient_matrix.cpp:39: SkScalar* ptsArray, int numImages) { On 2013/06/11 22:11:05, vandebo wrote: > nit: indent Done. https://codereview.chromium.org/16094020/diff/5001/gm/gradient_matrix.cpp#new... gm/gradient_matrix.cpp:49: SkPoint pts[2] = { On 2013/06/11 22:11:05, vandebo wrote: > You shouldn't need this bit. Done. https://codereview.chromium.org/16094020/diff/5001/gm/gradient_matrix.cpp#new... gm/gradient_matrix.cpp:55: SkMatrix shaderMat = SkMatrix(); On 2013/06/11 22:11:05, vandebo wrote: > Pull this stuff above the loop. Done. https://codereview.chromium.org/16094020/diff/5001/gm/gradient_matrix.cpp#new... gm/gradient_matrix.cpp:61: SkShader* shader = makeShader(pts); On 2013/06/11 22:11:05, vandebo wrote: > use SkAutoTUnref Done. https://codereview.chromium.org/16094020/diff/5001/gm/gradient_matrix.cpp#new... gm/gradient_matrix.cpp:93: canvas->save(); On 2013/06/11 22:11:05, vandebo wrote: > Do the save restore around the for loop instead of here. Done. https://codereview.chromium.org/16094020/diff/5001/gm/gradient_matrix.cpp#new... gm/gradient_matrix.cpp:96: linearPts, sizeof(linearPts) / sizeof(linearPts[0]) / 4); On 2013/06/11 22:11:05, vandebo wrote: > SK_ARRAY_COUNT() Done.
getting much cleaner, thanks. https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp File gm/gradient_matrix.cpp (right): https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:6: */ nit: LF after */ https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:12: SkColor gColors[] = { static const or move into scope of the caller https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:18: SkPoint linearPts[][2] = { static const or move into scope of the caller https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:43: SkMatrix shaderMat = SkMatrix(); not sure why you need this assignment? https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:44: shaderMat.setAll(138, 0, 43, can we have a brief comment why you're using these values? https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:82: SkString onShortName() { SK_OVERRIDE https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:86: virtual SkISize onISize() { SK_OVERRIDE https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:90: virtual void onDraw(SkCanvas* canvas) { SK_OVERRIDE https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:99: static GM* MyFactory(void*) { return new GradientMatrixGM; } DEF_GM( return new GradientMatrixGM; )
https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp File gm/gradient_matrix.cpp (right): https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:8: Include what you use... SkColor, SkPoint, SkScalar, SkMatrix, SkRect, SkCanvas, SkPaint, SkAutoTUnref https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:58: // unpack points, setup shader, draw Update comment. Comments should be sentences, starting with a capital letter and ending with a period. https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:66: // advance to next position Comments should explain why you're doing something, not what the code is doing, that should hopefully be obvious from the code. With that in mind, you probably don't need a comment here.
https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp File gm/gradient_matrix.cpp (right): https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:8: On 2013/06/12 16:25:33, vandebo wrote: > Include what you use... SkColor, SkPoint, SkScalar, SkMatrix, SkRect, SkCanvas, > SkPaint, SkAutoTUnref Possibly, but almost no where in skia to we really do this completely. Without some form of lint, its hard to know if you're including too much, etc. https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:66: // advance to next position On 2013/06/12 16:25:33, vandebo wrote: > Comments should explain why you're doing something, not what the code is doing, > that should hopefully be obvious from the code. With that in mind, you probably > don't need a comment here. Not sure I agree. I find this comment helpful to know why we are calling translate.
Fixed code review comments. https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp File gm/gradient_matrix.cpp (right): https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:6: */ On 2013/06/12 13:34:33, reed1 wrote: > nit: LF after */ Done. https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:8: On 2013/06/12 17:11:26, reed1 wrote: > On 2013/06/12 16:25:33, vandebo wrote: > > Include what you use... SkColor, SkPoint, SkScalar, SkMatrix, SkRect, > SkCanvas, > > SkPaint, SkAutoTUnref > > Possibly, but almost no where in skia to we really do this completely. Without > some form of lint, its hard to know if you're including too much, etc. Done - added additional includes. https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:12: SkColor gColors[] = { On 2013/06/12 13:34:33, reed1 wrote: > static const > > or move into scope of the caller Done. https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:18: SkPoint linearPts[][2] = { On 2013/06/12 13:34:33, reed1 wrote: > static const > > or move into scope of the caller Done. https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:43: SkMatrix shaderMat = SkMatrix(); On 2013/06/12 13:34:33, reed1 wrote: > not sure why you need this assignment? Done - removed. https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:44: shaderMat.setAll(138, 0, 43, On 2013/06/12 13:34:33, reed1 wrote: > can we have a brief comment why you're using these values? Done. https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:58: // unpack points, setup shader, draw On 2013/06/12 16:25:33, vandebo wrote: > Update comment. Comments should be sentences, starting with a capital letter > and ending with a period. Done. https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:66: // advance to next position On 2013/06/12 17:11:26, reed1 wrote: > On 2013/06/12 16:25:33, vandebo wrote: > > Comments should explain why you're doing something, not what the code is > doing, > > that should hopefully be obvious from the code. With that in mind, you > probably > > don't need a comment here. > > Not sure I agree. I find this comment helpful to know why we are calling > translate. Done - leaving as is. https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:82: SkString onShortName() { On 2013/06/12 13:34:33, reed1 wrote: > SK_OVERRIDE Done. https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:86: virtual SkISize onISize() { On 2013/06/12 13:34:33, reed1 wrote: > SK_OVERRIDE Done. https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:90: virtual void onDraw(SkCanvas* canvas) { On 2013/06/12 13:34:33, reed1 wrote: > SK_OVERRIDE Done. https://codereview.chromium.org/16094020/diff/10001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:99: static GM* MyFactory(void*) { return new GradientMatrixGM; } On 2013/06/12 13:34:33, reed1 wrote: > DEF_GM( return new GradientMatrixGM; ) Done.
https://codereview.chromium.org/16094020/diff/23001/gm/gradient_matrix.cpp File gm/gradient_matrix.cpp (right): https://codereview.chromium.org/16094020/diff/23001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:55: // Make matrix consistent with the rectangle, and have different What does 'consistent' mean here? That the matrix' translate is the same as the rect's top/left?
https://codereview.chromium.org/16094020/diff/23001/gm/gradient_matrix.cpp File gm/gradient_matrix.cpp (right): https://codereview.chromium.org/16094020/diff/23001/gm/gradient_matrix.cpp#ne... gm/gradient_matrix.cpp:55: // Make matrix consistent with the rectangle, and have different On 2013/06/12 18:32:28, reed1 wrote: > What does 'consistent' mean here? That the matrix' translate is the same as the > rect's top/left? Yes. Otherwise, the shader gradient bounds won't line up with the rectangle bounds.
How about matrix.setScale(138, 106); matrix.postTranslate(rect.fLeft, rect.fTop); ?
Generates shader matrix from rectangle.
lgtm
LGTM too
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/16094020/31001
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/16094020/26002
Message was sent while issue was closed.
Change committed as 9553 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
