Move skPaint2GrPaint to SkGr.h/cpp

Move skPaint2GrPaint to SkGr.h/cpp

BUG=skia:

Committed: http://code.google.com/p/skia/source/detail?r=14753

[egdaniel, 2014-05-13 15:48:38]

ptal

[brian1, 2014-05-13 16:14:28]

Lgtm, but could you remove the outdated comment about returning SkShader's
texture in the shader version?

[robertphillips, 2014-05-13 16:24:10]

https://codereview.chromium.org/283803003/diff/2/include/gpu/SkGr.h
File include/gpu/SkGr.h (right):

https://codereview.chromium.org/283803003/diff/2/include/gpu/SkGr.h#newcode81
include/gpu/SkGr.h:81: 
converts -> Converts ?
"skPaint's shader" -> "SkPaint's shader." ?
Add "Return true on success; false on failure. Upon failure grPaint ..." ?

https://codereview.chromium.org/283803003/diff/2/include/gpu/SkGr.h#newcode92
include/gpu/SkGr.h:92: // skPaint's shader to a GrTexture/GrEffectStage if
possible. The texture to
There is no param 'act'
Add "Return true on success; false on failure. Upon failure grPaint ..." ?

https://codereview.chromium.org/283803003/diff/2/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/283803003/diff/2/src/gpu/SkGr.cpp#newcode349
src/gpu/SkGr.cpp:349: //SkDEBUGCODE(SkDebugf("Unsupported xfer mode.\n");)
What's up with this? All caller seem to handle the return type. In which cases
can it fail?

https://codereview.chromium.org/283803003/diff/2/src/gpu/SkGr.cpp#newcode405
src/gpu/SkGr.cpp:405: } else {
Do we have a TODO for this? Is it something an intern could do?

[egdaniel, 2014-05-13 17:35:54]

https://codereview.chromium.org/283803003/diff/2/include/gpu/SkGr.h
File include/gpu/SkGr.h (right):

https://codereview.chromium.org/283803003/diff/2/include/gpu/SkGr.h#newcode81
include/gpu/SkGr.h:81: 
As of now we always fall back to src-over for the xfermode and never return
false. Does it still make sense for this function to even return a bool instead
of being void? In the Shader version, we only return false if we have
SkColorShader. Once that is implemented at some point, we really don't need a
return value on either of these functions.

On 2014/05/13 16:24:11, robertphillips wrote:
> converts -> Converts ?
> "skPaint's shader" -> "SkPaint's shader." ?
> Add "Return true on success; false on failure. Upon failure grPaint ..." ?

https://codereview.chromium.org/283803003/diff/2/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/283803003/diff/2/src/gpu/SkGr.cpp#newcode349
src/gpu/SkGr.cpp:349: //SkDEBUGCODE(SkDebugf("Unsupported xfer mode.\n");)
I tried following AsNewEffectOrCoeff to see what fails, and you basically run
into having to look at all the subclasses asNewEffect implementations to see if
they can fail. Brian do you know what our fail cases are off the top of your
head, so we can remove the return false path and just add a comment for what
cases we fall back to src-over for?
On 2014/05/13 16:24:11, robertphillips wrote:
> What's up with this? All caller seem to handle the return type. In which cases
> can it fail?

https://codereview.chromium.org/283803003/diff/2/src/gpu/SkGr.cpp#newcode405
src/gpu/SkGr.cpp:405: } else {
On 2014/05/13 16:24:11, robertphillips wrote:
> Do we have a TODO for this? Is it something an intern could do?
Getting this implemented should allow us to remove any need for return value
(assuming we fall back to src-over in the NoShader function).

[bsalomon, 2014-05-13 20:34:29]

https://codereview.chromium.org/283803003/diff/2/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/283803003/diff/2/src/gpu/SkGr.cpp#newcode349
src/gpu/SkGr.cpp:349: //SkDEBUGCODE(SkDebugf("Unsupported xfer mode.\n");)
On 2014/05/13 17:35:54, egdaniel wrote:
> I tried following AsNewEffectOrCoeff to see what fails, and you basically run
> into having to look at all the subclasses asNewEffect implementations to see
if
> they can fail. Brian do you know what our fail cases are off the top of your
> head, so we can remove the return false path and just add a comment for what
> cases we fall back to src-over for?
> On 2014/05/13 16:24:11, robertphillips wrote:
> > What's up with this? All caller seem to handle the return type. In which
cases
> > can it fail?
> 

I think we should just remove the return false. We don't similarly fail if other
pipeline stages fail to turn themselves into GrEffects. E.g. the color filter
code below.

https://codereview.chromium.org/283803003/diff/2/src/gpu/SkGr.cpp#newcode405
src/gpu/SkGr.cpp:405: } else {
On 2014/05/13 17:35:54, egdaniel wrote:
> On 2014/05/13 16:24:11, robertphillips wrote:
> > Do we have a TODO for this? Is it something an intern could do?
> Getting this implemented should allow us to remove any need for return value
> (assuming we fall back to src-over in the NoShader function).

Same argument here. Let's just make the function void. If we want to give
feedback to the client we can use the error reporting mechanism.

I'm not sure we really even want SkColorShader to return a GrEffect. Perhaps the
signature on SkShader::asNewEffect could allow the SkShader to override the
SkPaint's color(). Maybe this could remove the need for justAlpha? Or maybe the
color array on drawVertices() complicates removing it? Anyway, it's orthogonal
to this code move.

[robertphillips, 2014-05-14 14:58:20]

lgtm

https://codereview.chromium.org/283803003/diff/40001/include/gpu/SkGr.h
File include/gpu/SkGr.h (right):

https://codereview.chromium.org/283803003/diff/40001/include/gpu/SkGr.h#newco...
include/gpu/SkGr.h:82: // Converts a SkPaint to a GrPaint, ignoring the
SkPaint's shader.
(e.g., a,a,a,a)  ?

https://codereview.chromium.org/283803003/diff/40001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/283803003/diff/40001/src/gpu/SkGr.cpp#newcode390
src/gpu/SkGr.cpp:390: // SkShader::asNewEffect() may do offscreen rendering.
Setup default drawing state and require
Extra space before '.' ?

https://codereview.chromium.org/283803003/diff/40001/src/gpu/SkGr.cpp#newcode415
src/gpu/SkGr.cpp:415: }
Do we need a:

else {
    SkPaint2GrPaintNoShader(dev, skPaint, false, constantColor, grPaint);
}

[egdaniel, 2014-05-15 16:38:01]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2014-05-15 16:38:36]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/egdaniel@google.com/283803003/60001

[commit-bot: I haz the power, 2014-05-15 16:38:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-15 16:38:40]

Presubmit check for 283803003-60001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com')

[bsalomon, 2014-05-15 20:25:25]

On 2014/05/15 16:38:40, I haz the power (commit-bot) wrote:
> Presubmit check for 283803003-60001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Since the CL is editing public API, you must have an LGTM from one of:
> (mailto:, mailto:, mailto:,
> mailto:)

lgtm

[bsalomon, 2014-05-15 20:25:25]

On 2014/05/15 16:38:40, I haz the power (commit-bot) wrote:
> Presubmit check for 283803003-60001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Since the CL is editing public API, you must have an LGTM from one of:
> (mailto:, mailto:, mailto:,
> mailto:)

lgtm

[egdaniel, 2014-05-15 20:26:49]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2014-05-15 20:27:02]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/egdaniel@google.com/283803003/60001

[commit-bot: I haz the power, 2014-05-15 20:32:51]

Change committed as 14753