remove unneeded SkShader::validContext

remove unneeded SkShader::validContext

moved to https://codereview.chromium.org/261773005

[scroggo, 2014-05-05 14:01:19]

https://codereview.chromium.org/267923005/diff/60001/include/effects/SkTransp...
File include/effects/SkTransparentShader.h (right):

https://codereview.chromium.org/267923005/diff/60001/include/effects/SkTransp...
include/effects/SkTransparentShader.h:13: class SK_API SkTransparentShader :
public SkShader {
Thoughts for a potential future CL:

Do we need SkTransparentShader? I don't think Chrome or Android uses it. If we
dropped it, I think we could drop fDevice from ContextRec.

https://codereview.chromium.org/267923005/diff/60001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/267923005/diff/60001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:105: return NULL;
Don't we need to call SkBitmapProcState's destructor here? Same with the other
return NULL from computeTotalInverse. This is a good candidate for
SkAutoTCallVProc (or SkAutoTCallIProc, which appears to be identical in
implementation and comments).

https://codereview.chromium.org/267923005/diff/60001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:108: // Make sure we can use totalInverse as a
cache.
This comment no longer applies.

https://codereview.chromium.org/267923005/diff/60001/src/core/SkComposeShader...
File src/core/SkComposeShader.cpp (left):

https://codereview.chromium.org/267923005/diff/60001/src/core/SkComposeShader...
src/core/SkComposeShader.cpp:103: // TODO : must fix this to not "cheat" and
modify fPaint
Did this get fixed? It seems like this may change the behavior.

https://codereview.chromium.org/267923005/diff/60001/src/core/SkComposeShader...
File src/core/SkComposeShader.cpp (right):

https://codereview.chromium.org/267923005/diff/60001/src/core/SkComposeShader...
src/core/SkComposeShader.cpp:91: if (!contextA || !contextB) {
If exactly one of these is NULL, we need to call the destructor on the other.

https://codereview.chromium.org/267923005/diff/60001/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

https://codereview.chromium.org/267923005/diff/60001/src/core/SkDraw.cpp#newc...
src/core/SkDraw.cpp:2562: if (!blitter->resetShaderContext(*fBitmap, p,
*fMatrix)) {
The SkSmallAllocator will eventually call ~Context() on whatever it points to.
Now that it may point to NULL (since resetShaderContext can now destroy the old
one without creating a new one), we need to make sure we do not call ~Context()
in that case.

https://codereview.chromium.org/267923005/diff/60001/src/core/SkPictureShader...
File src/core/SkPictureShader.cpp (right):

https://codereview.chromium.org/267923005/diff/60001/src/core/SkPictureShader...
src/core/SkPictureShader.cpp:125: if (!ctx->isValid()) {
Don't we have a general rule against creating an object that might be invalid
and then querying it?

This could be simplified by creating the BitmapShaderContext outside of
PictureShaderContext and passing it in.

https://codereview.chromium.org/267923005/diff/60001/src/core/SkShader.cpp
File src/core/SkShader.cpp (right):

https://codereview.chromium.org/267923005/diff/60001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:48: bool SkShader::computeTotalInverse(const ContextRec&
rec, SkMatrix* totalInverse) const {
Why does this take a ContextRec when it only looks at the ctm?

[reed1, 2014-05-05 14:13:34]

moved to https://codereview.chromium.org/261773005

[reed1, 2014-05-05 15:10:00]

https://codereview.chromium.org/267923005/diff/60001/include/effects/SkTransp...
File include/effects/SkTransparentShader.h (right):

https://codereview.chromium.org/267923005/diff/60001/include/effects/SkTransp...
include/effects/SkTransparentShader.h:13: class SK_API SkTransparentShader :
public SkShader {
On 2014/05/05 14:01:19, scroggo wrote:
> Thoughts for a potential future <span id="abbrex-spnAbbr_16471"
class="abbrex-Abbr abbrex-Abbr_CL abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">CL</span>:
> 
> Do we need SkTransparentShader? I don't think Chrome or Android uses it. If we
> dropped it, I think we could drop fDevice from ContextRec.

Agreed, I bet we can remove it.

https://codereview.chromium.org/267923005/diff/60001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/267923005/diff/60001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:105: return NULL;
On 2014/05/05 14:01:19, scroggo wrote:
> Don't we need to call SkBitmapProcState's destructor here? Same with the other
> return <span id="abbrex-spnAbbr_16493" class="abbrex-Abbr abbrex-Abbr_NULL
abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">NULL</span> from
computeTotalInverse. This is a good candidate for
> SkAutoTCallVProc (or SkAutoTCallIProc, which appears to be identical in
> implementation and comments).

Rearranged to fix this.

https://codereview.chromium.org/267923005/diff/60001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:108: // Make sure we can use totalInverse as a
cache.
On 2014/05/05 14:01:19, scroggo wrote:
> This comment no longer applies.

Done.

https://codereview.chromium.org/267923005/diff/60001/src/core/SkComposeShader...
File src/core/SkComposeShader.cpp (left):

https://codereview.chromium.org/267923005/diff/60001/src/core/SkComposeShader...
src/core/SkComposeShader.cpp:103: // TODO : must fix this to not "cheat" and
modify fPaint
On 2014/05/05 14:01:19, scroggo wrote:
> Did this get fixed? It seems like this may change the behavior.

Dang, good catch! Fill fix/restore

https://codereview.chromium.org/267923005/diff/60001/src/core/SkComposeShader...
File src/core/SkComposeShader.cpp (right):

https://codereview.chromium.org/267923005/diff/60001/src/core/SkComposeShader...
src/core/SkComposeShader.cpp:91: if (!contextA || !contextB) {
On 2014/05/05 14:01:19, scroggo wrote:
> If exactly one of these is <span id="abbrex-spnAbbr_16528" class="abbrex-Abbr
abbrex-Abbr_NULL abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">NULL</span>, we
need to call the destructor on the other.

Doh! Good catch. Will fix.

https://codereview.chromium.org/267923005/diff/60001/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

https://codereview.chromium.org/267923005/diff/60001/src/core/SkDraw.cpp#newc...
src/core/SkDraw.cpp:2562: if (!blitter->resetShaderContext(*fBitmap, p,
*fMatrix)) {
On 2014/05/05 14:01:19, scroggo wrote:
> The SkSmallAllocator will eventually call ~Context() on whatever it points to.
> Now that it may point to NULL (since resetShaderContext can now destroy the
old
> one without creating a new one), we need to make sure we do not call
~Context()
> in that case.

Will change resetShaderContext to always have something valid in that buffer.

https://codereview.chromium.org/267923005/diff/60001/src/core/SkPictureShader...
File src/core/SkPictureShader.cpp (right):

https://codereview.chromium.org/267923005/diff/60001/src/core/SkPictureShader...
src/core/SkPictureShader.cpp:125: if (!ctx->isValid()) {
On 2014/05/05 14:01:19, scroggo wrote:
> Don't we have a general rule against creating an object that might be invalid
> and then querying it?
> 
> This could be simplified by creating the BitmapShaderContext outside of
> PictureShaderContext and passing it in.

Done.

https://codereview.chromium.org/267923005/diff/60001/src/core/SkShader.cpp
File src/core/SkShader.cpp (right):

https://codereview.chromium.org/267923005/diff/60001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:48: bool SkShader::computeTotalInverse(const ContextRec&
rec, SkMatrix* totalInverse) const {
On 2014/05/05 14:01:19, scroggo wrote:
> Why does this take a ContextRec when it only looks at the ctm?

Preemptive, as the method returns the inverse of *all* of the rec, which today
only has matrix, but tomorrow may/will have more state. (e.g. localmatrix)