Move SVG path blend state to its own object

Move SVG path blend state to its own object

Create a new inner state object SVGPathBlender::BlendState and move
SVGPathBlender::blendSegments and helpers to it. The per-segment blend*
methods are folded away into blendSegments(). This allows to better
exploit commonalities between the different segment types.
The update of the current point(s) is moved into a helper function.

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

[fs, 2015-04-02 13:59:31]

fs@opera.com changed reviewers:
+ fmalita@chromium.org, pdr@chromium.org, schenney@chromium.org

[Stephen Chennney, 2015-04-02 15:00:30]

I agree this is a good refactoring of the existing code. But I have some
questions about the existing logic.

https://codereview.chromium.org/1056873002/diff/20001/Source/core/svg/SVGPath...
File Source/core/svg/SVGPathBlender.cpp (right):

https://codereview.chromium.org/1056873002/diff/20001/Source/core/svg/SVGPath...
Source/core/svg/SVGPathBlender.cpp:97: FloatPoint
SVGPathBlender::BlendState::blendAnimatedFloatPointSameCoordinates(const
FloatPoint& fromPoint, const FloatPoint& toPoint)
Minor nit: Could you swap the order of this and the next to match the header
declaration order. And put canBlend before both.

https://codereview.chromium.org/1056873002/diff/20001/Source/core/svg/SVGPath...
Source/core/svg/SVGPathBlender.cpp:161: case PathSegLineToVerticalRel:
I think I get it. segment.targetPoint is already limited to just x or y, so you
can just add it here, while below you don't want to 0-out the existing y or x
position.

https://codereview.chromium.org/1056873002/diff/20001/Source/core/svg/SVGPath...
Source/core/svg/SVGPathBlender.cpp:294: }
The code implies that you can have an empty "from" sequence and everything will
work using the command from the "to" sequence. And in this case the "from"
sequence has the default point1, point2, targetPoint etc. Is that how it is
supposed to work?

[fs, 2015-04-02 15:08:31]

https://codereview.chromium.org/1056873002/diff/20001/Source/core/svg/SVGPath...
File Source/core/svg/SVGPathBlender.cpp (right):

https://codereview.chromium.org/1056873002/diff/20001/Source/core/svg/SVGPath...
Source/core/svg/SVGPathBlender.cpp:161: case PathSegLineToVerticalRel:
On 2015/04/02 15:00:30, Stephen Chenney wrote:
> I think I get it. segment.targetPoint is already limited to just x or y, so
you
> can just add it here, while below you don't want to 0-out the existing y or x
> position.

Right, Horizontal/VerticalRel is essentially the same as LineRel but with an
implicit 0 as the other part. Horizontal/Vertical_Abs_ on the other hand is
(weird) so need to explicitly just set one of them.

https://codereview.chromium.org/1056873002/diff/20001/Source/core/svg/SVGPath...
Source/core/svg/SVGPathBlender.cpp:294: }
On 2015/04/02 15:00:30, Stephen Chenney wrote:
> The code implies that you can have an empty "from" sequence and everything
will
> work using the command from the "to" sequence. And in this case the "from"
> sequence has the default point1, point2, targetPoint etc. Is that how it is
> supposed to work?

I don't know where this behavior actually stems from (the additive behavior I
suppose), but my interpretation matches yours. And all the "default" values are
0 (or false).

[Stephen Chennney, 2015-04-02 15:10:43]

LGTM. We're clearly not going to break anything that already exists.

[fs, 2015-04-07 08:27:29]

https://codereview.chromium.org/1056873002/diff/20001/Source/core/svg/SVGPath...
File Source/core/svg/SVGPathBlender.cpp (right):

https://codereview.chromium.org/1056873002/diff/20001/Source/core/svg/SVGPath...
Source/core/svg/SVGPathBlender.cpp:97: FloatPoint
SVGPathBlender::BlendState::blendAnimatedFloatPointSameCoordinates(const
FloatPoint& fromPoint, const FloatPoint& toPoint)
On 2015/04/02 15:00:30, Stephen Chenney wrote:
> Minor nit: Could you swap the order of this and the next to match the header
> declaration order. And put canBlend before both.

Assuming you wanted the order of declarations and definitions to match, I did it
the other way around instead (i.e. reordered the declarations.) This yields a
smaller (more obvious) diff. If you wanted the definitions to actually move
around for another reason let me know and I'll fix that up.

[fs, 2015-04-07 10:40:03]

The CQ bit was checked by fs@opera.com

[fs, 2015-04-07 10:40:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
schenney@chromium.org
Link to the patchset: https://codereview.chromium.org/1056873002/#ps40001
(title: "Re-order declarations.")

[commit-bot: I haz the power, 2015-04-07 10:40:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1056873002/40001

[commit-bot: I haz the power, 2015-04-07 10:43:48]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193262

[Stephen Chennney, 2015-04-07 13:48:26]

On 2015/04/07 08:27:29, fs wrote:
>
https://codereview.chromium.org/1056873002/diff/20001/Source/core/svg/SVGPath...
> File Source/core/svg/SVGPathBlender.cpp (right):
> 
>
https://codereview.chromium.org/1056873002/diff/20001/Source/core/svg/SVGPath...
> Source/core/svg/SVGPathBlender.cpp:97: FloatPoint
> SVGPathBlender::BlendState::blendAnimatedFloatPointSameCoordinates(const
> FloatPoint& fromPoint, const FloatPoint& toPoint)
> On 2015/04/02 15:00:30, Stephen Chenney wrote:
> > Minor nit: Could you swap the order of this and the next to match the header
> > declaration order. And put canBlend before both.
> 
> Assuming you wanted the order of declarations and definitions to match, I did
it
> the other way around instead (i.e. reordered the declarations.) This yields a
> smaller (more obvious) diff. If you wanted the definitions to actually move
> around for another reason let me know and I'll fix that up.

Your solution was better. Thanks.