Move painting code from InlineTextBox to InlineTextBoxPainter.

Move painting code from InlineTextBox to InlineTextBoxPainter.

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

[chrishtr, 2014-10-03 16:28:04]

chrishtr@chromium.org changed reviewers:
+ jbroman@chromium.org

[jbroman, 2014-10-03 17:46:17]

I assume that most of this is code movement, so I haven't inspected each and
every line of it closely.

Have you checked that this has no significant perf impact? I've had surprising
impact on record time from subtle changes to InlineTextBox painting before.

https://codereview.chromium.org/620243002/diff/20001/Source/core/paint/Inline...
File Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/620243002/diff/20001/Source/core/paint/Inline...
Source/core/paint/InlineTextBoxPainter.cpp:96:
context->concatCTM(m_inlineTextBox.rotation(boxRect, InlineTextBox::Clockwise));
InlineTextBox::rotation is static. It seems funny to call it through a
non-static reference.
(here and elsewhere)

https://codereview.chromium.org/620243002/diff/20001/Source/core/paint/Inline...
Source/core/paint/InlineTextBoxPainter.cpp:187: cachedTextBlob =
&gTextBlobCache->add(&m_inlineTextBox, nullptr).storedValue->value;
Sharing this member this way seems fishy. If I were editing InlineTextBox.cpp,
I'd have only the slightest clue that changing gTextBlobCache could break this
file.

How about either:

(a) moving these three lines into a method on InlineTextBox, or
(b) alternatively moving the hash entirely to this translation unit and exposing
a  static "InlineTextBoxPainter::removeFromTextBlobCache" method to use in
InlineTextBox.cpp? My CL to remove the start/length setters does something
similar to that
(https://codereview.chromium.org/622253002/diff/40001/Source/core/rendering/In...).

Depending on whether you think ITB or ITBP ought to "own" the cache.

https://codereview.chromium.org/620243002/diff/20001/Source/core/paint/Inline...
File Source/core/paint/InlineTextBoxPainter.h (right):

https://codereview.chromium.org/620243002/diff/20001/Source/core/paint/Inline...
Source/core/paint/InlineTextBoxPainter.h:18: struct PaintInfo;
forward declaration sort order

https://codereview.chromium.org/620243002/diff/20001/Source/core/paint/Inline...
Source/core/paint/InlineTextBoxPainter.h:25: InlineTextBoxPainter(InlineTextBox&
inlineTextBox) : m_inlineTextBox(inlineTextBox) { }
nit: Any reason this is a non-const ref? There are a handful of members it
accesses that are non-const, but most (maybe all) of them ought to be const.

I'm fine with leaving this for now if it's non-trivial to change.

https://codereview.chromium.org/620243002/diff/20001/Source/core/rendering/In...
File Source/core/rendering/InlineTextBox.h (right):

https://codereview.chromium.org/620243002/diff/20001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.h:105: LayoutUnit selectionTop();
This is missing an implementation.

Did you mean to remove this line too (and then presumably the one above it,
since this visibility section becomes empty)?

https://codereview.chromium.org/620243002/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGInlineTextBox.cpp (right):

https://codereview.chromium.org/620243002/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGInlineTextBox.cpp:267:
InlineTextBoxPainter(*this).paintDocumentMarkers(paintInfo.context, paintOffset,
style, textRenderer.scaledFont(), true);
Safe to assume there will eventually be an SVGInlineTextBoxPainter as well?

[jbroman, 2014-10-03 17:48:44]

https://codereview.chromium.org/620243002/diff/20001/Source/core/paint/Inline...
File Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/620243002/diff/20001/Source/core/paint/Inline...
Source/core/paint/InlineTextBoxPainter.cpp:96:
context->concatCTM(m_inlineTextBox.rotation(boxRect, InlineTextBox::Clockwise));
On 2014/10/03 17:46:17, jbroman wrote:
> InlineTextBox::rotation is static. It seems funny to call it through a
> non-static reference.
> (here and elsewhere)

Alternatively, maybe rotation() belongs in InlineTextBoxPainter or TextPainter.
It's a little odd, but it's never really had a home that felt right.

[chrishtr, 2014-10-03 18:10:37]

Regarding perf: no I did not run any perf tests. Will chat offine with you about
it to get the best approach.

https://codereview.chromium.org/620243002/diff/20001/Source/core/paint/Inline...
File Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/620243002/diff/20001/Source/core/paint/Inline...
Source/core/paint/InlineTextBoxPainter.cpp:96:
context->concatCTM(m_inlineTextBox.rotation(boxRect, InlineTextBox::Clockwise));
On 2014/10/03 17:48:43, jbroman wrote:
> On 2014/10/03 17:46:17, jbroman wrote:
> > InlineTextBox::rotation is static. It seems funny to call it through a
> > non-static reference.
> > (here and elsewhere)
> 
> Alternatively, maybe rotation() belongs in InlineTextBoxPainter or
TextPainter.
> It's a little odd, but it's never really had a home that felt right.

Made it static.

https://codereview.chromium.org/620243002/diff/20001/Source/core/paint/Inline...
Source/core/paint/InlineTextBoxPainter.cpp:187: cachedTextBlob =
&gTextBlobCache->add(&m_inlineTextBox, nullptr).storedValue->value;
On 2014/10/03 17:46:17, jbroman wrote:
> Sharing this member this way seems fishy. If I were editing InlineTextBox.cpp,
> I'd have only the slightest clue that changing gTextBlobCache could break this
> file.
> 
> How about either:
> 
> (a) moving these three lines into a method on InlineTextBox, or
> (b) alternatively moving the hash entirely to this translation unit and
exposing
> a  static "InlineTextBoxPainter::removeFromTextBlobCache" method to use in
> InlineTextBox.cpp? My CL to remove the start/length setters does something
> similar to that
>
(https://codereview.chromium.org/622253002/diff/40001/Source/core/rendering/In...).
> 
> Depending on whether you think ITB or ITBP ought to "own" the cache.

Good points.

Went with option (b).

https://codereview.chromium.org/620243002/diff/20001/Source/core/paint/Inline...
File Source/core/paint/InlineTextBoxPainter.h (right):

https://codereview.chromium.org/620243002/diff/20001/Source/core/paint/Inline...
Source/core/paint/InlineTextBoxPainter.h:18: struct PaintInfo;
On 2014/10/03 at 17:46:17, jbroman wrote:
> forward declaration sort order

Done.

https://codereview.chromium.org/620243002/diff/20001/Source/core/paint/Inline...
Source/core/paint/InlineTextBoxPainter.h:25: InlineTextBoxPainter(InlineTextBox&
inlineTextBox) : m_inlineTextBox(inlineTextBox) { }
On 2014/10/03 at 17:46:17, jbroman wrote:
> nit: Any reason this is a non-const ref? There are a handful of members it
accesses that are non-const, but most (maybe all) of them ought to be const.
> 
> I'm fine with leaving this for now if it's non-trivial to change.

const-ness is not preserved on some RenderObject methods, so in all of these
paint/ CLs I've been leaving it non-const.

https://codereview.chromium.org/620243002/diff/20001/Source/core/rendering/In...
File Source/core/rendering/InlineTextBox.h (right):

https://codereview.chromium.org/620243002/diff/20001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.h:105: LayoutUnit selectionTop();
On 2014/10/03 at 17:46:17, jbroman wrote:
> This is missing an implementation.
> 
> Did you mean to remove this line too (and then presumably the one above it,
since this visibility section becomes empty)?

Fixed.

https://codereview.chromium.org/620243002/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGInlineTextBox.cpp (right):

https://codereview.chromium.org/620243002/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGInlineTextBox.cpp:267:
InlineTextBoxPainter(*this).paintDocumentMarkers(paintInfo.context, paintOffset,
style, textRenderer.scaledFont(), true);
On 2014/10/03 at 17:46:17, jbroman wrote:
> Safe to assume there will eventually be an SVGInlineTextBoxPainter as well?

Yes. Working on that CL in parallel with this review.

[jbroman, 2014-10-03 18:18:26]

lgtm

https://codereview.chromium.org/620243002/diff/20001/Source/core/paint/Inline...
File Source/core/paint/InlineTextBoxPainter.h (right):

https://codereview.chromium.org/620243002/diff/20001/Source/core/paint/Inline...
Source/core/paint/InlineTextBoxPainter.h:18: struct PaintInfo;
On 2014/10/03 18:10:37, chrishtr wrote:
> On 2014/10/03 at 17:46:17, jbroman wrote:
> > forward declaration sort order
> 
> Done.

Hmm, I'd either expect this to be sorted with the Ps, or I'd expect both structs
(CompositionUnderline and PaintInfo) to be together. I've seen both in Blink
(and the style guide seems silent on the subject).

[chrishtr, 2014-10-03 18:37:46]

https://codereview.chromium.org/620243002/diff/20001/Source/core/paint/Inline...
File Source/core/paint/InlineTextBoxPainter.h (right):

https://codereview.chromium.org/620243002/diff/20001/Source/core/paint/Inline...
Source/core/paint/InlineTextBoxPainter.h:18: struct PaintInfo;
On 2014/10/03 18:18:26, jbroman wrote:
> On 2014/10/03 18:10:37, chrishtr wrote:
> > On 2014/10/03 at 17:46:17, jbroman wrote:
> > > forward declaration sort order
> > 
> > Done.
> 
> Hmm, I'd either expect this to be sorted with the Ps, or I'd expect both
structs
> (CompositionUnderline and PaintInfo) to be together. I've seen both in Blink
> (and the style guide seems silent on the subject).

Fixed.

[chrishtr, 2014-10-03 18:43:31]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-10-03 18:44:07]

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

[commit-bot: I haz the power, 2014-10-03 20:27:03]

Committed patchset #5 (id:80001) as 183212