remove unused (by clients) SkUnitMapper

remove unused (by clients) SkUnitMapper

BUG=skia:

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

[reed1, 2014-05-14 22:48:37]

have to fix the one #if 0 in flattening for gradients...

[robertphillips, 2014-05-15 12:24:21]

Looks very promising to me modulo the serialization hiccup in
SkGradientShaderBase.

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

https://codereview.chromium.org/283273002/diff/1/include/core/SkFlattenable.h...
include/core/SkFlattenable.h:63: kSkShader_Type,
kSkUnused_Type,   // used to be kSkUnitMapper_Type   ?

[scroggo, 2014-05-15 14:33:38]

https://codereview.chromium.org/283273002/diff/1/gm/tilemodes_scaled.cpp
File gm/tilemodes_scaled.cpp (left):

https://codereview.chromium.org/283273002/diff/1/gm/tilemodes_scaled.cpp#oldc...
gm/tilemodes_scaled.cpp:35: um = new SkCosineMapper;
Does this gm change? Please add any that change to ignored-tests.txt

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

https://codereview.chromium.org/283273002/diff/1/include/core/SkShader.h#newc...
include/core/SkShader.h:24: #ifdef SK_SUPPORT_LEGACY_DEADUNITMAPPERTYPE
Is this just in case Mozilla or someone uses it? It appears neither Chrome nor
Android use it.

https://codereview.chromium.org/283273002/diff/1/include/effects/SkGradientSh...
File include/effects/SkGradientShader.h (right):

https://codereview.chromium.org/283273002/diff/1/include/effects/SkGradientSh...
include/effects/SkGradientShader.h:44: @param  mapper  May be NULL. Callback to
modify the spread of the colors.
This comment can be removed.

https://codereview.chromium.org/283273002/diff/1/include/effects/SkGradientSh...
include/effects/SkGradientShader.h:83: @param  mapper  May be NULL. Callback to
modify the spread of the colors.
No longer needed.

https://codereview.chromium.org/283273002/diff/1/include/effects/SkGradientSh...
include/effects/SkGradientShader.h:124: @param  mapper  May be NULL. Callback to
modify the spread of the colors.
No longer needed.

https://codereview.chromium.org/283273002/diff/1/include/effects/SkGradientSh...
include/effects/SkGradientShader.h:198: @param  mapper  May be NULL. Callback to
modify the spread of the colors.
No longer needed.

https://codereview.chromium.org/283273002/diff/1/src/effects/gradients/SkGrad...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/283273002/diff/1/src/effects/gradients/SkGrad...
src/effects/gradients/SkGradientShader.cpp:143: #if 0
Can this code be removed entirely?

https://codereview.chromium.org/283273002/diff/1/src/utils/SkUnitMappers.cpp
File src/utils/SkUnitMappers.cpp (left):

https://codereview.chromium.org/283273002/diff/1/src/utils/SkUnitMappers.cpp#...
src/utils/SkUnitMappers.cpp:13: SkDiscreteMapper::SkDiscreteMapper(int segments)
{
You'll need to remove this file from skia_gn_files:
https://code.google.com/p/chromium/codesearch#chromium/src/skia/skia_gn_files...

[reed1, 2014-05-15 15:37:20]

https://codereview.chromium.org/283273002/diff/1/gm/tilemodes_scaled.cpp
File gm/tilemodes_scaled.cpp (left):

https://codereview.chromium.org/283273002/diff/1/gm/tilemodes_scaled.cpp#oldc...
gm/tilemodes_scaled.cpp:35: um = new SkCosineMapper;
On 2014/05/15 14:33:38, scroggo wrote:
> Does this gm change? Please add any that change to ignored-tests.txt

Yes (unfortunately), all the edited GMs will have to be rebaselined :(

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

https://codereview.chromium.org/283273002/diff/1/include/core/SkFlattenable.h...
include/core/SkFlattenable.h:63: kSkShader_Type,
On 2014/05/15 12:24:21, robertphillips wrote:
> kSkUnused_Type,   // used to be kSkUnitMapper_Type   ?

Sure... Do we need to keep these values in order? Can I just delete this enum?

https://codereview.chromium.org/283273002/diff/1/include/effects/SkGradientSh...
File include/effects/SkGradientShader.h (right):

https://codereview.chromium.org/283273002/diff/1/include/effects/SkGradientSh...
include/effects/SkGradientShader.h:44: @param  mapper  May be NULL. Callback to
modify the spread of the colors.
On 2014/05/15 14:33:38, scroggo wrote:
> This comment can be removed.

Done.

https://codereview.chromium.org/283273002/diff/1/include/effects/SkGradientSh...
include/effects/SkGradientShader.h:83: @param  mapper  May be NULL. Callback to
modify the spread of the colors.
On 2014/05/15 14:33:38, scroggo wrote:
> No longer needed.

Done.

https://codereview.chromium.org/283273002/diff/1/include/effects/SkGradientSh...
include/effects/SkGradientShader.h:124: @param  mapper  May be NULL. Callback to
modify the spread of the colors.
On 2014/05/15 14:33:38, scroggo wrote:
> No longer needed.

Done.

https://codereview.chromium.org/283273002/diff/1/include/effects/SkGradientSh...
include/effects/SkGradientShader.h:198: @param  mapper  May be NULL. Callback to
modify the spread of the colors.
On 2014/05/15 14:33:38, scroggo wrote:
> No longer needed.

Done.

https://codereview.chromium.org/283273002/diff/1/src/effects/gradients/SkGrad...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/283273002/diff/1/src/effects/gradients/SkGrad...
src/effects/gradients/SkGradientShader.cpp:143: #if 0
On 2014/05/15 14:33:38, scroggo wrote:
> Can this code be removed entirely?

Replaced with code to skip the unitmapper

https://codereview.chromium.org/283273002/diff/1/src/utils/SkUnitMappers.cpp
File src/utils/SkUnitMappers.cpp (left):

https://codereview.chromium.org/283273002/diff/1/src/utils/SkUnitMappers.cpp#...
src/utils/SkUnitMappers.cpp:13: SkDiscreteMapper::SkDiscreteMapper(int segments)
{
On 2014/05/15 14:33:38, scroggo wrote:
> You'll need to remove this file from skia_gn_files:
>
https://code.google.com/p/chromium/codesearch#chromium/src/skia/skia_gn_files...

curse you gn!

[reed1, 2014-05-15 16:57:26]

ptal

[scroggo, 2014-05-15 17:18:13]

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

https://codereview.chromium.org/283273002/diff/1/include/core/SkFlattenable.h...
include/core/SkFlattenable.h:63: kSkShader_Type,
On 2014/05/15 15:37:21, reed1 wrote:
> On 2014/05/15 12:24:21, robertphillips wrote:
> > kSkUnused_Type,   // used to be kSkUnitMapper_Type   ?
> 
> Sure... Do we need to keep these values in order? Can I just delete this enum?

I think it's fine to delete (and rearrange if necessary).

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

https://codereview.chromium.org/283273002/diff/1/include/core/SkShader.h#newc...
include/core/SkShader.h:24: #ifdef SK_SUPPORT_LEGACY_DEADUNITMAPPERTYPE
On 2014/05/15 14:33:38, scroggo wrote:
> Is this just in case Mozilla or someone uses it? It appears neither Chrome nor
> Android use it.

Thoughts? Is it just here because Chrome calls the functions which require the
parameter for a unitmapper? If so, we'll need to define it for chrome.

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

https://codereview.chromium.org/283273002/diff/40001/include/core/SkPicture.h...
include/core/SkPicture.h:330: // V27: Move writers into CreateProc, and obsolete
constructor(SkReadBuffer&).
Is this comment regarding another CL you have in flight? This version change
seems to be just to call skipFlattenable.

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

https://codereview.chromium.org/283273002/diff/40001/src/core/SkReadBuffer.cp...
src/core/SkReadBuffer.cpp:340: if (fFactoryCount > 0) {
This structure does appear to be the same as readFlattenable(), but it does
worry me they could get out of sync. Maybe we just need some comments?

https://codereview.chromium.org/283273002/diff/40001/src/core/SkValidatingRea...
File src/core/SkValidatingReadBuffer.cpp (right):

https://codereview.chromium.org/283273002/diff/40001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:283: uint32_t sizeRecorded =
this->readUInt();
There are a couple of cases where readFlattenable might have returned before
reaching its own call to readUInt(). Is it okay that this function calls it
without doing those checks?

[reed1, 2014-05-16 12:26:56]

https://code.google.com/p/skia/issues/detail?id=2560

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

https://codereview.chromium.org/283273002/diff/40001/include/core/SkPicture.h...
include/core/SkPicture.h:330: // V27: Move writers into CreateProc, and obsolete
constructor(SkReadBuffer&).
On 2014/05/15 17:18:13, scroggo wrote:
> Is this comment regarding another <span id="abbrex-spnAbbr_687"
class="abbrex-Abbr abbrex-Abbr_CL abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">CL</span> you have in flight? This version change
> seems to be just to call skipFlattenable.

Done.

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

https://codereview.chromium.org/283273002/diff/40001/src/core/SkReadBuffer.cp...
src/core/SkReadBuffer.cpp:340: if (fFactoryCount > 0) {
On 2014/05/15 17:18:13, scroggo wrote:
> This structure does appear to be the same as readFlattenable(), but it does
> worry me they could get out of sync. Maybe we just need some comments?

Done.

https://codereview.chromium.org/283273002/diff/40001/src/core/SkValidatingRea...
File src/core/SkValidatingReadBuffer.cpp (right):

https://codereview.chromium.org/283273002/diff/40001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:283: uint32_t sizeRecorded =
this->readUInt();
On 2014/05/15 17:18:13, scroggo wrote:
> There are a couple of cases where readFlattenable might have returned before
> reaching its own call to readUInt(). Is it okay that this function calls it
> without doing those checks?

1. Some of those tests involve knowing the type (enum), which I don't here
2. I think perhaps that logic is faulty, as it won't actually leave the buffer
past the data when it returns early.

I will file a bug/investigation for #2 separate from this CL.

[scroggo, 2014-05-16 13:21:02]

lgtm

[reed1, 2014-05-16 13:29:24]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-05-16 13:29:40]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/283273002/120001

[commit-bot: I haz the power, 2014-05-16 13:35:32]

Change committed as 14761

[reed1, 2014-05-16 13:48:37]

A revert of this CL has been created in
https://codereview.chromium.org/287063009/ by reed@google.com.

The reason for reverting is: does not address all legacy callsites in chrome.

e.g.

[13:45:32.091872] ../../ui/native_theme/native_theme_base.cc:608:76: error: no
matching function for call to ‘SkGradientShader::CreateLinear(SkPoint [3],
SkColor [3], NULL, int, SkShader::TileMode, NULL)’
[13:45:32.091919]            gradient_bounds, colors, NULL, 3,
SkShader::kClamp_TileMode, NULL));
.