Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1311043008:
Added ComposeShaderBitmapGM that tests composing bitmaps with gradients (Closed)
DescriptionWill try composing a linear gradient with a color bitmap as well as an alpha8 bitmap to make sure the paint's color is being used/ignored in the right places.
BUG=skia:4182
Committed: https://skia.googlesource.com/skia/+/d7059583758a4a500cb4ebfe6b3bc18c86d1816a
Patch Set 1 #
Total comments: 7
Patch Set 2 : made squareLength a static const member #Patch Set 3 : rebase #Patch Set 4 : fixed build errors #Messages
Total messages: 20 (8 generated)
wangyix@google.com changed reviewers: + bsalomon@google.com, egdaniel@google.com, joshualitt@google.com, tomhudson@google.com
https://codereview.chromium.org/1311043008/diff/1/gm/composeshader.cpp File gm/composeshader.cpp (right): https://codereview.chromium.org/1311043008/diff/1/gm/composeshader.cpp#newcode17 gm/composeshader.cpp:17: #include "SkBitmapProcShader.h" Alphabetize this include https://codereview.chromium.org/1311043008/diff/1/gm/composeshader.cpp#newcod... gm/composeshader.cpp:163: /** Can set the dimension of the bitmap that will be used. This way, larger numbers can be Are you saying this will draw differently in release and debug? We want to make sure that those are identical https://codereview.chromium.org/1311043008/diff/1/gm/composeshader.cpp#newcod... gm/composeshader.cpp:164: * used for release mode since debug mode will fail an assertion in SkSmallAllocator unless the align comment https://codereview.chromium.org/1311043008/diff/1/gm/composeshader.cpp#newcod... gm/composeshader.cpp:195: SkAutoTUnref<SkXfermode> xfer(SkXfermode::Create(SkXfermode::kDstOver_Mode)); I wonder if it would be interesting to loop over xfermodes here https://codereview.chromium.org/1311043008/diff/1/gm/composeshader.cpp#newcod... gm/composeshader.cpp:242: DEF_GM( return new ComposeShaderBitmapGM(20); ) This is a general question to anyone, but do we have any other gm's that take in a param like this? Would it make more sense to declare some const value within the code itself and if someone wanted to make it larger they could come in and update that const. I at least feel that is more uniform with other gms but I may be wrong.
https://codereview.chromium.org/1311043008/diff/1/gm/composeshader.cpp File gm/composeshader.cpp (right): https://codereview.chromium.org/1311043008/diff/1/gm/composeshader.cpp#newcod... gm/composeshader.cpp:163: /** Can set the dimension of the bitmap that will be used. This way, larger numbers can be On 2015/09/02 20:15:36, egdaniel wrote: > Are you saying this will draw differently in release and debug? We want to make > sure that those are identical No, I was trying to say you can easily change this number to a larger number in the code, then compile a release build so the images show up larger and is easier to inspect. Maybe all this isn't necessary and the parameter should just be removed?
On 2015/09/02 20:27:43, wangyix wrote: > https://codereview.chromium.org/1311043008/diff/1/gm/composeshader.cpp > File gm/composeshader.cpp (right): > > https://codereview.chromium.org/1311043008/diff/1/gm/composeshader.cpp#newcod... > gm/composeshader.cpp:163: /** Can set the dimension of the bitmap that will be > used. This way, larger numbers can be > On 2015/09/02 20:15:36, egdaniel wrote: > > Are you saying this will draw differently in release and debug? We want to > make > > sure that those are identical > > No, I was trying to say you can easily change this number to a larger number in > the code, then compile a release build so the images show up larger and is > easier to inspect. Maybe all this isn't necessary and the parameter should just > be removed? I'm thinking the cleanest and best compromise would probably just to have some const variable in the code. Then along with that variable you can have a comment saying "any value above 20 causes an assertion to be hit in _____ in debug builds" Having the parameter being passed into the gm seems confusing since it is not a value we're actively changing to test different things.
https://codereview.chromium.org/1311043008/diff/1/gm/composeshader.cpp File gm/composeshader.cpp (right): https://codereview.chromium.org/1311043008/diff/1/gm/composeshader.cpp#newcode17 gm/composeshader.cpp:17: #include "SkBitmapProcShader.h" On 2015/09/02 20:15:36, egdaniel wrote: > Alphabetize this include Done.
On 2015/09/02 20:49:35, wangyix wrote: > https://codereview.chromium.org/1311043008/diff/1/gm/composeshader.cpp > File gm/composeshader.cpp (right): > > https://codereview.chromium.org/1311043008/diff/1/gm/composeshader.cpp#newcode17 > gm/composeshader.cpp:17: #include "SkBitmapProcShader.h" > On 2015/09/02 20:15:36, egdaniel wrote: > > Alphabetize this include > > Done. lgtm
The CQ bit was checked by wangyix@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1311043008/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1311043008/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by wangyix@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from joshualitt@google.com Link to the patchset: https://codereview.chromium.org/1311043008/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1311043008/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1311043008/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...) Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by wangyix@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from joshualitt@google.com Link to the patchset: https://codereview.chromium.org/1311043008/#ps60001 (title: "fixed build errors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1311043008/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1311043008/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/d7059583758a4a500cb4ebfe6b3bc18c86d1816a |
