Alter SkXfermode's asFragmentProcessor & asXPFactory contracts

Alter SkXfermode's asFragmentProcessor & asXPFactory contracts

TBR=bsalomon@google.com

GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1674673002

Committed: https://skia.googlesource.com/skia/+/4f0379444db31421894d2fce7c85889fe5eaa01a

[robertphillips, 2016-02-05 15:33:14]

Description was changed from

==========
Alter SkXfermode's asFragmentProcessor & asXPFactory contracts
==========

to

==========
Alter SkXfermode's asFragmentProcessor & asXPFactory contracts
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[robertphillips, 2016-02-05 22:10:12]

Description was changed from

==========
Alter SkXfermode's asFragmentProcessor & asXPFactory contracts
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Alter SkXfermode's asFragmentProcessor & asXPFactory contracts

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[robertphillips, 2016-02-05 22:10:12]

robertphillips@google.com changed reviewers:
+ egdaniel@google.com

[egdaniel, 2016-02-08 15:32:11]

https://codereview.chromium.org/1674673002/diff/40001/include/core/SkXfermode.h
File include/core/SkXfermode.h (right):

https://codereview.chromium.org/1674673002/diff/40001/include/core/SkXfermode...
include/core/SkXfermode.h:203: virtual const GrFragmentProcessor*
asFragmentProcessor(const GrFragmentProcessor* dst) const;
change name to have image filter in name, getFragmnetProcessorForImageFilter

https://codereview.chromium.org/1674673002/diff/40001/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/1674673002/diff/40001/src/core/SkXfermode.cpp...
src/core/SkXfermode.cpp:918: SkASSERT(fMode <= kLastCoeffMode);
I feel like this assert assumes knowledge of exactly what GrPorterDuffXPFactory
will or will not accept. Would it not be safer to do something like:

"return SkAssertResult(GrPDXPF::Create(fMode));"

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

https://codereview.chromium.org/1674673002/diff/40001/src/gpu/SkGr.cpp#newcod...
src/gpu/SkGr.cpp:493: if (xfermode) {
This is the check for srcOver right? So that we keep it null on the GrPaint? If
that is the case I think we may want an assert the before this call the XPF on
GrPaint is null. Along with a comment explaining why this is here?

[robertphillips, 2016-02-08 15:54:54]

https://codereview.chromium.org/1674673002/diff/40001/include/core/SkXfermode.h
File include/core/SkXfermode.h (right):

https://codereview.chromium.org/1674673002/diff/40001/include/core/SkXfermode...
include/core/SkXfermode.h:203: virtual const GrFragmentProcessor*
asFragmentProcessor(const GrFragmentProcessor* dst) const;
On 2016/02/08 15:32:11, egdaniel wrote:
> change name to have image filter in name, getFragmnetProcessorForImageFilter

Done.

https://codereview.chromium.org/1674673002/diff/40001/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/1674673002/diff/40001/src/core/SkXfermode.cpp...
src/core/SkXfermode.cpp:918: SkASSERT(fMode <= kLastCoeffMode);
On 2016/02/08 15:32:11, egdaniel wrote:
> I feel like this assert assumes knowledge of exactly what
GrPorterDuffXPFactory
> will or will not accept. Would it not be safer to do something like:
> 
> "return SkAssertResult(GrPDXPF::Create(fMode));"

Done.

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

https://codereview.chromium.org/1674673002/diff/40001/src/gpu/SkGr.cpp#newcod...
src/gpu/SkGr.cpp:493: if (xfermode) {
On 2016/02/08 15:32:11, egdaniel wrote:
> This is the check for srcOver right? So that we keep it null on the GrPaint?
If
> that is the case I think we may want an assert the before this call the XPF on
> GrPaint is null. Along with a comment explaining why this is here?

Done.

[egdaniel, 2016-02-08 16:09:03]

lgtm with comment nit

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

https://codereview.chromium.org/1674673002/diff/60001/src/gpu/SkGr.cpp#newcod...
src/gpu/SkGr.cpp:497: // SafeUnref b.c. kSrcOver_Mode will return a null factory
Nit: it is not that kSrcOver_Mode will return a null factory (I actually don't
think it will), but just the general fact that we allow any xfermode to return
null and we will simply fall back to srcOver in that case.

[robertphillips, 2016-02-08 16:24:15]

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

https://codereview.chromium.org/1674673002/diff/60001/src/gpu/SkGr.cpp#newcod...
src/gpu/SkGr.cpp:497: // SafeUnref b.c. kSrcOver_Mode will return a null factory
On 2016/02/08 16:09:03, egdaniel wrote:
> Nit: it is not that kSrcOver_Mode will return a null factory (I actually don't
> think it will), but just the general fact that we allow any xfermode to return
> null and we will simply fall back to srcOver in that case.

Done.

[robertphillips, 2016-02-08 16:26:37]

The CQ bit was checked by robertphillips@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-08 16:26:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674673002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674673002/80001

[commit-bot: I haz the power, 2016-02-08 16:29:25]

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

[commit-bot: I haz the power, 2016-02-08 16:29:25]

Dry run: Try jobs failed on following builders:
  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...)

[robertphillips, 2016-02-08 16:40:59]

The CQ bit was checked by robertphillips@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-08 16:41:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674673002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674673002/100001

[robertphillips, 2016-02-08 16:46:21]

robertphillips@google.com changed reviewers:
+ bsalomon@google.com

[commit-bot: I haz the power, 2016-02-08 16:50:17]

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

[commit-bot: I haz the power, 2016-02-08 16:50:18]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)

[robertphillips, 2016-02-08 20:51:35]

The CQ bit was checked by robertphillips@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-08 20:51:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674673002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674673002/120001

[robertphillips, 2016-02-08 21:11:22]

Description was changed from

==========
Alter SkXfermode's asFragmentProcessor & asXPFactory contracts

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Alter SkXfermode's asFragmentProcessor & asXPFactory contracts

TBR=bsalomon@google.com

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[commit-bot: I haz the power, 2016-02-08 22:00:17]

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

[commit-bot: I haz the power, 2016-02-08 22:00:18]

Dry run: This issue passed the CQ dry run.

[robertphillips, 2016-02-09 13:08:32]

The CQ bit was checked by robertphillips@google.com

[robertphillips, 2016-02-09 13:08:32]

The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com
Link to the patchset: https://codereview.chromium.org/1674673002/#ps120001
(title: "Fix unit test bug")

[commit-bot: I haz the power, 2016-02-09 13:08:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674673002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674673002/120001

[commit-bot: I haz the power, 2016-02-09 13:09:32]

Description was changed from

==========
Alter SkXfermode's asFragmentProcessor & asXPFactory contracts

TBR=bsalomon@google.com

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Alter SkXfermode's asFragmentProcessor & asXPFactory contracts

TBR=bsalomon@google.com

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/4f0379444db31421894d2fce7c85889fe5eaa01a
==========

[commit-bot: I haz the power, 2016-02-09 13:09:33]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/4f0379444db31421894d2fce7c85889fe5eaa01a