Move SkShader::fLocalMatrix into SkShader constructor.

Move SkShader::fLocalMatrix into SkShader constructor.

As a first step towards removing SkShader::setLocalMatrix, which will make
SkShader thread-safe, remove calls to setLocalMatrix that happen immediately
after the shader is being created. Instead, pass the matrix into the constructor
or factory method.

BUG=skia:1976
Committed: http://code.google.com/p/skia/source/detail?r=14401

[Dominik Grewe, 2014-04-23 17:19:06]

Assuming the first patch gets "re-reverted", here's my current WIP on
fLocalMatrix.

I've moved calls to setLocalMatrix to the constructor wherever possible, but I
think there are a few places where we won't get around a wrapper or something
similar (see TODOs).
In some places I ended up creating the same shader multiple times but with
different local matrices. That's only in benchmarking code though, so it
shouldn't affect actual performance.

I've not looked into Chromium/Blink yet. There are also a few calls to
setLocalMatrix there.

[scroggo, 2014-04-23 19:01:27]

On 2014/04/23 17:19:06, Dominik Grewe wrote:
> I've not looked into Chromium/Blink yet. There are also a few calls to
> setLocalMatrix there.

We'll also need to update Android. Android may be pretty simple though. We might
be able lazily create the shader, and just add the local matrix when the client
actually sets one.

https://codereview.chromium.org/245963010/diff/20001/include/core/SkColorShad...
File include/core/SkColorShader.h (right):

https://codereview.chromium.org/245963010/diff/20001/include/core/SkColorShad...
include/core/SkColorShader.h:25: SkColorShader(const SkMatrix* localMatrix =
NULL);
FYI: I think Brian is removing this constructor.

https://codereview.chromium.org/245963010/diff/20001/src/animator/SkDrawGradi...
File src/animator/SkDrawGradient.cpp (right):

https://codereview.chromium.org/245963010/diff/20001/src/animator/SkDrawGradi...
src/animator/SkDrawGradient.cpp:179: fUnitMapper, 0, getMatrix());
Generally we would prepend this->

this->getMatrix()

although the surrounding code does not follow that convention, so it should be
fine as is.

https://codereview.chromium.org/245963010/diff/20001/src/effects/gradients/Sk...
File src/effects/gradients/SkGradientShaderPriv.h (right):

https://codereview.chromium.org/245963010/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkGradientShaderPriv.h:102: SkGradientShaderBase(const
Descriptor& desc, const SkMatrix* localMatrix = NULL);
Does this function need a default param, or will it always be called passing a
value from a factory?

https://codereview.chromium.org/245963010/diff/20001/src/effects/gradients/Sk...
File src/effects/gradients/SkLinearGradient.h (right):

https://codereview.chromium.org/245963010/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkLinearGradient.h:16: SkLinearGradient(const SkPoint
pts[2], const Descriptor&, const SkMatrix* localMatrix = NULL);
Again, is the default necessary?

https://codereview.chromium.org/245963010/diff/20001/src/effects/gradients/Sk...
File src/effects/gradients/SkRadialGradient.h (right):

https://codereview.chromium.org/245963010/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkRadialGradient.h:17: const SkMatrix* localMatrix =
NULL);
Default necessary?

https://codereview.chromium.org/245963010/diff/20001/src/effects/gradients/Sk...
File src/effects/gradients/SkSweepGradient.h (right):

https://codereview.chromium.org/245963010/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkSweepGradient.h:17: const SkMatrix* localMatrix = NULL);
Default necessary?

https://codereview.chromium.org/245963010/diff/20001/src/effects/gradients/Sk...
File src/effects/gradients/SkTwoPointConicalGradient.h (right):

https://codereview.chromium.org/245963010/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkTwoPointConicalGradient.h:48: const SkMatrix*
localMatrix = NULL);
Default necessary?

https://codereview.chromium.org/245963010/diff/20001/src/effects/gradients/Sk...
File src/effects/gradients/SkTwoPointRadialGradient.h (right):

https://codereview.chromium.org/245963010/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkTwoPointRadialGradient.h:18: const Descriptor&, const
SkMatrix* localMatrix = NULL);
Default necessary?

[reed1, 2014-04-23 23:54:20]

Boy, time to kill SkUnitMapper... any android objections?

[reed1, 2014-04-23 23:55:55]

macro comment:

1. I kind of like the option of specifying the matrix up front.
2. We may still want to support wrapper shaders that can modify the
local-matrix.
3. #1 and #2 may not be mutually exclusive

[Dominik Grewe, 2014-04-24 15:47:17]

I've added a new wrapper class: SkLocalMatrixShaderWrapper. I replaced all
remaining setLocalMatrix calls with it except the ones in drawVertices. It may
be possible to also use wrappers here but it will probably be quite ugly. So if
you're thinking of re-writing that function we can maybe leave them in there for
now?

The code for the wrapper class is currently in SkShader.h/cpp but we may want to
move that. Because of SkShader::CreateLocalMatrixWrapper, it doesn't even have
to be public. So we could either have it all in SkShader.cpp or in its own
.h/.cpp under src/. Wdyt?

I've not looked into the default params yet. I just left them for now. For the
ones publicly visible we need to check if they're called by Chromium.

[scroggo, 2014-04-24 17:02:53]

On 2014/04/23 23:54:20, reed1 wrote:
> Boy, time to kill SkUnitMapper... any android objections?

No. And each time we iterate on SkTwoPointRadialGradient it makes me want to get
rid of it as well.

On 2014/04/24 15:47:17, Dominik Grewe wrote:
> The code for the wrapper class is currently in SkShader.h/cpp but we may want
to
> move that. Because of SkShader::CreateLocalMatrixWrapper, it doesn't even have
> to be public. So we could either have it all in SkShader.cpp or in its own
> .h/.cpp under src/. Wdyt?

My vote would be in its own .cpp file under src/.

> 
> I've not looked into the default params yet. I just left them for now. For the
> ones publicly visible we need to check if they're called by Chromium.

I'm pretty sure Chrome calls the factories (the headers are in src/, so Chrome
shouldn't be calling the constructors).

https://codereview.chromium.org/245963010/diff/60001/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/245963010/diff/60001/include/core/SkShader.h#...
include/core/SkShader.h:54: void setLocalMatrix(const SkMatrix& localM) {
fLocalMatrix = localM; }
Can we go ahead and put this function behind a flag?

#ifdef SK_SUPPORT_DEPRECATED_SET_LOCAL_MATRIX
    void setLocalMatrix(...)
#endif

If so, we need to include the new flag in common_conditions.gypi for
skia_android_framework. Chrome also uses it, so we'll need to add the flag to
skia_for_chromium_defines.gypi (in Skia) and the GN build file for chromium
trunk/src/skia/skia_gn_files.gypi (in Chromium).

I guess we currently still need the flag for drawVertices. Florin added a flag
for calls that can only be called internally in
https://codereview.chromium.org/246023008/ (which has not yet been submitted).
We could use that, or we could make it private with a friend class for
drawVertices. (This would have the option of limiting our internal call sites
more.)

https://codereview.chromium.org/245963010/diff/60001/include/core/SkShader.h#...
include/core/SkShader.h:394: *  @return             Returns a new shader object.
Note: this function never returns null.
What if the caller calls with a NULL shader?

https://codereview.chromium.org/245963010/diff/60001/include/core/SkShader.h#...
include/core/SkShader.h:449: return fShader->asNewEffect(context, paint);
This function may call getLocalMatrix (e.g. SkBitmapProcShader). We'll need to
pass our local matrix to this function somehow (maybe we just add a parameter;
not sure what is the best way). SkPictureShader is a little more elaborate:
buildBitmapShader calls getLocalMatrix, and is called by asNewEffect.

I would guess that asABitmap and asAGradient might, too, but I haven't
investigated.

https://codereview.chromium.org/245963010/diff/60001/include/core/SkShader.h#...
include/core/SkShader.h:455: SkLocalMatrixShaderWrapper(SkShader* shader, const
SkMatrix& localMatrix);
Any reason not to make this private?

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

https://codereview.chromium.org/245963010/diff/60001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:214: , fShader(shader) {
nit: Brace should go on the next line.

https://codereview.chromium.org/245963010/diff/60001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:220: : INHERITED(buffer) {
ditto

https://codereview.chromium.org/245963010/diff/60001/src/effects/SkRectShader...
File src/effects/SkRectShaderImageFilter.cpp (right):

https://codereview.chromium.org/245963010/diff/60001/src/effects/SkRectShader...
src/effects/SkRectShaderImageFilter.cpp:72:
paint.setShader(SkShader::CreateLocalMatrixWrapper(fShader, matrix));
Don't we need to unref this shader? (Previously we were just passing fShader,
which we would unref later, but now we've created a new one.)

https://codereview.chromium.org/245963010/diff/60001/src/pdf/SkPDFDeviceFlatt...
File src/pdf/SkPDFDeviceFlattener.cpp (right):

https://codereview.chromium.org/245963010/diff/60001/src/pdf/SkPDFDeviceFlatt...
src/pdf/SkPDFDeviceFlattener.cpp:30:
paint->setShader(SkShader::CreateLocalMatrixWrapper(paint->getShader(), local));
Don't we need to unref this new shader?

[Dominik Grewe, 2014-04-24 17:16:46]

https://codereview.chromium.org/245963010/diff/60001/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/245963010/diff/60001/include/core/SkShader.h#...
include/core/SkShader.h:394: *  @return             Returns a new shader object.
Note: this function never returns null.
On 2014/04/24 17:02:53, scroggo wrote:
> What if the caller calls with a NULL shader?

Good point. I currently assert that it's not NULL but we might just want to
return NULL in that case.

https://codereview.chromium.org/245963010/diff/60001/include/core/SkShader.h#...
include/core/SkShader.h:449: return fShader->asNewEffect(context, paint);
On 2014/04/24 17:02:53, scroggo wrote:
> This function may call getLocalMatrix (e.g. SkBitmapProcShader). We'll need to
> pass our local matrix to this function somehow (maybe we just add a parameter;
> not sure what is the best way). SkPictureShader is a little more elaborate:
> buildBitmapShader calls getLocalMatrix, and is called by asNewEffect.
> 
> I would guess that asABitmap and asAGradient might, too, but I haven't
> investigated.

Thinking about this, I'm not sure what I've done here works... Any function may
call getLocalMatrix() and it will of course use the original shader's one. All
I've done here is to change the local matrix to the outside world, but anything
internal won't see it.
Guess I'll need to rethink this a bit...

https://codereview.chromium.org/245963010/diff/60001/include/core/SkShader.h#...
include/core/SkShader.h:455: SkLocalMatrixShaderWrapper(SkShader* shader, const
SkMatrix& localMatrix);
On 2014/04/24 17:02:53, scroggo wrote:
> Any reason not to make this private?

As long as we never have any subclasses we can make it private.

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

https://codereview.chromium.org/245963010/diff/60001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:214: , fShader(shader) {
On 2014/04/24 17:02:53, scroggo wrote:
> nit: Brace should go on the next line.

Done.

https://codereview.chromium.org/245963010/diff/60001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:220: : INHERITED(buffer) {
On 2014/04/24 17:02:53, scroggo wrote:
> ditto

Done.

https://codereview.chromium.org/245963010/diff/60001/src/effects/SkRectShader...
File src/effects/SkRectShaderImageFilter.cpp (right):

https://codereview.chromium.org/245963010/diff/60001/src/effects/SkRectShader...
src/effects/SkRectShaderImageFilter.cpp:72:
paint.setShader(SkShader::CreateLocalMatrixWrapper(fShader, matrix));
On 2014/04/24 17:02:53, scroggo wrote:
> Don't we need to unref this shader? (Previously we were just passing fShader,
> which we would unref later, but now we've created a new one.)

Yeah, I think you're right.

[scroggo, 2014-04-24 18:24:46]

https://codereview.chromium.org/245963010/diff/60001/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/245963010/diff/60001/include/core/SkShader.h#...
include/core/SkShader.h:455: SkLocalMatrixShaderWrapper(SkShader* shader, const
SkMatrix& localMatrix);
On 2014/04/24 17:16:47, Dominik Grewe wrote:
> On 2014/04/24 17:02:53, scroggo wrote:
> > Any reason not to make this private?
> 
> As long as we never have any subclasses we can make it private.

My vote would be to make it private unless/until we think we have a reason for a
subclass.

https://codereview.chromium.org/245963010/diff/80001/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/245963010/diff/80001/include/core/SkShader.h#...
include/core/SkShader.h:392: *  @param shader       The shader being wrapped.
Should there be a comment that its ref count is incremented?

[Dominik Grewe, 2014-04-25 11:02:11]

Having thought about how to implement the wrapper and after a discussion with
Sami, here's what I think are the alternatives for what we want to achieve.

1) Use wrapper which references the original shader and holds the new local
matrix. Function calls are forwarded to the original shader and the new matrix
is passed in. That would require changing the interface of SkShader to take a
matrix for some (all?) methods. That parameter could be optional (requiring the
shader impl to always check whether to use the parameter or its own matrix) or
mandatory (changing all call sites, passing in getLocalMatrix() most of the
time?). In any way it would complicate the interface and/or the implementation.

2) Alternatively we could have onFoo() methods for each public foo() method,
where onFoo takes an additional matrix parameter. In SkShaderWrapper::onFoo we'd
simply call the original shader's onFoo method and pass in the new matrix. The
public interface would stay the same and the implementations would always use
the matrix parameter. The downside is that this would require having two methods
(foo() and virtual onFoo()) for each public method (at least the ones using the
local matrix).

3) Instead of a wrapper, clone the object and replace the local matrix. Would
require a clone() method for each subclass but no further modifications to the
other methods.

4) If the cost of copying the entire object is too high, we could separate the
data that's never going to change from the local matrix. So the cloned object
could simply reference the never-gonna-change data and just have its own matrix.
Would require some refactoring.

Does this make sense? Can you think of any other options? Which do you prefer?

My vote goes to option 2. Seems the least overhead while keeping the interface
clean.

[Dominik Grewe, 2014-04-25 13:09:35]

On 2014/04/25 11:02:11, Dominik Grewe wrote:
> Having thought about how to implement the wrapper and after a discussion with
> Sami, here's what I think are the alternatives for what we want to achieve.
> 
> 1) Use wrapper which references the original shader and holds the new local
> matrix. Function calls are forwarded to the original shader and the new matrix
> is passed in. That would require changing the interface of SkShader to take a
> matrix for some (all?) methods. That parameter could be optional (requiring
the
> shader impl to always check whether to use the parameter or its own matrix) or
> mandatory (changing all call sites, passing in getLocalMatrix() most of the
> time?). In any way it would complicate the interface and/or the
implementation.
> 
> 2) Alternatively we could have onFoo() methods for each public foo() method,
> where onFoo takes an additional matrix parameter. In SkShaderWrapper::onFoo
we'd
> simply call the original shader's onFoo method and pass in the new matrix. The
> public interface would stay the same and the implementations would always use
> the matrix parameter. The downside is that this would require having two
methods
> (foo() and virtual onFoo()) for each public method (at least the ones using
the
> local matrix).
> 
> 3) Instead of a wrapper, clone the object and replace the local matrix. Would
> require a clone() method for each subclass but no further modifications to the
> other methods.
> 
> 4) If the cost of copying the entire object is too high, we could separate the
> data that's never going to change from the local matrix. So the cloned object
> could simply reference the never-gonna-change data and just have its own
matrix.
> Would require some refactoring.
> 
> Does this make sense? Can you think of any other options? Which do you prefer?
> 
> My vote goes to option 2. Seems the least overhead while keeping the interface
> clean.

The only ugly thing about (2) is that it requires that getLocalMatrix is never
called from within SkShader or its subclasses. If you need the local matrix
there, you'd have to pass it in. Otherwise you'd break the wrapper. Would be
great if we can enforce that somehow, but I don't see how (we can't get rid of
getLocalMatrix).

[scroggo, 2014-04-25 15:06:44]

On 2014/04/25 11:02:11, Dominik Grewe wrote:
> Having thought about how to implement the wrapper and after a discussion with
> Sami, here's what I think are the alternatives for what we want to achieve.
> 
> 1) Use wrapper which references the original shader and holds the new local
> matrix. Function calls are forwarded to the original shader and the new matrix
> is passed in. That would require changing the interface of SkShader to take a
> matrix for some (all?) methods. That parameter could be optional (requiring
the
> shader impl to always check whether to use the parameter or its own matrix) or
> mandatory (changing all call sites, passing in getLocalMatrix() most of the
> time?). In any way it would complicate the interface and/or the
implementation.

I'm guessing it would be cleaner to make it optional.

> 
> 2) Alternatively we could have onFoo() methods for each public foo() method,
> where onFoo takes an additional matrix parameter. In SkShaderWrapper::onFoo
we'd
> simply call the original shader's onFoo method and pass in the new matrix. The
> public interface would stay the same and the implementations would always use
> the matrix parameter. The downside is that this would require having two
methods
> (foo() and virtual onFoo()) for each public method (at least the ones using
the
> local matrix).

This fits with a pattern we use in many other places, so I don't see that as a
downside.

> 
> 3) Instead of a wrapper, clone the object and replace the local matrix. Would
> require a clone() method for each subclass but no further modifications to the
> other methods.

In some ways this is simpler, but it means writing clone functions for all
subclasses. It seems like this would be easy to miss a bug somewhere, and not
catch it if it's not currently used.

> 
> 4) If the cost of copying the entire object is too high, we could separate the
> data that's never going to change from the local matrix. So the cloned object
> could simply reference the never-gonna-change data and just have its own
matrix.
> Would require some refactoring.

I don't have a full picture of this in my head, but it seems like the
refactoring
would be extensive.

> 
> Does this make sense? Can you think of any other options? Which do you prefer?
> 
> My vote goes to option 2. Seems the least overhead while keeping the interface
> clean.

Either 1 or 2 sounds good to me.

> The only ugly thing about (2) is that it requires that getLocalMatrix is never
> called from within SkShader or its subclasses. If you need the local matrix
> there, you'd have to pass it in. Otherwise you'd break the wrapper. Would be
> great if we can enforce that somehow, but I don't see how (we can't get rid of
> getLocalMatrix).

I don't see why we can't get rid of getLocalMatrix. Chrome doesn't appear to use
it. We need to be able to mimic its behavior on Android (and internally,
obviously), but it doesn't need to exist in its current form.

What if the only way to set a local matrix was to use the wrapper, and from the
shader's perspective, the local matrix was always passed in?

ccing mtklein. Mike, we discussed wrapping the shader to replace its local
matrix, but did not discuss how the wrapped shader would be sure to use the
correct one. Any thoughts?

[reed1, 2014-04-25 16:15:25]

1. I'm good with adding an optional LM (local matrix) to the factories.

2. I think I will add an optional LM* to createContext() and asEffect(), in
support of drawVertices. The matrix passed should be internally concatenated
with the shader's notion of its LM (if not null).

3. I expect that Leon will be able to use #2 to help him support Android's
behavior.

4. lgtm

[scroggo, 2014-04-25 16:22:35]

On 2014/04/25 16:15:25, reed1 wrote:
> 1. I'm good with adding an optional LM (local matrix) to the factories.
> 
> 2. I think I will add an optional LM* to createContext() and asEffect(), in
> support of drawVertices. The matrix passed should be internally concatenated
> with the shader's notion of its LM (if not null).
> 
> 3. I expect that Leon will be able to use #2 to help him support Android's
> behavior.
> 
> 4. lgtm

-lgtm. Without 2, the current wrapper does not actually do the right thing.

[scroggo, 2014-04-25 16:22:52]

On 2014/04/25 16:22:35, scroggo wrote:
> On 2014/04/25 16:15:25, reed1 wrote:
> > 1. I'm good with adding an optional LM (local matrix) to the factories.
> > 
> > 2. I think I will add an optional LM* to createContext() and asEffect(), in
> > support of drawVertices. The matrix passed should be internally concatenated
> > with the shader's notion of its LM (if not null).
> > 
> > 3. I expect that Leon will be able to use #2 to help him support Android's
> > behavior.
> > 
> > 4. lgtm
> 
> -lgtm. Without 2, the current wrapper does not actually do the right thing.

not lgtm

[Dominik Grewe, 2014-04-25 16:27:12]

On 2014/04/25 16:22:52, scroggo wrote:
> On 2014/04/25 16:22:35, scroggo wrote:
> > On 2014/04/25 16:15:25, reed1 wrote:
> > > 1. I'm good with adding an optional LM (local matrix) to the factories.
> > > 
> > > 2. I think I will add an optional LM* to createContext() and asEffect(),
in
> > > support of drawVertices. The matrix passed should be internally
concatenated
> > > with the shader's notion of its LM (if not null).
> > > 
> > > 3. I expect that Leon will be able to use #2 to help him support Android's
> > > behavior.
> > > 
> > > 4. lgtm
> > 
> > -lgtm. Without 2, the current wrapper does not actually do the right thing.
> 
> not lgtm

That's right. We either need to fix the wrapper or leave it for now. If we add
an optional LM* to createContext and asEffect we could simply pass that to the
original shader.

[reed1, 2014-04-25 16:28:18]

On 2014/04/25 16:27:12, Dominik Grewe wrote:
> On 2014/04/25 16:22:52, scroggo wrote:
> > On 2014/04/25 16:22:35, scroggo wrote:
> > > On 2014/04/25 16:15:25, reed1 wrote:
> > > > 1. I'm good with adding an optional <span id="abbrex-spnAbbr_484"
class="abbrex-Abbr abbrex-Abbr_LM abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">LM</span> (local matrix) to the factories.
> > > > 
> > > > 2. I think I will add an optional <span id="abbrex-spnAbbr_485"
class="abbrex-Abbr abbrex-Abbr_LM abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">LM</span>* to createContext() and asEffect(),
> in
> > > > support of drawVertices. The matrix passed should be internally
> concatenated
> > > > with the shader's notion of its <span id="abbrex-spnAbbr_486"
class="abbrex-Abbr abbrex-Abbr_LM abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">LM</span> (if not null).
> > > > 
> > > > 3. I expect that Leon will be able to use #2 to help him support
Android's
> > > > behavior.
> > > > 
> > > > 4. lgtm
> > > 
> > > -lgtm. Without 2, the current wrapper does not actually do the right
thing.
> > 
> > not lgtm
> 
> That's right. We either need to fix the wrapper or leave it for now. If we add
> an optional <span id="abbrex-spnAbbr_487" class="abbrex-Abbr abbrex-Abbr_LM
abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">LM</span>* to createContext and
asEffect we could simply pass that to the
> original shader.

I will start on the extra-param CL now (independent of this CL)

[scroggo, 2014-04-25 16:28:58]

On 2014/04/25 16:22:52, scroggo wrote:
> On 2014/04/25 16:22:35, scroggo wrote:
> > On 2014/04/25 16:15:25, reed1 wrote:
> > > 1. I'm good with adding an optional LM (local matrix) to the factories.
> > > 
> > > 2. I think I will add an optional LM* to createContext() and asEffect(),
in
> > > support of drawVertices. The matrix passed should be internally
concatenated
> > > with the shader's notion of its LM (if not null).
> > > 
> > > 3. I expect that Leon will be able to use #2 to help him support Android's
> > > behavior.
> > > 
> > > 4. lgtm
> > 
> > -lgtm. Without 2, the current wrapper does not actually do the right thing.
> 
> not lgtm

I would be fine with just submitting the factory change, and leaving the wrapper
for a separate CL (after Mike/someone adds LM to createContext and asEffect).

(I also still think we should remove the optional parameter from the hidden
constructors that are only called by our factories, which have the meaningful
optional parameters.)

[Dominik Grewe, 2014-04-25 16:29:54]

On 2014/04/25 16:28:58, scroggo wrote:
> On 2014/04/25 16:22:52, scroggo wrote:
> > On 2014/04/25 16:22:35, scroggo wrote:
> > > On 2014/04/25 16:15:25, reed1 wrote:
> > > > 1. I'm good with adding an optional LM (local matrix) to the factories.
> > > > 
> > > > 2. I think I will add an optional LM* to createContext() and asEffect(),
> in
> > > > support of drawVertices. The matrix passed should be internally
> concatenated
> > > > with the shader's notion of its LM (if not null).
> > > > 
> > > > 3. I expect that Leon will be able to use #2 to help him support
Android's
> > > > behavior.
> > > > 
> > > > 4. lgtm
> > > 
> > > -lgtm. Without 2, the current wrapper does not actually do the right
thing.
> > 
> > not lgtm
> 
> I would be fine with just submitting the factory change, and leaving the
wrapper
> for a separate CL (after Mike/someone adds LM to createContext and asEffect).
> 
> (I also still think we should remove the optional parameter from the hidden
> constructors that are only called by our factories, which have the meaningful
> optional parameters.)

Sounds good. I'll remove the wrapper and the optional parameters where possible.

[Dominik Grewe, 2014-04-25 17:50:15]

Currently only those shader classes have an optional LM parameter in their
constructur that need it to compile. Guess it would make sense to add it to
every class though.

https://codereview.chromium.org/245963010/diff/100001/gm/shadertext2.cpp
File gm/shadertext2.cpp (right):

https://codereview.chromium.org/245963010/diff/100001/gm/shadertext2.cpp#newc...
gm/shadertext2.cpp:102: SkShader::kRepeat_TileMode));
I think we can delete this shader here. Wdyt?

https://codereview.chromium.org/245963010/diff/100001/include/core/SkColorSha...
File include/core/SkColorShader.h (right):

https://codereview.chromium.org/245963010/diff/100001/include/core/SkColorSha...
include/core/SkColorShader.h:31: SkColorShader(SkColor c, const SkMatrix*
localMatrix = NULL);
This currently causes conflicts in Chromium because the compiler can't
distinguish between the two constructors when a color is passed. Should go away
once the first constructor has been removed.

[scroggo, 2014-04-25 17:58:32]

lgtm, modulo breaking chrome build.

https://codereview.chromium.org/245963010/diff/100001/gm/shadertext2.cpp
File gm/shadertext2.cpp (right):

https://codereview.chromium.org/245963010/diff/100001/gm/shadertext2.cpp#newc...
gm/shadertext2.cpp:102: SkShader::kRepeat_TileMode));
On 2014/04/25 17:50:15, Dominik Grewe wrote:
> I think we can delete this shader here. Wdyt?

I agree. I can't imagine measureText needs to look at the shader

[reed1, 2014-04-25 18:11:18]

On 2014/04/25 17:58:32, scroggo wrote:
> lgtm, modulo breaking chrome build.
> 
> https://codereview.chromium.org/245963010/diff/100001/gm/shadertext2.cpp
> File gm/shadertext2.cpp (right):
> 
>
https://codereview.chromium.org/245963010/diff/100001/gm/shadertext2.cpp#newc...
> gm/shadertext2.cpp:102: SkShader::kRepeat_TileMode));
> On 2014/04/25 17:50:15, Dominik Grewe wrote:
> > I think we can delete this shader here. Wdyt?
> 
> I agree. I can't imagine measureText needs to look at the shader

1. ColorShader does *NOT* need a local matrix; it will always be ignored.
2. text measurement *NEVER* needs to know the shader.

[Dominik Grewe, 2014-04-28 09:43:04]

I've removed the localMatrix parameter from SkColor's constructor. Chromium is
now building fine (also because the default constructor has been removed).

I've also removed the unused shader in gm/shadertext2.cpp as discussed.

[Dominik Grewe, 2014-04-28 09:58:46]

The CQ bit was checked by dominikg@chromium.org

[commit-bot: I haz the power, 2014-04-28 09:59:05]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/dominikg@chromium.org/245963010/120001

[scroggo, 2014-04-28 13:11:57]

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

https://codereview.chromium.org/245963010/diff/120001/src/core/SkShader.cpp#n...
src/core/SkShader.cpp:262: color = shader.fColor;
nit: no need to separate declaration and assignment for these variables now that
there's no if statement. (And these could even be const.)

[Dominik Grewe, 2014-04-28 13:14:30]

The CQ bit was unchecked by dominikg@chromium.org

[Dominik Grewe, 2014-04-28 13:37:37]

The CQ bit was checked by dominikg@chromium.org

[commit-bot: I haz the power, 2014-04-28 13:37:56]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/dominikg@chromium.org/245963010/140001

[commit-bot: I haz the power, 2014-04-28 14:56:27]

Change committed as 14401