cc: Remove unnecessary "default" cases from switch statements.

cc: Remove unnecessary "default" cases from switch statements.

Makes all cc/ code consistent and improves compile time error
checking when adding new enums.

TEST=compile,cc_unittests
BUG=277861
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219691

[reveman, 2013-08-22 23:14:45]

https://codereview.chromium.org/22875045/diff/1/cc/output/filter_operations.cc
File cc/output/filter_operations.cc (right):

https://codereview.chromium.org/22875045/diff/1/cc/output/filter_operations.c...
cc/output/filter_operations.cc:121: return true;
I changed the logic here as it looks like it was wrong before. This function
would previously return false as soon as it came across a COLOR_MATRIX operation
that doesn't affect alpha. It's now only returning false if no operations affect
alpha.

[vmpstr, 2013-08-22 23:33:34]

The comment below applies to other enums as well

https://codereview.chromium.org/22875045/diff/1/cc/resources/managed_tile_sta...
File cc/resources/managed_tile_state.cc (right):

https://codereview.chromium.org/22875045/diff/1/cc/resources/managed_tile_sta...
cc/resources/managed_tile_state.cc:42: scoped_ptr<base::Value>
ManagedTileBinPriorityAsValue(
I don't think this function is used at all

https://codereview.chromium.org/22875045/diff/1/cc/resources/managed_tile_sta...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/22875045/diff/1/cc/resources/managed_tile_sta...
cc/resources/managed_tile_state.h:27: MAX_BIN = NEVER_BIN
I'd prefer we keep NUM_BINS. Maybe something like the following would be better?

enum ManagedTileBin {
   ...
};

enum {
  NUM_BINS = NEVER_BIN + 1
};

If we have MAX_BIN then passing it somewhere (incorrectly) will just get handled
as NEVER_BIN.

[danakj, 2013-08-23 00:38:13]

https://codereview.chromium.org/22875045/diff/1/cc/resources/managed_tile_sta...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/22875045/diff/1/cc/resources/managed_tile_sta...
cc/resources/managed_tile_state.h:27: MAX_BIN = NEVER_BIN
On 2013/08/22 23:33:34, vmpstr wrote:
> I'd prefer we keep NUM_BINS. Maybe something like the following would be
better?
> 
> enum ManagedTileBin {
>    ...
> };
> 
> enum {
>   NUM_BINS = NEVER_BIN + 1
> };
> 
> If we have MAX_BIN then passing it somewhere (incorrectly) will just get
handled
> as NEVER_BIN. 

This seems like a pretty nice idea to me. Maybe without the blank line in
between so it's super obviously connected even.

[enne (OOO), 2013-08-23 00:44:38]

https://codereview.chromium.org/22875045/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/22875045/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:29: kBinPolicyMap[MAX_TILE_MEMORY_LIMIT_POLICY +
1][MAX_BIN + 1] = {
Ugh.

I really don't like how awkward this makes enums that want to have a
NUM_THINGERS in them.  Is this really necessary?

[enne (OOO), 2013-08-23 00:49:58]

Sorry to be grumpy, but the separate enum also seems a little messy.  Somebody
could add another enum and forget to update the other enum struct.

Personally, I think...

    enum Things {
      THING,
      THING2,
      NUM_THINGS
    };

...is a common, self-documenting, hard to screw up C++ pattern.   I'd prefer to
replace default with case NUM_THINGS: NOT_REACHED() to fix this bug.

[reveman, 2013-08-23 15:16:57]

Thanks for the feedback.

I like keeping switch statements as clean as possible as we usually have
important logic there. "case NUM_THINGS: NOT_REACHED()" is not great in that
sense.

I don't like the extra enum solution as you can forget to adjust it when adding
new enum values. This is also not a commonly used pattern. I think both the num
values or max value enum concepts are widely used.

Could we keep the current MAX concept but make the way we get the array size a
bit cleaner? Maybe require the max value to be named kEnumNameMax and add a
ENUM_ARRAY_SIZE macro.

TileVersion tile_versions[MAX_RASTER_MODE + 1]
would become something like:
TileVersion tile_versions[ENUM_ARRAY_SIZE(RasterMode)]

[enne (OOO), 2013-08-23 16:14:50]

On 2013/08/23 15:16:57, David Reveman wrote:

> I like keeping switch statements as clean as possible as we usually have
> important logic there. "case NUM_THINGS: NOT_REACHED()" is not great in that
> sense.

Isn't the bug that these switch statements already have a "default:
NOTREACHED()" case?  I've never seen anybody balk at those for code cleanliness
reasons.

It seems like a relative net win to just change those existing default cases to
be explicit cases for NUM_THINGS.  No new cases have to be added and now there
are compiler warnings.
 
> Could we keep the current MAX concept but make the way we get the array size a
> bit cleaner? Maybe require the max value to be named kEnumNameMax and add a
> ENUM_ARRAY_SIZE macro.

That solution sounds like vmpstr's solution in comment #2.  You'd have to put
kEnumNameMax in a separate enumeration to not have the switch statements pick it
up.

Comparing kEnumMax to explicit NUM cases:
* Both add useless code.  kEnumMax makes the enum declaration less clean. 
explicit NUM cases make the switch statement less clean.  This is probably a
wash.
* kEnumMax requires an extra change to be up to date when you add a new final
value and you could screw it up.  It's much harder to mess up NUM_THINGS.
* kEnumMax is a lot more complicated.  I don't want to have to explain that to
folks submitting patches to cc.  "Ok, make this second enum with a magic name
and use this special macro to find the size."
* You could maybe presubmit-ban "default:" in cc, but it'd be hard to presubmit
enforce kEnumMax.
* Declaring NUM_THINGS as the last thing in an enum is something that other
programmers expect and understand.  kEnumMax will make cc even stranger than the
rest of the codebase.

[vmpstr, 2013-08-23 16:39:52]

On 2013/08/23 16:14:50, enne wrote:
> On 2013/08/23 15:16:57, David Reveman wrote:
> 
> > I like keeping switch statements as clean as possible as we usually have
> > important logic there. "case NUM_THINGS: NOT_REACHED()" is not great in that
> > sense.
> 
> Isn't the bug that these switch statements already have a "default:
> NOTREACHED()" case?  I've never seen anybody balk at those for code
cleanliness
> reasons.
> 
> It seems like a relative net win to just change those existing default cases
to
> be explicit cases for NUM_THINGS.  No new cases have to be added and now there
> are compiler warnings.
>  
> > Could we keep the current MAX concept but make the way we get the array size
a
> > bit cleaner? Maybe require the max value to be named kEnumNameMax and add a
> > ENUM_ARRAY_SIZE macro.
> 
> That solution sounds like vmpstr's solution in comment #2.  You'd have to put
> kEnumNameMax in a separate enumeration to not have the switch statements pick
it
> up.
> 
> Comparing kEnumMax to explicit NUM cases:
> * Both add useless code.  kEnumMax makes the enum declaration less clean. 
> explicit NUM cases make the switch statement less clean.  This is probably a
> wash.
> * kEnumMax requires an extra change to be up to date when you add a new final
> value and you could screw it up.  It's much harder to mess up NUM_THINGS.
> * kEnumMax is a lot more complicated.  I don't want to have to explain that to
> folks submitting patches to cc.  "Ok, make this second enum with a magic name
> and use this special macro to find the size."
> * You could maybe presubmit-ban "default:" in cc, but it'd be hard to
presubmit
> enforce kEnumMax.
> * Declaring NUM_THINGS as the last thing in an enum is something that other
> programmers expect and understand.  kEnumMax will make cc even stranger than
the
> rest of the codebase.

I tend to agree. I prefer to keep the complexity of maintaining an enum near the
enum itself. That is, all of the declarations that use either NUM or MAX should
be as simple as possible and adding new arrays or whatnot that use the count
should be intuitive. Updating an enum is the thing that should possibly require
an extra step.

Keeping the current NUM/COUNT seems like the easiest to maintain with that
criteria. The approach that I proposed is more difficult to maintain (ie, it's
easier to forget to update it), but it adds the nicety that NUM/COUNT is not a
valid value for the enum, so it doesn't have to be handled in switch statements
as well as it can't be passed in where a valid enum value is expected. 

With that, the current approach of NUM as the last enum seems like a good
compromise: it's the easiest to maintain, but it needs to be handled in switch
statements. So I would vote for that.

[reveman, 2013-08-23 16:46:17]

On 2013/08/23 16:14:50, enne wrote:
> On 2013/08/23 15:16:57, David Reveman wrote:
> 
> > I like keeping switch statements as clean as possible as we usually have
> > important logic there. "case NUM_THINGS: NOT_REACHED()" is not great in that
> > sense.
> 
> Isn't the bug that these switch statements already have a "default:
> NOTREACHED()" case?  I've never seen anybody balk at those for code
cleanliness
> reasons.
> 
> It seems like a relative net win to just change those existing default cases
to
> be explicit cases for NUM_THINGS.  No new cases have to be added and now there
> are compiler warnings.
>  
> > Could we keep the current MAX concept but make the way we get the array size
a
> > bit cleaner? Maybe require the max value to be named kEnumNameMax and add a
> > ENUM_ARRAY_SIZE macro.
> 
> That solution sounds like vmpstr's solution in comment #2.  You'd have to put
> kEnumNameMax in a separate enumeration to not have the switch statements pick
it
> up.
> 
> Comparing kEnumMax to explicit NUM cases:
> * Both add useless code.  kEnumMax makes the enum declaration less clean. 
> explicit NUM cases make the switch statement less clean.  This is probably a
> wash.
> * kEnumMax requires an extra change to be up to date when you add a new final
> value and you could screw it up.  It's much harder to mess up NUM_THINGS.
> * kEnumMax is a lot more complicated.  I don't want to have to explain that to
> folks submitting patches to cc.  "Ok, make this second enum with a magic name
> and use this special macro to find the size."
> * You could maybe presubmit-ban "default:" in cc, but it'd be hard to
presubmit
> enforce kEnumMax.
> * Declaring NUM_THINGS as the last thing in an enum is something that other
> programmers expect and understand.  kEnumMax will make cc even stranger than
the
> rest of the codebase.

The kEnumMax would still be in the same enum as the current patch. Only
difference would be that we standardize the name of the max value relative to
the enum name.

There's also the option of adding a DEFINE_ENUM macro that can be used to
cleanly declare an enum that will be used for array indexing.

Btw, we can't remove "default:" completely as switch statements are often used
with non-enum types.

[enne (OOO), 2013-08-23 16:50:41]

On 2013/08/23 16:46:17, David Reveman wrote:

> The kEnumMax would still be in the same enum as the current patch. Only
> difference would be that we standardize the name of the max value relative to
> the enum name.

Are you suggesting this?

enum Things {
  THING1,
  THING2,
  MaxThings = THING2
};

> Btw, we can't remove "default:" completely as switch statements are often used
> with non-enum types.

Ok, fair enough.  This makes me feel even more strongly to not do anything
complicated if it can't be automatically enforced, because that complication
falls on the reviewer.

[danakj, 2013-08-23 17:42:15]

I'm going to vote for NUM_THINGS: NOTREACHED();

Keeping this as simple as possible is good. Vlad's idea was pretty cool but
has some drawbacks as discussed above. I don't see the drawback of
NUM_THINGS: NOTREACHED() other than the line of code exists. But we still
get compiler warnings etc and things work simply and as expected.


On Fri, Aug 23, 2013 at 12:50 PM, <enne@chromium.org> wrote:

> On 2013/08/23 16:46:17, David Reveman wrote:
>
>  The kEnumMax would still be in the same enum as the current patch. Only
>> difference would be that we standardize the name of the max value
>> relative to
>> the enum name.
>>
>
> Are you suggesting this?
>
> enum Things {
>   THING1,
>   THING2,
>   MaxThings = THING2
>
> };
>
>  Btw, we can't remove "default:" completely as switch statements are often
>> used
>> with non-enum types.
>>
>
> Ok, fair enough.  This makes me feel even more strongly to not do anything
> complicated if it can't be automatically enforced, because that
> complication
> falls on the reviewer.
>
>
https://codereview.chromium.**org/22875045/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reveman, 2013-08-23 19:57:56]

On 2013/08/23 16:50:41, enne wrote:
> On 2013/08/23 16:46:17, David Reveman wrote:
> 
> > The kEnumMax would still be in the same enum as the current patch. Only
> > difference would be that we standardize the name of the max value relative
to
> > the enum name.
> 
> Are you suggesting this?
> 
> enum Things {
>   THING1,
>   THING2,
>   MaxThings = THING2
> };

Yes, and the other suggestion I made was to add a DEFINE_ENUM macro that can be
used only in cases where we want to create an array from the number of enum
values. It would look something like this:

#define DEFINE_ENUM(name, ...)                             \
    enum name { __VA_ARGS__ };                             \
    const name name ## _MIN = static_cast<name>(0);        \
    const name name ## _MAX = static_cast<name>(           \
        sizeof((int[]){ __VA_ARGS__ }) / sizeof(int) - 1); \
    const int name ## _ARRAYSIZE = name ## _MAX + 1

example usage:

DEFINE_ENUM(RasterMode,
            HIGH_QUALITY_RASTER_MODE,
            LOW_QUALITY_RASTER_MODE);

TileVersion tile_versions[RasterMode_ARRAYSIZE];

this way the switch statement stays clean, array declarations are as clean as
before, there's only one place to edit when adding a new enum value and we
prevent the author from using bad enum values such as "enum RasterMode {
HIGH_QUALITY_RASTER_MODE = 10, LOW_QUALITY_RASTER_MODE = 1, NUM_RASTER_MODE }"

Wdyt?

[enne (OOO), 2013-08-23 20:31:33]

On 2013/08/23 19:57:56, David Reveman wrote:

> > Are you suggesting this?
> > 
> > enum Things {
> >   THING1,
> >   THING2,
> >   MaxThings = THING2
> > };
> 
> Yes, and the other suggestion I made was to add a DEFINE_ENUM macro that can
be
> used only in cases where we want to create an array from the number of enum
> values. It would look something like this:

I think this approach is also error-prone.  As I mentioned before, you have to
keep an extra line up to date.  Worse, this code works:

Things thing;
switch (thing) {
  case THING1:
    break;
  case MaxThings:
    // OOPS, MaxThings is a real value and aliases with THING2.
    // Also, the compiler won't complain because all values are handled.
    break;
};
 
> #define DEFINE_ENUM(name, ...)
> ...
> Wdyt?

I am not a C++ expert enough to be convinced that you can cast a variadic macro
like that.

That said, this is certainly more elegant than other solutions, although you
have to know what the magic incantation is to get the right name for the array
size which is a little unfortunate.

I think mostly I'm a little skeptical of changing the way a language feature
like enums are declared in one subset of the codebase.  If all of Chromium
decided this was better, then great.  But, if it's only in cc, then I think it
would add extra friction for new contributors in an unfortunate way.

[reveman, 2013-08-23 20:45:00]

On 2013/08/23 20:31:33, enne wrote:
> On 2013/08/23 19:57:56, David Reveman wrote:
> 
> > > Are you suggesting this?
> > > 
> > > enum Things {
> > >   THING1,
> > >   THING2,
> > >   MaxThings = THING2
> > > };
> > 
> > Yes, and the other suggestion I made was to add a DEFINE_ENUM macro that can
> be
> > used only in cases where we want to create an array from the number of enum
> > values. It would look something like this:
> 
> I think this approach is also error-prone.  As I mentioned before, you have to
> keep an extra line up to date.  Worse, this code works:
> 
> Things thing;
> switch (thing) {
>   case THING1:
>     break;
>   case MaxThings:
>     // OOPS, MaxThings is a real value and aliases with THING2.
>     // Also, the compiler won't complain because all values are handled.
>     break;
> };
>  
> > #define DEFINE_ENUM(name, ...)
> > ...
> > Wdyt?
> 
> I am not a C++ expert enough to be convinced that you can cast a variadic
macro
> like that.

It's an int array cast. I think this is guaranteed to work with all C
preprocessors that support variadic macro expansion but I would have to verify
that.

> 
> That said, this is certainly more elegant than other solutions, although you
> have to know what the magic incantation is to get the right name for the array
> size which is a little unfortunate.
> 
> I think mostly I'm a little skeptical of changing the way a language feature
> like enums are declared in one subset of the codebase.  If all of Chromium
> decided this was better, then great.  But, if it's only in cc, then I think it
> would add extra friction for new contributors in an unfortunate way.

I think the extra friction here is minimal. Maybe we can start with cc/ and if
it works well we push it to base/

Anyhow, I'm going to change this patch to only remove the "default:" cases that
doesn't require the addition of "case NUM_THINGS: NOTREACHED;" for now.

[reveman, 2013-08-26 13:56:14]

Reduced this to the cases which I think we all agree on. PTAL.

I uploaded a patch with my DEFINE_ENUM idea in case you'd like to see what that
could look like: https://codereview.chromium.org/22911037/

[vmpstr, 2013-08-26 15:38:29]

lgtm

https://codereview.chromium.org/22875045/diff/6001/cc/resources/raster_mode.cc
File cc/resources/raster_mode.cc (right):

https://codereview.chromium.org/22875045/diff/6001/cc/resources/raster_mode.c...
cc/resources/raster_mode.cc:24: default:
I think this can also be removed with NOTREACHED at the end of the function.

[danakj, 2013-08-26 15:55:39]

Looks good but in cases where all switch cases should return  a value, I'd
prefer to have each case return their value instead of just breaking, which
allows us to NOTREACHED below the switch statement.

https://codereview.chromium.org/22875045/diff/6001/cc/animation/transform_ope...
File cc/animation/transform_operation.cc (right):

https://codereview.chromium.org/22875045/diff/6001/cc/animation/transform_ope...
cc/animation/transform_operation.cc:300: break;
can you return false here and NOTREACHED() below?

https://codereview.chromium.org/22875045/diff/6001/cc/output/filter_operation.cc
File cc/output/filter_operation.cc (right):

https://codereview.chromium.org/22875045/diff/6001/cc/output/filter_operation...
cc/output/filter_operation.cc:175: break;
can you NOTREACHED() inside this case as well, just for clarity?

https://codereview.chromium.org/22875045/diff/6001/cc/output/filter_operation...
File cc/output/filter_operations.cc (right):

https://codereview.chromium.org/22875045/diff/6001/cc/output/filter_operation...
cc/output/filter_operations.cc:99: break;
can you return false here and NOTREACHED() below?

https://codereview.chromium.org/22875045/diff/6001/cc/output/filter_operation...
cc/output/filter_operations.cc:132: break;
same request here

https://codereview.chromium.org/22875045/diff/6001/cc/output/render_surface_f...
File cc/output/render_surface_filters.cc (right):

https://codereview.chromium.org/22875045/diff/6001/cc/output/render_surface_f...
cc/output/render_surface_filters.cc:232: break;
and return false here with NOTREACHED below? also no need for {}

https://codereview.chromium.org/22875045/diff/6001/cc/resources/platform_color.h
File cc/resources/platform_color.h (right):

https://codereview.chromium.org/22875045/diff/6001/cc/resources/platform_colo...
cc/resources/platform_color.h:33: // fall-through
how about just return GL_RGBA here?

[vmpstr, 2013-08-26 16:04:26]

https://codereview.chromium.org/22875045/diff/6001/cc/resources/platform_color.h
File cc/resources/platform_color.h (right):

https://codereview.chromium.org/22875045/diff/6001/cc/resources/platform_colo...
cc/resources/platform_color.h:33: // fall-through
On 2013/08/26 15:55:39, danakj wrote:
> how about just return GL_RGBA here?

+1 :)

[reveman, 2013-08-26 17:16:57]

https://codereview.chromium.org/22875045/diff/6001/cc/animation/transform_ope...
File cc/animation/transform_operation.cc (right):

https://codereview.chromium.org/22875045/diff/6001/cc/animation/transform_ope...
cc/animation/transform_operation.cc:300: break;
On 2013/08/26 15:55:39, danakj wrote:
> can you return false here and NOTREACHED() below?

Done.

https://codereview.chromium.org/22875045/diff/6001/cc/output/filter_operation.cc
File cc/output/filter_operation.cc (right):

https://codereview.chromium.org/22875045/diff/6001/cc/output/filter_operation...
cc/output/filter_operation.cc:175: break;
On 2013/08/26 15:55:39, danakj wrote:
> can you NOTREACHED() inside this case as well, just for clarity?

Done.

https://codereview.chromium.org/22875045/diff/6001/cc/output/filter_operation...
File cc/output/filter_operations.cc (right):

https://codereview.chromium.org/22875045/diff/6001/cc/output/filter_operation...
cc/output/filter_operations.cc:99: break;
On 2013/08/26 15:55:39, danakj wrote:
> can you return false here and NOTREACHED() below?

Done.

https://codereview.chromium.org/22875045/diff/6001/cc/output/filter_operation...
cc/output/filter_operations.cc:132: break;
On 2013/08/26 15:55:39, danakj wrote:
> same request here

Done.

https://codereview.chromium.org/22875045/diff/6001/cc/output/render_surface_f...
File cc/output/render_surface_filters.cc (right):

https://codereview.chromium.org/22875045/diff/6001/cc/output/render_surface_f...
cc/output/render_surface_filters.cc:232: break;
On 2013/08/26 15:55:39, danakj wrote:
> and return false here with NOTREACHED below? also no need for {}

Done.

https://codereview.chromium.org/22875045/diff/6001/cc/resources/platform_color.h
File cc/resources/platform_color.h (right):

https://codereview.chromium.org/22875045/diff/6001/cc/resources/platform_colo...
cc/resources/platform_color.h:33: // fall-through
On 2013/08/26 15:55:39, danakj wrote:
> how about just return GL_RGBA here?

Done.

https://codereview.chromium.org/22875045/diff/6001/cc/resources/raster_mode.cc
File cc/resources/raster_mode.cc (right):

https://codereview.chromium.org/22875045/diff/6001/cc/resources/raster_mode.c...
cc/resources/raster_mode.cc:24: default:
On 2013/08/26 15:38:30, vmpstr wrote:
> I think this can also be removed with NOTREACHED at the end of the function.

We need it as NUM_RASTER_MODES still exists.

[vmpstr, 2013-08-26 17:19:17]

https://codereview.chromium.org/22875045/diff/6001/cc/resources/raster_mode.cc
File cc/resources/raster_mode.cc (right):

https://codereview.chromium.org/22875045/diff/6001/cc/resources/raster_mode.c...
cc/resources/raster_mode.cc:24: default:
On 2013/08/26 17:16:57, David Reveman wrote:
> On 2013/08/26 15:38:30, vmpstr wrote:
> > I think this can also be removed with NOTREACHED at the end of the function.
> 
> We need it as NUM_RASTER_MODES still exists.

Ah, my bad. I confused this with NUM_MODES that you removed.

[danakj, 2013-08-26 17:22:53]

LGTM, thanks.

https://codereview.chromium.org/22875045/diff/30001/cc/output/filter_operatio...
File cc/output/filter_operation.cc (right):

https://codereview.chromium.org/22875045/diff/30001/cc/output/filter_operatio...
cc/output/filter_operation.cc:176: break;
nit: return amount here?

[reveman, 2013-08-26 20:05:13]

https://codereview.chromium.org/22875045/diff/30001/cc/output/filter_operatio...
File cc/output/filter_operation.cc (right):

https://codereview.chromium.org/22875045/diff/30001/cc/output/filter_operatio...
cc/output/filter_operation.cc:176: break;
On 2013/08/26 17:22:54, danakj wrote:
> nit: return amount here?

Done.

[commit-bot: I haz the power, 2013-08-26 20:22:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/22875045/35001

[commit-bot: I haz the power, 2013-08-26 22:01:14]

Retried try job too often on linux_aura for step(s) aura_unittests,
browser_tests, compositor_unittests, content_browsertests, content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[reveman, 2013-08-26 23:38:40]

https://chromiumcodereview.appspot.com/22875045/diff/6001/cc/output/filter_op...
File cc/output/filter_operations.cc (right):

https://chromiumcodereview.appspot.com/22875045/diff/6001/cc/output/filter_op...
cc/output/filter_operations.cc:99: break;
On 2013/08/26 15:55:39, danakj wrote:
> can you return false here and NOTREACHED() below?

I was too quick to change this. We can't of course change this as this is a loop
that should only return false if no filter moves pixels. Changed it back in
latest patch.

[commit-bot: I haz the power, 2013-08-27 00:07:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/22875045/60001

[danakj, 2013-08-27 00:18:54]

https://chromiumcodereview.appspot.com/22875045/diff/6001/cc/output/filter_op...
File cc/output/filter_operations.cc (right):

https://chromiumcodereview.appspot.com/22875045/diff/6001/cc/output/filter_op...
cc/output/filter_operations.cc:99: break;
On 2013/08/26 23:38:41, David Reveman wrote:
> On 2013/08/26 15:55:39, danakj wrote:
> > can you return false here and NOTREACHED() below?
> 
> I was too quick to change this. We can't of course change this as this is a
loop
> that should only return false if no filter moves pixels. Changed it back in
> latest patch.

Oh. Loops, how do they work? Sorry about that, thanks for catching it.

[commit-bot: I haz the power, 2013-08-27 04:17:53]

Change committed as 219691