Merge two determineFilterPrimitiveSubregion implementations

Merge two determineFilterPrimitiveSubregion implementations

These methods seem similar enough to merge.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162713

[rwlbuis, 2013-11-26 21:54:58]

This patch is optional of course but I think a bit cleaner and gives no chance
for the two methods to diverge.
It would be even better if there was no real difference in
determineFilterPrimitiveSubregion for CSS vs SVG filters, but I don't know how
realistic that is atm and it is not a short term thing for sure...

[Stephen White, 2013-11-26 22:11:13]

This is a refactoring I always meant to do.. thanks for doing it! Just a few
comments.

https://codereview.chromium.org/89283002/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/filters/FilterEffect.h (right):

https://codereview.chromium.org/89283002/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/filters/FilterEffect.h:158: FloatRect
determineFilterPrimitiveSubregion(bool mapRectForward = true, bool
clipToFilterRegion = false);
Per http://www.chromium.org/blink/coding-style#TOC-Right:25

rather than two bools, I think it would be clearer if this was a single "Flags"
enum, so the call sites are self-documenting, e.g.,

effect->determineFilterPrimitiveSubregion(MapRectForward | ClipToFilterRegion)

https://codereview.chromium.org/89283002/diff/1/Source/core/rendering/svg/Ren...
File Source/core/rendering/svg/RenderSVGResourceFilterPrimitive.h (right):

https://codereview.chromium.org/89283002/diff/1/Source/core/rendering/svg/Ren...
Source/core/rendering/svg/RenderSVGResourceFilterPrimitive.h:34: class
FilterEffect;
I think this could be removed now.

[rwlbuis, 2013-11-26 22:28:35]

I tried to implement the suggestions, which I agree help readability.

https://codereview.chromium.org/89283002/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/filters/FilterEffect.h (right):

https://codereview.chromium.org/89283002/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/filters/FilterEffect.h:158: FloatRect
determineFilterPrimitiveSubregion(bool mapRectForward = true, bool
clipToFilterRegion = false);
On 2013/11/26 22:11:14, Stephen White wrote:
> Per http://www.chromium.org/blink/coding-style#TOC-Right:25
> 
> rather than two bools, I think it would be clearer if this was a single
"Flags"
> enum, so the call sites are self-documenting, e.g.,
> 
> effect->determineFilterPrimitiveSubregion(MapRectForward | ClipToFilterRegion)

Sounds great, fixing.

https://codereview.chromium.org/89283002/diff/1/Source/core/rendering/svg/Ren...
File Source/core/rendering/svg/RenderSVGResourceFilterPrimitive.h (right):

https://codereview.chromium.org/89283002/diff/1/Source/core/rendering/svg/Ren...
Source/core/rendering/svg/RenderSVGResourceFilterPrimitive.h:34: class
FilterEffect;
On 2013/11/26 22:11:14, Stephen White wrote:
> I think this could be removed now.

Correct, fixed.

[Stephen White, 2013-11-26 22:31:37]

One final nit..

https://codereview.chromium.org/89283002/diff/20001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/FilterEffect.h (right):

https://codereview.chromium.org/89283002/diff/20001/Source/core/platform/grap...
Source/core/platform/graphics/filters/FilterEffect.h:166: FloatRect
determineFilterPrimitiveSubregion(DetermineSubregionFlags = MapRectForward);
I'd actually prefer that the default argument is None, and that you have to
explicitly request MapRectForward.

[rwlbuis, 2013-11-26 22:41:35]

This hopefully addresses the last suggestion.

https://codereview.chromium.org/89283002/diff/20001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/FilterEffect.h (right):

https://codereview.chromium.org/89283002/diff/20001/Source/core/platform/grap...
Source/core/platform/graphics/filters/FilterEffect.h:166: FloatRect
determineFilterPrimitiveSubregion(DetermineSubregionFlags = MapRectForward);
On 2013/11/26 22:31:37, Stephen White wrote:
> I'd actually prefer that the default argument is None, and that you have to
> explicitly request MapRectForward.

Right, for some reason I thought there might be quite some call sites to change
but it is just one, fixed.

[Stephen White, 2013-11-26 22:43:32]

LGTM. Thanks again!

[commit-bot: I haz the power, 2013-11-26 22:45:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/89283002/40001

[commit-bot: I haz the power, 2013-11-27 00:20:29]

Change committed as 162713