Consolidate SkCodec functions for handling rewind

Consolidate SkCodec functions for handling rewind

Previously, many of our codec implementations followed the same
pattern (often in a function named handleRewind):

switch (this->rewindIfNeeded()) {
  case CouldNotRewind:
    return CouldNotRewind;
  case NoRewindNecessary:
    // keep going
    break;
  case Rewound:
     <re-read header etc>
    break;
}

In this CL, remove the enum, and put the piece that happens in the
Rewound case into a virtual function, onRewind. rewindIfNeeded now
contains the common pieces from various functions named handleRewind.

In SkBmpCodec, add a function that returns whether the BMP is in ICO,
so it can have a common implementation for onRewind.

BUG=skia:3257
Committed: https://skia.googlesource.com/skia/+/b427db1d457a083f2652756a453fbb91bc6a7447

[scroggo_chromium, 2015-08-11 16:29:41]

scroggo@chromium.org changed reviewers:
+ djsollen@google.com, emmaleer@google.com, msarett@google.com

[scroggo_chromium, 2015-08-11 16:29:42]

Derek, I need your approval for the API change. Feel free to look at the rest of
the code as well.

https://codereview.chromium.org/1288483002/diff/1/src/codec/SkBmpCodec.h
File src/codec/SkBmpCodec.h (right):

https://codereview.chromium.org/1288483002/diff/1/src/codec/SkBmpCodec.h#newc...
src/codec/SkBmpCodec.h:71: bool inIco() const {
This split follows the convention we follow for public functions:

public:

function() {
  return this->onVirtualFunction();
}

protected:

virtual onVirtualFunction();

Maybe this isn't necessary here, since inIco() does not need to be made public?

[djsollen, 2015-08-11 19:27:50]

api lgtm

[msarett, 2015-08-12 13:38:46]

lgtm

Thanks for doing this!  I know we've been talking about needing to clean this up
for a little while now.

https://codereview.chromium.org/1288483002/diff/1/src/codec/SkBmpCodec.h
File src/codec/SkBmpCodec.h (right):

https://codereview.chromium.org/1288483002/diff/1/src/codec/SkBmpCodec.h#newc...
src/codec/SkBmpCodec.h:71: bool inIco() const {
On 2015/08/11 16:29:42, scroggo_chromium wrote:
> This split follows the convention we follow for public functions:
> 
> public:
> 
> function() {
>   return this->onVirtualFunction();
> }
> 
> protected:
> 
> virtual onVirtualFunction();
> 
> Maybe this isn't necessary here, since inIco() does not need to be made
public?

I don't feel strongly on this.  Looks fine to me.

[scroggo, 2015-08-12 14:16:37]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2015-08-12 14:16:48]

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

[commit-bot: I haz the power, 2015-08-12 14:24:16]

Committed patchset #1 (id:1) as
https://skia.googlesource.com/skia/+/b427db1d457a083f2652756a453fbb91bc6a7447