Push LayoutState from RenderSVGText

Columns Don't Apply to SVG Elements

Column styling is not used on SVG elements so don't allow it to apply when parsing 
the style.

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

[rhogan, 2014-11-03 21:22:55]

robhogan@gmail.com changed reviewers:
+ leviw@chromium.org

[leviw_travelin_and_unemployed, 2014-11-03 21:55:10]

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

[leviw_travelin_and_unemployed, 2014-11-03 21:55:11]

https://codereview.chromium.org/695423002/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGText.cpp (right):

https://codereview.chromium.org/695423002/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGText.cpp:400:
checkForPaginationLogicalHeightChange(pageLogicalHeight,
pageLogicalHeightChanged, hasSpecifiedPageLogicalHeight);
We also ASSERT(!hasColumns()) in RenderSVGText. It seems like there's something
going wrong here, since we won't actually paginate svg text (to my knowledge).

[rhogan, 2014-11-10 19:06:19]

On 2014/11/03 at 21:55:11, leviw wrote:
>
https://codereview.chromium.org/695423002/diff/1/Source/core/rendering/svg/Re...
> Source/core/rendering/svg/RenderSVGText.cpp:400:
checkForPaginationLogicalHeightChange(pageLogicalHeight,
pageLogicalHeightChanged, hasSpecifiedPageLogicalHeight);
> We also ASSERT(!hasColumns()) in RenderSVGText. It seems like there's
something going wrong here, since we won't actually paginate svg text (to my
knowledge).

Yup, was too hasty here. I believe the solution is to ensure column stylings
don't get applied to SVG elements. Columns seem to be the only ones not already
catered for - RenderSVGBlock ensures setOverflowClip() is not set.

[leviw_travelin_and_unemployed, 2014-11-10 21:07:23]

https://codereview.chromium.org/695423002/diff/20001/Source/core/rendering/st...
File Source/core/rendering/style/DataRef.h (right):

https://codereview.chromium.org/695423002/diff/20001/Source/core/rendering/st...
Source/core/rendering/style/DataRef.h:65: void operator=(std::nullptr_t) {
m_data = nullptr; }
This makes a lot more sense as a solution than your previous patch. This part is
the only part I find surprising... Do we really not reset DataRefs elsewhere?
Ideally, we'd just use access() and copy in a default StyleMultiColData (which
could potentially avoid some RefPtr churn). That may be more yak shaving than
this patch is worth, though.

https://codereview.chromium.org/695423002/diff/20001/Source/core/rendering/st...
Source/core/rendering/style/DataRef.h:65: void operator=(std::nullptr_t) {
m_data = nullptr; }
This makes a lot more sense as a solution than your previous patch. This part is
the only part I find surprising... Do we really not reset DataRefs elsewhere?
Ideally, we'd just use access() and copy in a default StyleMultiColData (which
could potentially avoid some RefPtr churn). Seems like a lot of yak shaving,
though. WDYT?

[rhogan, 2014-11-10 21:31:51]

On 2014/11/10 at 21:07:23, leviw wrote:
>
https://codereview.chromium.org/695423002/diff/20001/Source/core/rendering/st...
> Source/core/rendering/style/DataRef.h:65: void operator=(std::nullptr_t) {
m_data = nullptr; }
> This makes a lot more sense as a solution than your previous patch. This part
is the only part I find surprising... Do we really not reset DataRefs elsewhere?


When resetting them we reset their individual members. That's fine when it's
only one or two, but MultiCol has a lot.

> Ideally, we'd just use access() and copy in a default StyleMultiColData (which
could potentially avoid some RefPtr churn). Seems like a lot of yak shaving,
though. WDYT?

That's what init() is doing, but it currently ASSERTS !m_data which is why I
introduced the nullptr operator.

Wouldn't removing the assert or creating a function that just does m_data =
T::create();  not result in the same quantity of RefPtr operations as the
current solution? My thought is that m_data = T::Create() results in the
destruction of the current m_data if it is non-null, which is what the nullptr
operator does.

[leviw_travelin_and_unemployed, 2014-11-10 21:47:37]

On 2014/11/10 at 21:31:51, robhogan wrote:
> On 2014/11/10 at 21:07:23, leviw wrote:
> >
https://codereview.chromium.org/695423002/diff/20001/Source/core/rendering/st...
> > Source/core/rendering/style/DataRef.h:65: void operator=(std::nullptr_t) {
m_data = nullptr; }
> > This makes a lot more sense as a solution than your previous patch. This
part is the only part I find surprising... Do we really not reset DataRefs
elsewhere? 
> 
> When resetting them we reset their individual members. That's fine when it's
only one or two, but MultiCol has a lot.
> 
> > Ideally, we'd just use access() and copy in a default StyleMultiColData
(which could potentially avoid some RefPtr churn). Seems like a lot of yak
shaving, though. WDYT?
> 
> That's what init() is doing, but it currently ASSERTS !m_data which is why I
introduced the nullptr operator.
> 
> Wouldn't removing the assert or creating a function that just does m_data =
T::create();  not result in the same quantity of RefPtr operations as the
current solution? My thought is that m_data = T::Create() results in the
destruction of the current m_data if it is non-null, which is what the nullptr
operator does.

Using the create method would result in the same churn. Honestly, this is fine.
It's just surprising that this change requires the DataRef change. LGTM.

[rhogan, 2014-11-10 21:50:13]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2014-11-10 21:51:35]

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

[commit-bot: I haz the power, 2014-11-10 21:53:44]

Committed patchset #2 (id:20001) as 185066