[Line Layout API] Some harder SVGTextLayoutEngine changes

[Line Layout API] Finish switching SVGTextLayoutEngine to line layout API

This is the last use of InlineBox::layoutObject(), so make it private.

It required an additional api method: LineLayoutItem::isSVGText().

BUG=499321
Committed: https://crrev.com/186d9f5ab877027c814438621fcfdd2107d9702d
Cr-Commit-Position: refs/heads/master@{#375113}

[dgrogan, 2016-01-23 03:34:16]

dgrogan@chromium.org changed reviewers:
+ pilgrim@chromium.org

[dgrogan, 2016-01-23 03:34:16]

Mark,

I got confused at the first non-trivial conversion. I've got some questions
embedded as comments. Let me know if you'd rather VC and I'll throw something on
your calendar.

https://codereview.chromium.org/1625683003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp (right):

https://codereview.chromium.org/1625683003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:256: const
LayoutObject* layoutObject =
LineLayoutAPIShim::constLayoutObjectFrom(lineLayoutItem);
This feels like a cop-out but it appears that the rule you've been following is
anything not in core/layout/ gets the shim for now?

[pilgrim_google, 2016-01-27 16:55:27]

On 2016/01/23 at 03:34:16, dgrogan wrote:
> Mark,
> 
> I got confused at the first non-trivial conversion. I've got some questions
embedded as comments. Let me know if you'd rather VC and I'll throw something on
your calendar.
> 
>
https://codereview.chromium.org/1625683003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp (right):
> 
>
https://codereview.chromium.org/1625683003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:256: const
LayoutObject* layoutObject =
LineLayoutAPIShim::constLayoutObjectFrom(lineLayoutItem);
> This feels like a cop-out but it appears that the rule you've been following
is anything not in core/layout/ gets the shim for now?

Levi, did you get my questions about whether we should add LineLayoutSVGTextPath
to the line layout API?

[dgrogan, 2016-02-11 17:39:19]

Description was changed from

==========
[Line Layout API] Some harder SVGTextLayoutEngine changes

BUG=499321
==========

to

==========
[Line Layout API] Finish switching SVGTextLayoutEngine to line layout API

This is the last use of InlineBox::layoutObject(), so make it private.

It required a shim in core/svg/SVGTextContentElement.h.

BUG=499321
==========

[dgrogan, 2016-02-11 17:42:03]

dgrogan@chromium.org changed reviewers:
+ leviw@chromium.org

[dgrogan, 2016-02-11 17:42:03]

Mark and Levi, could you take a look? I won't submit until I have 2 lgtms.

[pilgrim_google, 2016-02-11 17:46:33]

https://codereview.chromium.org/1625683003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp (right):

https://codereview.chromium.org/1625683003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:255: {
Rename this to elementFromLineLayoutItem

https://codereview.chromium.org/1625683003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:256: const
LayoutObject* layoutObject =
LineLayoutAPIShim::constLayoutObjectFrom(lineLayoutItem);
I don't think we need the shim at all. The only methods called on the
layoutObject are isSVGText(), isSVGInline(), and node(). All of these exist on
LineLayoutItem (and if they don't, we need to add them!)

[dgrogan, 2016-02-11 17:59:46]

Description was changed from

==========
[Line Layout API] Finish switching SVGTextLayoutEngine to line layout API

This is the last use of InlineBox::layoutObject(), so make it private.

It required a shim in core/svg/SVGTextContentElement.h.

BUG=499321
==========

to

==========
[Line Layout API] Finish switching SVGTextLayoutEngine to line layout API

This is the last use of InlineBox::layoutObject(), so make it private.

It required an additional api method: LineLayoutItem::isSVGText().

BUG=499321
==========

[dgrogan, 2016-02-11 18:05:10]

https://codereview.chromium.org/1625683003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp (right):

https://codereview.chromium.org/1625683003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:255: {
On 2016/02/11 17:46:33, pilgrim_google wrote:
> Rename this to elementFromLineLayoutItem

Done.

https://codereview.chromium.org/1625683003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:256: const
LayoutObject* layoutObject =
LineLayoutAPIShim::constLayoutObjectFrom(lineLayoutItem);
On 2016/02/11 17:46:33, pilgrim_google wrote:
> I don't think we need the shim at all. The only methods called on the
> layoutObject are isSVGText(), isSVGInline(), and node(). All of these exist on
> LineLayoutItem (and if they don't, we need to add them!)

You're right. Done.

https://codereview.chromium.org/1625683003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:257: if
(!layoutObject)
I'm a little nervous about getting rid of this check but fast/svg/ and svg/ pass
locally...

[pilgrim_google, 2016-02-11 18:19:28]

https://codereview.chromium.org/1625683003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp (left):

https://codereview.chromium.org/1625683003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:255: if
(!layoutObject)
Yeah, that worries me too. Levi needs to weigh in on whether this is acceptable,
or how we should solve it. I don't remember if the magical operators we put on
LineLayoutItem extend to null checks, or if we would need to code a specific
isNull() method to check the underlying LayoutObject.

[pilgrim_google, 2016-02-11 18:20:07]

On 2016/02/11 at 18:19:28, pilgrim_google wrote:
>
https://codereview.chromium.org/1625683003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp (left):
> 
>
https://codereview.chromium.org/1625683003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:255: if
(!layoutObject)
> Yeah, that worries me too. Levi needs to weigh in on whether this is
acceptable, or how we should solve it. I don't remember if the magical operators
we put on LineLayoutItem extend to null checks, or if we would need to code a
specific isNull() method to check the underlying LayoutObject.

The rest of the new patch looks good.

[dgrogan, 2016-02-11 19:12:34]

https://codereview.chromium.org/1625683003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp (left):

https://codereview.chromium.org/1625683003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:255: if
(!layoutObject)
On 2016/02/11 18:19:28, pilgrim_google wrote:
> Yeah, that worries me too. Levi needs to weigh in on whether this is
acceptable,
> or how we should solve it. I don't remember if the magical operators we put on
> LineLayoutItem extend to null checks, or if we would need to code a specific
> isNull() method to check the underlying LayoutObject.

What operators are you referring to? I only see 1:
operator LayoutObject*() const { return m_layoutObject; }

The more I think about it, I don't see how we can guarantee the underlying
object isn't null; I think we need to expose an isNull(). But I'd love it if
Levi somehow knew that we didn't.

[leviw_travelin_and_unemployed, 2016-02-11 21:35:02]

LGTM.

https://codereview.chromium.org/1625683003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/1625683003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.h:427: LayoutObject&
layoutObject() const { return m_layoutObject; }
woohoo :D

https://codereview.chromium.org/1625683003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp (left):

https://codereview.chromium.org/1625683003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:255: if
(!layoutObject)
On 2016/02/11 at 19:12:33, dgrogan wrote:
> On 2016/02/11 18:19:28, pilgrim_google wrote:
> > Yeah, that worries me too. Levi needs to weigh in on whether this is
acceptable,
> > or how we should solve it. I don't remember if the magical operators we put
on
> > LineLayoutItem extend to null checks, or if we would need to code a specific
> > isNull() method to check the underlying LayoutObject.
> 
> What operators are you referring to? I only see 1:
> operator LayoutObject*() const { return m_layoutObject; }
> 
> The more I think about it, I don't see how we can guarantee the underlying
object isn't null; I think we need to expose an isNull(). But I'd love it if
Levi somehow knew that we didn't.


Ahh, allow me to introduce you to the magical world of UnspecifiedBoolType!
https://code.google.com/p/chromium/codesearch#search/&q=UnspecifiedBoolType&s...

LineLayoutItem needs to have an operator UnspecifiedBoolType(). You can use any
of the other versions as a guide :)

[dgrogan, 2016-02-11 23:36:02]

Take another quick look? There is less new code change than rietveld comments.

https://codereview.chromium.org/1625683003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp (left):

https://codereview.chromium.org/1625683003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:255: if
(!layoutObject)
> Ahh, allow me to introduce you to the magical world of UnspecifiedBoolType!
>
https://code.google.com/p/chromium/codesearch#search/&q=UnspecifiedBoolType&s...
> 
> LineLayoutItem needs to have an operator UnspecifiedBoolType(). You can use
any
> of the other versions as a guide :)

For anyone following along at home, Levi and I discussed on chat: having both
UnspecifiedBoolType AND LayoutObject* operator() brings unknown complexity and
there are enough uses non-bool uses of the operator that we should eliminate
them in a separate patch. Then when they are gone we can replace isNull() and
LayoutObject* operator() with UnspecifiedBoolType.

Or C++11 will have possibly saved us by then and we can just use explicit
operator bool() instead of UnspecifiedBoolType hackery. But I've already passed
the frontier of my C++ knowledge.

https://codereview.chromium.org/1625683003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp (left):

https://codereview.chromium.org/1625683003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:255: if
(!layoutObject)
In the meantime this nullptr check lives on as isNull().

[pilgrim_google, 2016-02-12 01:22:42]

On 2016/02/11 at 23:36:02, dgrogan wrote:
> Take another quick look? There is less new code change than rietveld comments.
> 
>
https://codereview.chromium.org/1625683003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp (left):
> 
>
https://codereview.chromium.org/1625683003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:255: if
(!layoutObject)
> > Ahh, allow me to introduce you to the magical world of UnspecifiedBoolType!
> >
https://code.google.com/p/chromium/codesearch#search/&q=UnspecifiedBoolType&s...
> > 
> > LineLayoutItem needs to have an operator UnspecifiedBoolType(). You can use
any
> > of the other versions as a guide :)
> 
> For anyone following along at home, Levi and I discussed on chat: having both
UnspecifiedBoolType AND LayoutObject* operator() brings unknown complexity and
there are enough uses non-bool uses of the operator that we should eliminate
them in a separate patch. Then when they are gone we can replace isNull() and
LayoutObject* operator() with UnspecifiedBoolType.
> 
> Or C++11 will have possibly saved us by then and we can just use explicit
operator bool() instead of UnspecifiedBoolType hackery. But I've already passed
the frontier of my C++ knowledge.
> 
>
https://codereview.chromium.org/1625683003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp (left):
> 
>
https://codereview.chromium.org/1625683003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp:255: if
(!layoutObject)
> In the meantime this nullptr check lives on as isNull().

LGTM

[dgrogan, 2016-02-12 02:41:36]

The CQ bit was checked by dgrogan@chromium.org

[dgrogan, 2016-02-12 02:41:36]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/1625683003/#ps80001
(title: "readd null check with new isNull() method on LineLayoutItem")

[commit-bot: I haz the power, 2016-02-12 02:41:58]

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

[commit-bot: I haz the power, 2016-02-12 03:48:14]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-16 22:41:27]

Description was changed from

==========
[Line Layout API] Finish switching SVGTextLayoutEngine to line layout API

This is the last use of InlineBox::layoutObject(), so make it private.

It required an additional api method: LineLayoutItem::isSVGText().

BUG=499321
==========

to

==========
[Line Layout API] Finish switching SVGTextLayoutEngine to line layout API

This is the last use of InlineBox::layoutObject(), so make it private.

It required an additional api method: LineLayoutItem::isSVGText().

BUG=499321

Committed: https://crrev.com/186d9f5ab877027c814438621fcfdd2107d9702d
Cr-Commit-Position: refs/heads/master@{#375113}
==========

[commit-bot: I haz the power, 2016-02-16 22:41:28]

Patchset 5 (id:??) landed as
https://crrev.com/186d9f5ab877027c814438621fcfdd2107d9702d
Cr-Commit-Position: refs/heads/master@{#375113}