Deleted SkImage::newShaderClamp: not used, not implemented.

Deleted SkImage::newShaderClamp: not used, not implemented.

Implemented SkImage::newShader.

BUG=skia:2701, 344804
Committed: https://skia.googlesource.com/skia/+/cef04f818820e59fb534eb226f967f72497845d3

[Rémi Piotaix, 2014-06-27 21:49:37]

PTAL

[Justin Novosad, 2014-07-02 15:31:09]

Please add a test for this.

https://codereview.chromium.org/345463009/diff/1/src/image/SkImage_Codec.cpp
File src/image/SkImage_Codec.cpp (right):

https://codereview.chromium.org/345463009/diff/1/src/image/SkImage_Codec.cpp#...
src/image/SkImage_Codec.cpp:24: SkShader* newShader(SkShader::TileMode,
SkShader::TileMode) const;
missing "virtual".
Also, please use the same pattern as the other methods (call this "onNewShader",
overloaded from SkImage_Base)

https://codereview.chromium.org/345463009/diff/1/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/345463009/diff/1/src/image/SkImage_Gpu.cpp#ne...
src/image/SkImage_Gpu.cpp:30: SkShader* newShader(SkShader::TileMode,
SkShader::TileMode) const;
same here

https://codereview.chromium.org/345463009/diff/1/src/image/SkImage_Raster.cpp
File src/image/SkImage_Raster.cpp (right):

https://codereview.chromium.org/345463009/diff/1/src/image/SkImage_Raster.cpp...
src/image/SkImage_Raster.cpp:67: SkShader* newShader(SkShader::TileMode,
SkShader::TileMode) const;
and here

[reed1, 2014-07-02 15:38:51]

if we really have no callers, lets just delete the API entrypoints.

[Justin Novosad, 2014-07-02 15:41:31]

On 2014/07/02 15:38:51, reed1 wrote:
> if we really have no callers, lets just delete the API entrypoints.

The headline is misleading. This CL does delete newShaderClamp, but it
implements newShader, which will be needed to fix an issue in blink.

[reed1, 2014-07-02 15:42:56]

can we add a link to the related blink issue/request?

[Rémi Piotaix, 2014-07-02 17:44:13]

Link to the blink issue added in the description.

[Rémi Piotaix, 2014-07-02 20:13:18]

https://codereview.chromium.org/345463009/diff/1/src/image/SkImage_Codec.cpp
File src/image/SkImage_Codec.cpp (right):

https://codereview.chromium.org/345463009/diff/1/src/image/SkImage_Codec.cpp#...
src/image/SkImage_Codec.cpp:24: SkShader* newShader(SkShader::TileMode,
SkShader::TileMode) const;
On 2014/07/02 15:31:08, junov wrote:
> missing "virtual".
> Also, please use the same pattern as the other methods (call this
"onNewShader",
> overloaded from SkImage_Base)

Done.

https://codereview.chromium.org/345463009/diff/1/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/345463009/diff/1/src/image/SkImage_Gpu.cpp#ne...
src/image/SkImage_Gpu.cpp:30: SkShader* newShader(SkShader::TileMode,
SkShader::TileMode) const;
On 2014/07/02 15:31:09, junov wrote:
> same here

Done.

https://codereview.chromium.org/345463009/diff/1/src/image/SkImage_Raster.cpp
File src/image/SkImage_Raster.cpp (right):

https://codereview.chromium.org/345463009/diff/1/src/image/SkImage_Raster.cpp...
src/image/SkImage_Raster.cpp:67: SkShader* newShader(SkShader::TileMode,
SkShader::TileMode) const;
On 2014/07/02 15:31:09, junov wrote:
> and here

Done.

[Justin Novosad, 2014-07-02 20:17:08]

Test?

[Rémi Piotaix, 2014-07-03 22:10:10]

On 2014/07/02 20:17:08, junov wrote:
> Test?

First test uploaded, any comments ? (i'm not sure if i am doing it right)

[Rémi Piotaix, 2014-07-07 14:57:35]

New tests added.

[Justin Novosad, 2014-07-10 19:24:36]

https://codereview.chromium.org/345463009/diff/60001/src/image/SkImage_Codec.cpp
File src/image/SkImage_Codec.cpp (right):

https://codereview.chromium.org/345463009/diff/60001/src/image/SkImage_Codec....
src/image/SkImage_Codec.cpp:43: SkShader*
SkImage_Codec::onNewShader(SkShader::TileMode tileX, SkShader::TileMode tileY)
const {
Do you have test coverage for this guy?

https://codereview.chromium.org/345463009/diff/60001/tests/ImageNewShaderTest...
File tests/ImageNewShaderTest.cpp (right):

https://codereview.chromium.org/345463009/diff/60001/tests/ImageNewShaderTest...
tests/ImageNewShaderTest.cpp:8: #include "SkImage.h"
Alphabetical order please.

https://codereview.chromium.org/345463009/diff/60001/tests/ImageNewShaderTest...
tests/ImageNewShaderTest.cpp:12: #include "GrContextFactory.h"
The include of GrContextFactory should be guarded by #if SK_SUPPORT_GPU

https://codereview.chromium.org/345463009/diff/60001/tests/ImageNewShaderTest...
tests/ImageNewShaderTest.cpp:19: REPORTER_ASSERT(reporter, 5 >= x && 5 >= y);
Where does the 5 come from? I don't think this is an interesting check.

https://codereview.chromium.org/345463009/diff/60001/tests/ImageNewShaderTest...
tests/ImageNewShaderTest.cpp:20: REPORTER_ASSERT(reporter, bm1.getColor(x, y) ==
bm2.getColor(x, y));
This is OK, but it would be more efficient to do a memcmp of bitmap.getPixels()

https://codereview.chromium.org/345463009/diff/60001/tests/ImageNewShaderTest...
tests/ImageNewShaderTest.cpp:37: canvasDest->drawPaint(paint);
To make the test more robust, consider clearing the destination before painting
to it.

https://codereview.chromium.org/345463009/diff/60001/tests/ImageNewShaderTest...
tests/ImageNewShaderTest.cpp:88: DEF_GPUTEST(ImageNewSHader_GPU, reporter,
factory) {
"H" -> "h"

[Rémi Piotaix, 2014-07-10 20:16:04]

New patch

https://codereview.chromium.org/345463009/diff/60001/src/image/SkImage_Codec.cpp
File src/image/SkImage_Codec.cpp (right):

https://codereview.chromium.org/345463009/diff/60001/src/image/SkImage_Codec....
src/image/SkImage_Codec.cpp:43: SkShader*
SkImage_Codec::onNewShader(SkShader::TileMode tileX, SkShader::TileMode tileY)
const {
On 2014/07/10 19:24:36, junov wrote:
> Do you have test coverage for this guy?

It seems that SkImage_Codec is never used...
Method onNewShader removed from SkImage_Codec + default implementation returns
NULL.

https://codereview.chromium.org/345463009/diff/60001/tests/ImageNewShaderTest...
File tests/ImageNewShaderTest.cpp (right):

https://codereview.chromium.org/345463009/diff/60001/tests/ImageNewShaderTest...
tests/ImageNewShaderTest.cpp:8: #include "SkImage.h"
On 2014/07/10 19:24:36, junov wrote:
> Alphabetical order please.

Done.

https://codereview.chromium.org/345463009/diff/60001/tests/ImageNewShaderTest...
tests/ImageNewShaderTest.cpp:12: #include "GrContextFactory.h"
On 2014/07/10 19:24:36, junov wrote:
> The include of GrContextFactory should be guarded by #if SK_SUPPORT_GPU

Done.

https://codereview.chromium.org/345463009/diff/60001/tests/ImageNewShaderTest...
tests/ImageNewShaderTest.cpp:19: REPORTER_ASSERT(reporter, 5 >= x && 5 >= y);
On 2014/07/10 19:24:36, junov wrote:
> Where does the 5 come from? I don't think this is an interesting check.

Done.

https://codereview.chromium.org/345463009/diff/60001/tests/ImageNewShaderTest...
tests/ImageNewShaderTest.cpp:20: REPORTER_ASSERT(reporter, bm1.getColor(x, y) ==
bm2.getColor(x, y));
On 2014/07/10 19:24:36, junov wrote:
> This is OK, but it would be more efficient to do a memcmp of
bitmap.getPixels()

Done.

https://codereview.chromium.org/345463009/diff/60001/tests/ImageNewShaderTest...
tests/ImageNewShaderTest.cpp:37: canvasDest->drawPaint(paint);
On 2014/07/10 19:24:36, junov wrote:
> To make the test more robust, consider clearing the destination before
painting
> to it.

Done.

https://codereview.chromium.org/345463009/diff/60001/tests/ImageNewShaderTest...
tests/ImageNewShaderTest.cpp:88: DEF_GPUTEST(ImageNewSHader_GPU, reporter,
factory) {
On 2014/07/10 19:24:36, junov wrote:
> "H" -> "h"

Done.

[Justin Novosad, 2014-07-11 15:13:55]

https://codereview.chromium.org/345463009/diff/80001/src/image/SkImage_Codec.cpp
File src/image/SkImage_Codec.cpp (left):

https://codereview.chromium.org/345463009/diff/80001/src/image/SkImage_Codec....
src/image/SkImage_Codec.cpp:23: 
Revert this whitespace change please

https://codereview.chromium.org/345463009/diff/80001/tests/ImageNewShaderTest...
File tests/ImageNewShaderTest.cpp (right):

https://codereview.chromium.org/345463009/diff/80001/tests/ImageNewShaderTest...
tests/ImageNewShaderTest.cpp:20: REPORTER_ASSERT(reporter, 0 ==
memcmp(bm1.getPixels(), bm2.getPixels(), bm1.getSize()));
I don't understand why this does not crash. You have to lock pixels before
calling getPixels on a texture.

[Rémi Piotaix, 2014-07-11 15:33:51]

New patch

https://codereview.chromium.org/345463009/diff/80001/src/image/SkImage_Codec.cpp
File src/image/SkImage_Codec.cpp (left):

https://codereview.chromium.org/345463009/diff/80001/src/image/SkImage_Codec....
src/image/SkImage_Codec.cpp:23: 
On 2014/07/11 15:13:55, junov wrote:
> Revert this whitespace change please

Done.

https://codereview.chromium.org/345463009/diff/80001/tests/ImageNewShaderTest...
File tests/ImageNewShaderTest.cpp (right):

https://codereview.chromium.org/345463009/diff/80001/tests/ImageNewShaderTest...
tests/ImageNewShaderTest.cpp:20: REPORTER_ASSERT(reporter, 0 ==
memcmp(bm1.getPixels(), bm2.getPixels(), bm1.getSize()));
On 2014/07/11 15:13:55, junov wrote:
> I don't understand why this does not crash. You have to lock pixels before
> calling getPixels on a texture.

Done.

[Justin Novosad, 2014-07-11 16:50:04]

On 2014/07/11 15:33:51, Rémi Piotaix wrote:

lgtm

[Rémi Piotaix, 2014-07-11 16:59:15]

The CQ bit was checked by piotaixr@chromium.org

[commit-bot: I haz the power, 2014-07-11 17:00:17]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/piotaixr@chromium.org/345463009/100001

[commit-bot: I haz the power, 2014-07-11 17:00:21]

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

[commit-bot: I haz the power, 2014-07-11 17:00:21]

Presubmit check for 345463009-100001 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')

[reed1, 2014-07-12 11:29:51]

api lgtm

[Rémi Piotaix, 2014-07-14 14:30:39]

The CQ bit was checked by piotaixr@chromium.org

[commit-bot: I haz the power, 2014-07-14 14:30:54]

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

[commit-bot: I haz the power, 2014-07-14 14:48:09]

Change committed as cef04f818820e59fb534eb226f967f72497845d3