Adding more validation

Adding more validation

- Added a way to check that the number of inputs of a filter is not more than a filter expects
- Added validation of reftype in SkBitmap::unflatten()
- Added validation on fKD (diffuse lighting constant) and fKS (specular lighting constant) to make sure that they are always non-negative numbers
- Added validation of SkPerlinNoiseShader::fType and SkPerlinNoiseShader::fNumOctaves

BUG=

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

[reed1, 2013-11-22 20:04:57]

https://codereview.chromium.org/83343003/diff/1/include/core/SkImageFilter.h
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/83343003/diff/1/include/core/SkImageFilter.h#...
include/core/SkImageFilter.h:149: 
Don't some filters have 0 inputs?
Not sure why we need an enum here?

[sugoi1, 2013-11-22 20:15:52]

https://codereview.chromium.org/83343003/diff/1/include/core/SkImageFilter.h
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/83343003/diff/1/include/core/SkImageFilter.h#...
include/core/SkImageFilter.h:149: 
On 2013/11/22 20:04:58, reed1 wrote:
> Don't some filters have 0 inputs?
Yes, but there are no 0 input constructors, and I was trying to follow those. I
think a single NULL input still counts as 1 input for serialization purposes (I
could be wrong).

> Not sure why we need an enum here?
No particular reason, it's just an easy way to define constants. I can use
"static const int" or "#define" or something else, whichever is preferred.

[reed1, 2013-11-22 20:28:46]

I mean to ask, why not pass 1, 2 etc. instead of kOne, kTwo, etc. ? Do we really
care about compiler-checking for this value? If we create a new filter that
takes 3, must we update the enum? Mostly curious why it occured to you to use an
enum-like-thing at all for a count.

[Stephen White, 2013-11-22 20:52:11]

On 2013/11/22 20:28:46, reed1 wrote:
> I mean to ask, why not pass 1, 2 etc. instead of kOne, kTwo, etc. ? Do we
really
> care about compiler-checking for this value? If we create a new filter that
> takes 3, must we update the enum? Mostly curious why it occured to you to use
an
> enum-like-thing at all for a count.

+1 to just using an int. That's what the non-flattenable constructor takes:

    SkImageFilter(int inputCount, SkImageFilter** inputs, const CropRect*
cropRect = NULL);

[Stephen White, 2013-11-22 21:00:17]

https://codereview.chromium.org/83343003/diff/230001/src/effects/SkBitmapSour...
File src/effects/SkBitmapSource.cpp (right):

https://codereview.chromium.org/83343003/diff/230001/src/effects/SkBitmapSour...
src/effects/SkBitmapSource.cpp:16: 
This should probably zero inputs. In fact, the regular constructor should
probably be calling SkImageFilter(0, 0), instead of SkImageFilter(0).

(On a tangential note, we could probably rid of the one-arg constructor on the
base class, and call SkImageFilter(0, &arg) from the derived class instead.
Slightly harder to get rid of the two-arg version, sadly.)

https://codereview.chromium.org/83343003/diff/230001/src/effects/SkPerlinNois...
File src/effects/SkPerlinNoiseShader.cpp (right):

https://codereview.chromium.org/83343003/diff/230001/src/effects/SkPerlinNois...
src/effects/SkPerlinNoiseShader.cpp:316: 
Could we express this as decimal 255, unless there's a strong reason for it to
be hex?  Maybe it should be a constant at the top of the file? (Doesn't seem to
be in the spec -- I assume it's a limitation of this implementation.)

Now that I look at it, I see the regular constructor is and'ing the octaves with
0xFF, which seems kinda weird to me. Should probably just be octaves > 255 ? 255
: 0, no?

[sugoi, 2013-11-25 15:53:12]

Fixed comments

[Stephen White, 2013-11-25 16:11:21]

https://codereview.chromium.org/83343003/diff/410001/src/effects/SkBitmapSour...
File src/effects/SkBitmapSource.cpp (right):

https://codereview.chromium.org/83343003/diff/410001/src/effects/SkBitmapSour...
src/effects/SkBitmapSource.cpp:16: : INHERITED(1, buffer) {
I'm pretty sure this should be zero (and the other constructor call fixed to be
0, 0).

https://codereview.chromium.org/83343003/diff/410001/src/effects/SkPerlinNois...
File src/effects/SkPerlinNoiseShader.cpp (right):

https://codereview.chromium.org/83343003/diff/410001/src/effects/SkPerlinNois...
src/effects/SkPerlinNoiseShader.cpp:55: switch (type) {
Nit: this could be:

return type == SkPerlinNoiseShader::kFractalNoise_Type 
    || type == SkPerlinNoiseShader::kTurbulence_Type;

[sugoi, 2013-11-25 16:20:32]

https://codereview.chromium.org/83343003/diff/410001/src/effects/SkBitmapSour...
File src/effects/SkBitmapSource.cpp (right):

https://codereview.chromium.org/83343003/diff/410001/src/effects/SkBitmapSour...
src/effects/SkBitmapSource.cpp:16: : INHERITED(1, buffer) {
On 2013/11/25 16:11:21, Stephen White wrote:
> I'm pretty sure this should be zero (and the other constructor call fixed to
be
> 0, 0).

Done.

https://codereview.chromium.org/83343003/diff/410001/src/effects/SkPerlinNois...
File src/effects/SkPerlinNoiseShader.cpp (right):

https://codereview.chromium.org/83343003/diff/410001/src/effects/SkPerlinNois...
src/effects/SkPerlinNoiseShader.cpp:55: switch (type) {
On 2013/11/25 16:11:21, Stephen White wrote:
> Nit: this could be:
> 
> return type == SkPerlinNoiseShader::kFractalNoise_Type 
>     || type == SkPerlinNoiseShader::kTurbulence_Type;

Done.

[Stephen White, 2013-11-25 16:32:58]

LGTM

[commit-bot: I haz the power, 2013-11-25 16:35:22]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/83343003/430001

[commit-bot: I haz the power, 2013-11-25 16:35:33]

Presubmit check for 83343003-430001 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')

Presubmit checks took 1.4s to calculate.

[reed1, 2013-11-25 20:49:45]

lgtm

[commit-bot: I haz the power, 2013-11-25 20:50:19]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/83343003/430001

[commit-bot: I haz the power, 2013-11-25 21:06:25]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/83343003/430001

[commit-bot: I haz the power, 2013-11-25 21:46:56]

Change committed as 12388