Remove duplicated code for image-strip computation in InlineFlowBoxPainter

Remove duplicated code for image-strip computation in InlineFlowBoxPainter

The code to compute the "strip"/"slice" of an inline was duplicated for
the border, mask and fill cases.
Move this code to a new method paintRectForImageStrip.

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

[fs, 2014-12-04 14:00:11]

fs@opera.com changed reviewers:
+ chrishtr@chromium.org, leviw@chromium.org

[chrishtr, 2014-12-04 17:25:04]

https://codereview.chromium.org/752373003/diff/1/Source/core/paint/InlineFlow...
File Source/core/paint/InlineFlowBoxPainter.cpp (left):

https://codereview.chromium.org/752373003/diff/1/Source/core/paint/InlineFlow...
Source/core/paint/InlineFlowBoxPainter.cpp:236: // FIXME: What the heck do we do
with RTL here? The math we're using is obviously not right,
Are you sure it's correct to address RTL directly in this code?

https://codereview.chromium.org/752373003/diff/1/Source/core/paint/InlineFlow...
Source/core/paint/InlineFlowBoxPainter.cpp:305: // We need to adjust _tx and _ty
by the width of all previous lines.
Same Q here about RTL. Are we missing RTL test coverage?

[fs, 2014-12-04 17:42:03]

https://codereview.chromium.org/752373003/diff/1/Source/core/paint/InlineFlow...
File Source/core/paint/InlineFlowBoxPainter.cpp (left):

https://codereview.chromium.org/752373003/diff/1/Source/core/paint/InlineFlow...
Source/core/paint/InlineFlowBoxPainter.cpp:236: // FIXME: What the heck do we do
with RTL here? The math we're using is obviously not right,
On 2014/12/04 17:25:03, chrishtr wrote:
> Are you sure it's correct to address RTL directly in this code?

I've not done anything to address this FIXME (so maybe it should still be
there... oops, I'll add it back.) - ie. isLeftToRightDirection is passed as
'true' here.

https://codereview.chromium.org/752373003/diff/1/Source/core/paint/InlineFlow...
Source/core/paint/InlineFlowBoxPainter.cpp:305: // We need to adjust _tx and _ty
by the width of all previous lines.
On 2014/12/04 17:25:03, chrishtr wrote:
> Same Q here about RTL. Are we missing RTL test coverage?

See above. I'm not sure about the test-coverage for RTL, but it's not unlikely
that it's missing. I've not intended to change behavior with this change though
(isLeftToRightDirection == true here and above.)

[chrishtr, 2014-12-04 17:47:57]

https://codereview.chromium.org/752373003/diff/1/Source/core/paint/InlineFlow...
File Source/core/paint/InlineFlowBoxPainter.cpp (left):

https://codereview.chromium.org/752373003/diff/1/Source/core/paint/InlineFlow...
Source/core/paint/InlineFlowBoxPainter.cpp:236: // FIXME: What the heck do we do
with RTL here? The math we're using is obviously not right,
On 2014/12/04 at 17:42:02, fs wrote:
> On 2014/12/04 17:25:03, chrishtr wrote:
> > Are you sure it's correct to address RTL directly in this code?
> 
> I've not done anything to address this FIXME (so maybe it should still be
there... oops, I'll add it back.) - ie. isLeftToRightDirection is passed as
'true' here.

Ah I missed that subtlety.

https://codereview.chromium.org/752373003/diff/1/Source/core/paint/InlineFlow...
File Source/core/paint/InlineFlowBoxPainter.h (right):

https://codereview.chromium.org/752373003/diff/1/Source/core/paint/InlineFlow...
Source/core/paint/InlineFlowBoxPainter.h:35: LayoutRect
paintRectForImageStrip(const LayoutPoint&, const LayoutSize&, bool
isLeftToRightDirection = true) const;
Please change the third arg to a TextDirection and make it required.

https://codereview.chromium.org/752373003/diff/1/Source/core/paint/InlineFlow...
Source/core/paint/InlineFlowBoxPainter.h:35: LayoutRect
paintRectForImageStrip(const LayoutPoint&, const LayoutSize&, bool
isLeftToRightDirection = true) const;
Please change the third arg to a TextDirection and make it required.

[fs, 2014-12-04 18:15:46]

https://codereview.chromium.org/752373003/diff/1/Source/core/paint/InlineFlow...
File Source/core/paint/InlineFlowBoxPainter.cpp (left):

https://codereview.chromium.org/752373003/diff/1/Source/core/paint/InlineFlow...
Source/core/paint/InlineFlowBoxPainter.cpp:236: // FIXME: What the heck do we do
with RTL here? The math we're using is obviously not right,
On 2014/12/04 17:47:57, chrishtr wrote:
> On 2014/12/04 at 17:42:02, fs wrote:
> > On 2014/12/04 17:25:03, chrishtr wrote:
> > > Are you sure it's correct to address RTL directly in this code?
> > 
> > I've not done anything to address this FIXME (so maybe it should still be
> there... oops, I'll add it back.) - ie. isLeftToRightDirection is passed as
> 'true' here.
> 
> Ah I missed that subtlety.

Restored FIXME (and one like it to the mask case.)

https://codereview.chromium.org/752373003/diff/1/Source/core/paint/InlineFlow...
File Source/core/paint/InlineFlowBoxPainter.h (right):

https://codereview.chromium.org/752373003/diff/1/Source/core/paint/InlineFlow...
Source/core/paint/InlineFlowBoxPainter.h:35: LayoutRect
paintRectForImageStrip(const LayoutPoint&, const LayoutSize&, bool
isLeftToRightDirection = true) const;
On 2014/12/04 17:47:57, chrishtr wrote:
> Please change the third arg to a TextDirection and make it required.

Done.

[chrishtr, 2014-12-04 18:21:02]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2014-12-04 18:21:02]

lgtm

[commit-bot: I haz the power, 2014-12-04 18:21:23]

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

[commit-bot: I haz the power, 2014-12-04 19:09:55]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186531