More genericity: overload isPaintOpaque(SkPaint, SkBitmap)

More genericity: overload isPaintOpaque(SkPaint, SkBitmap)

Instead of taking a Bitmap as an argument this version
takes only the type of content (None/Opaque/Transparent).
This will be used to check the opaqueness of a SkPaint
that draws a SkImage.

BUG=skia:3042
Committed: https://skia.googlesource.com/skia/+/46c248da267d315d500d2296a205cf22077b3e87

[Rémi Piotaix, 2014-10-20 20:32:08]

piotaixr@chromium.org changed reviewers:
+ junov@chromium.org, reed@google.com

[Rémi Piotaix, 2014-10-20 20:33:37]

PTAL

This CL will be used to resubmit  https://codereview.chromium.org/613673005/

[reed1, 2014-10-20 21:07:08]

1. Can you add dox per line of the enum, explaining what each means?
2. nit: global enums have their full type as their suffix: e.g.
kOpaque_SkPaintOpaquenessResult

[Justin Novosad, 2014-10-20 21:19:07]

On 2014/10/20 21:07:08, reed1 wrote:
> 2. nit: global enums have their full type as their suffix: e.g.
> kOpaque_SkPaintOpaquenessResult

Also, "Tranparent is not the opposite of opaque. You should use "NonOpaque".
Opaque: alpha == 1.0 for all pixels
Transparent: alpha == 0 for all pixels
NonOpaque: alpha < 1.0 for one or more pixels


I would suggest something like:
enum SkPaintBitmapOpacity {
   kNoBitmap_SkPaintBitmapOpacity = 0,
   kOpaque_SkPaintBitmapOpacity = 1,
   kNonOpaque_SkPaintBitmapOpacity = 2
};

[reed1, 2014-10-21 13:15:18]

On 2014/10/20 21:19:07, junov wrote:
> On 2014/10/20 21:07:08, reed1 wrote:
> > 2. nit: global enums have their full type as their suffix: e.g.
> > kOpaque_SkPaintOpaquenessResult
> 
> Also, "Tranparent is not the opposite of opaque. You should use "NonOpaque".
> Opaque: alpha == 1.0 for all pixels
> Transparent: alpha == 0 for all pixels
> NonOpaque: alpha < 1.0 for one or more pixels
> 
> 
> I would suggest something like:
> enum SkPaintBitmapOpacity {
>    kNoBitmap_SkPaintBitmapOpacity = 0,
>    kOpaque_SkPaintBitmapOpacity = 1,
>    kNonOpaque_SkPaintBitmapOpacity = 2
> };

Or kUnknown_... for the state when it is not known to be opaque, and not known
to be transparent.

[Rémi Piotaix, 2014-10-21 19:35:20]

On 2014/10/20 21:07:08, reed1 wrote:
> 1. Can you add dox per line of the enum, explaining what each means?
Done.

> 2. nit: global enums have their full type as their suffix: e.g.
> kOpaque_SkPaintOpaquenessResult
Done.

On 2014/10/20 21:19:07, junov wrote:
> Also, "Tranparent is not the opposite of opaque. You should use "NonOpaque".
> Opaque: alpha == 1.0 for all pixels
> Transparent: alpha == 0 for all pixels
> NonOpaque: alpha < 1.0 for one or more pixels
Done.

[reed1, 2014-10-21 19:44:06]

https://codereview.chromium.org/663233002/diff/40001/src/core/SkPaintPriv.h
File src/core/SkPaintPriv.h (right):

https://codereview.chromium.org/663233002/diff/40001/src/core/SkPaintPriv.h#n...
src/core/SkPaintPriv.h:16: enum SkPaintBitmapOpacity {
The comments here seem to focus in on "color", but the predicate is really about
what "drawing with the paint" will do, yes?

another bikeshed: why PaintBitmapOpacity? Why not just PaintOpacity?

https://codereview.chromium.org/663233002/diff/40001/src/core/SkPaintPriv.h#n...
src/core/SkPaintPriv.h:17: // No content replaces the paint's color
why are these explicitly numbered? In general we don't do that for skia's enums.

https://codereview.chromium.org/663233002/diff/40001/src/core/SkPaintPriv.h#n...
src/core/SkPaintPriv.h:18: kNoBitmap_SkPaintBitmapOpacity = 0,
Hmmm, kNoBitmap? I definitely find kTransparent_ very clear (esp. when it is
along side kOpaque).

[Rémi Piotaix, 2014-10-21 20:01:32]

https://codereview.chromium.org/663233002/diff/40001/src/core/SkPaintPriv.h
File src/core/SkPaintPriv.h (right):

https://codereview.chromium.org/663233002/diff/40001/src/core/SkPaintPriv.h#n...
src/core/SkPaintPriv.h:16: enum SkPaintBitmapOpacity {
On 2014/10/21 19:44:05, reed1 wrote:
> The comments here seem to focus in on "color", but the predicate is really
about
> what "drawing with the paint" will do, yes?
Yes. We need to have this parameter because, is some cases, the opacity of the
thing that will be drawn does not depends on the alpha value of the paint but on
the alpha of the thing that is drawn with the paint.

> 
> another bikeshed: why PaintBitmapOpacity? Why not just PaintOpacity?
The enum is not really about the opacity of the paint itself, so PaintOpacity is
maybe not the good choice.
PaintBitmapOpacity is not really better because i am doing this change to be
able to call isPaintOpaque() without a bitmap (a SkImage in this case)

https://codereview.chromium.org/663233002/diff/40001/src/core/SkPaintPriv.h#n...
src/core/SkPaintPriv.h:17: // No content replaces the paint's color
On 2014/10/21 19:44:05, reed1 wrote:
> why are these explicitly numbered? In general we don't do that for skia's
enums.
Yes, i forgot to clean it :s

Ack

https://codereview.chromium.org/663233002/diff/40001/src/core/SkPaintPriv.h#n...
src/core/SkPaintPriv.h:18: kNoBitmap_SkPaintBitmapOpacity = 0,
On 2014/10/21 19:44:05, reed1 wrote:
> Hmmm, kNoBitmap? I definitely find kTransparent_ very clear (esp. when it is
> along side kOpaque).

The kNoBitmap is because it happens that the isPaintOpaque is called with
bitmap=NULL.

For the 2 other values, the thing is that the isOpaque() method returns true
only if we are 100% sure that the image(or bitmap) is opaque. So, having
isOpaque()==false does not means that the bitmap is transparent.

[reed1, 2014-10-21 20:43:36]

Ah! I'm an idiot. I was totally misunderstanding this API. Sorry.

[reed1, 2014-10-21 20:44:54]

Now I (think I) understand it.

lgtm

sorry for the churn.

[Rémi Piotaix, 2014-10-21 21:19:23]

On 2014/10/21 20:44:54, reed1 wrote:
> Now I (think I) understand it.
> 
> lgtm
> 
> sorry for the churn.

No problem.

I'd still like to replace 'Bitmap' by 'Content' (or something else) because the
main idea is to be able to call the function "without" a SkBitmap.
What do you think about that?

[reed1, 2014-10-21 21:56:59]

I hereby remove myself for all further bikesheding :)
Deferring to juno

[Rémi Piotaix, 2014-10-22 15:26:05]

The CQ bit was checked by piotaixr@chromium.org

[commit-bot: I haz the power, 2014-10-22 15:27:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/663233002/80001

[Justin Novosad, 2014-10-22 15:27:46]

On 2014/10/21 21:56:59, reed1 wrote:
> I hereby remove myself for all further bikesheding :)
> Deferring to juno

I think "content" is too vague. We are referring specifically to SkBitmap and
SkImage.
Bitmap without the "Sk" would be fine IMO.

[Justin Novosad, 2014-10-22 15:28:12]

lgtm

[commit-bot: I haz the power, 2014-10-22 15:35:50]

Committed patchset #5 (id:80001) as 46c248da267d315d500d2296a205cf22077b3e87