Make RenderObject::style() return a const object

Make RenderObject::style() return a const object

The current code was sloppy and allowed accidental mutation
of RenderStyle outside of style recalc. While this change
doesn't fix that class of bugs (as we introduce some
mutable accessors), it greatly reduces the risk of
them being introduced by mistake.

In order to make the change work, 2 fields on RenderStyle had
to be made mutable (unique and explicitInheritance) because
they are propagated up the tree during style recalc.

Also StyleSelectorState had to be changed to store the override
parent style independently of the regular parent style. That's
required as RefPtr modifies the style by increasing the refCount
and we have to hold onto the override as it can be a free style.

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

[Julien - ping for review, 2015-01-29 16:45:43]

jchaffraix@chromium.org changed reviewers:
+ dmazzoni@chromium.org, pdr@chromium.org, rune@opera.com

[Julien - ping for review, 2015-01-29 16:45:46]

dmazzoni: For modules/accessibility/
rune: For core (and for the core/css/ changes so that you can raise a flag if I
missed something)
pdr: For web/ + it impacts SVG (and to celebrate the New Year ;-))

Massive patch (over 300 files touched), happy to split if you think it's worth
it.

All the build bots compile and tests passed on linux_blink_rel.

[dmazzoni, 2015-01-29 16:52:14]

lgtm for modules/accessibility

[andersr, 2015-01-29 18:10:04]

andersr@opera.com changed reviewers:
+ andersr@opera.com

[andersr, 2015-01-29 18:10:08]

Drive-by review of css/resolver/*:

https://codereview.chromium.org/889563002/diff/20001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/889563002/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:599: if (RenderStyle*
styleOfShadowHost = parent->mutableRenderStyle())
We can use a const RenderStyle here?

[rune, 2015-01-29 18:47:01]

On 2015/01/29 18:10:08, andersr wrote:
> Drive-by review of css/resolver/*:
> 
>
https://codereview.chromium.org/889563002/diff/20001/Source/core/css/resolver...
> File Source/core/css/resolver/StyleResolver.cpp (right):
> 
>
https://codereview.chromium.org/889563002/diff/20001/Source/core/css/resolver...
> Source/core/css/resolver/StyleResolver.cpp:599: if (RenderStyle*
> styleOfShadowHost = parent->mutableRenderStyle())
> We can use a const RenderStyle here?

Looks like it, since it's only used to retriev userModify() which is a const
method.

[rune, 2015-01-29 23:16:32]

I'm through core/ now. That was a lot of files!

I think this is a good change.

https://codereview.chromium.org/889563002/diff/80001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/889563002/diff/80001/Source/core/dom/Element....
Source/core/dom/Element.cpp:1653: RefPtr<RenderStyle> oldStyle =
mutableRenderStyle();
All use of oldStyle below looks const to me.

https://codereview.chromium.org/889563002/diff/80001/Source/core/rendering/Te...
File Source/core/rendering/TextAutosizer.cpp (right):

https://codereview.chromium.org/889563002/diff/80001/Source/core/rendering/Te...
Source/core/rendering/TextAutosizer.cpp:967: RenderStyle* currentStyle =
renderer->deprecatedMutableStyle();
You need a mutable style here for storing a RefPtr in
m_stylesRetainedDuringLayout?

https://codereview.chromium.org/889563002/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/889563002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.h:261: // FIXME: This should be moved to
ElementRateData.
ElementRareData.

https://codereview.chromium.org/889563002/diff/80001/Source/core/svg/SVGEleme...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/889563002/diff/80001/Source/core/svg/SVGEleme...
Source/core/svg/SVGElement.cpp:944: RenderStyle* parentStyle = 0;
Can't parentStyle be const, style() used below, and overrideComputedStyle's
parentStyle parameter be made const?

https://codereview.chromium.org/889563002/diff/80001/Source/core/svg/SVGStopE...
File Source/core/svg/SVGStopElement.cpp (right):

https://codereview.chromium.org/889563002/diff/80001/Source/core/svg/SVGStopE...
Source/core/svg/SVGStopElement.cpp:73: const RenderStyle* style = renderer() ?
renderer()->style() : 0;
Does nullptr instead of 0 work here?

[rune, 2015-01-29 23:21:00]

"becuase" -> "because" in the description.

[Julien - ping for review, 2015-02-04 04:53:13]

On 2015/01/29 at 23:21:00, rune wrote:
> "becuase" -> "because" in the description.

Done too.

I have started splitting out the patch to ease reviews. There was also related
change that could be done that are good (like using references for style()).

[Julien - ping for review, 2015-02-10 07:33:20]

I should have fixed all the issues from the previous review. The patch also has
a reviewable size now.

https://codereview.chromium.org/889563002/diff/80001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/889563002/diff/80001/Source/core/dom/Element....
Source/core/dom/Element.cpp:1653: RefPtr<RenderStyle> oldStyle =
mutableRenderStyle();
On 2015/01/29 23:16:32, rune wrote:
> All use of oldStyle below looks const to me.

RefPtr modifies |oldStyle| so it's not const at all. We could hold onto the raw
pointer but it's tricky as we call setStyle below that could destroy the pointer
under our feet. That's why I think we should stick with mutableRenderStyle here.

https://codereview.chromium.org/889563002/diff/80001/Source/core/rendering/Te...
File Source/core/rendering/TextAutosizer.cpp (right):

https://codereview.chromium.org/889563002/diff/80001/Source/core/rendering/Te...
Source/core/rendering/TextAutosizer.cpp:967: RenderStyle* currentStyle =
renderer->deprecatedMutableStyle();
On 2015/01/29 23:16:32, rune wrote:
> You need a mutable style here for storing a RefPtr in
> m_stylesRetainedDuringLayout?

Indeed.

https://codereview.chromium.org/889563002/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/889563002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.h:261: // FIXME: This should be moved to
ElementRateData.
On 2015/01/29 23:16:32, rune wrote:
> ElementRareData.

Done.

https://codereview.chromium.org/889563002/diff/80001/Source/core/svg/SVGEleme...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/889563002/diff/80001/Source/core/svg/SVGEleme...
Source/core/svg/SVGElement.cpp:944: RenderStyle* parentStyle = 0;
On 2015/01/29 23:16:32, rune wrote:
> Can't parentStyle be const, style() used below, and overrideComputedStyle's
> parentStyle parameter be made const?

You're right. Done.

https://codereview.chromium.org/889563002/diff/80001/Source/core/svg/SVGStopE...
File Source/core/svg/SVGStopElement.cpp (right):

https://codereview.chromium.org/889563002/diff/80001/Source/core/svg/SVGStopE...
Source/core/svg/SVGStopElement.cpp:73: const RenderStyle* style = renderer() ?
renderer()->style() : 0;
On 2015/01/29 23:16:32, rune wrote:
> Does nullptr instead of 0 work here?

Done.

[rune, 2015-02-10 12:03:01]

https://codereview.chromium.org/889563002/diff/140001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/889563002/diff/140001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.h:814: const LayoutStyle& styleRef(bool
firstLine) const;
Why do you need the pointer-return variants?

I assume the pointer is always expected to be non-null since you have reference
returns for the same member?

Is this to avoid a larger size of the patch? work in progress?

https://codereview.chromium.org/889563002/diff/140001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.h:814: const LayoutStyle& styleRef(bool
firstLine) const;
Do we really need the pointer-return variants?

Which are the callers mentioned above that can expect a nullptr?

Would it be possible to hide those accesses inside the LayoutObject
implementation so that we can get away with reference getters only (keeping the
assert in the ref-getter)?

https://codereview.chromium.org/889563002/diff/140001/Source/web/mac/WebSubst...
File Source/web/mac/WebSubstringUtil.mm (right):

https://codereview.chromium.org/889563002/diff/140001/Source/web/mac/WebSubst...
Source/web/mac/WebSubstringUtil.mm:87: font = [NSFont
systemFontOfSize:style.font().fontDescription().computedSize()];
Correct indentation since you're changing this line?

[rune, 2015-02-10 12:03:59]

On 2015/02/10 at 12:03:01, rune wrote:
>
https://codereview.chromium.org/889563002/diff/140001/Source/core/layout/Layo...
> File Source/core/layout/LayoutObject.h (right):
> 
>
https://codereview.chromium.org/889563002/diff/140001/Source/core/layout/Layo...
> Source/core/layout/LayoutObject.h:814: const LayoutStyle& styleRef(bool
firstLine) const;
> Why do you need the pointer-return variants?
> 
> I assume the pointer is always expected to be non-null since you have
reference returns for the same member?
> 
> Is this to avoid a larger size of the patch? work in progress?

Ignore this. It's a comment I drafted before the one below.

>
https://codereview.chromium.org/889563002/diff/140001/Source/core/layout/Layo...
> Source/core/layout/LayoutObject.h:814: const LayoutStyle& styleRef(bool
firstLine) const;
> Do we really need the pointer-return variants?
> 
> Which are the callers mentioned above that can expect a nullptr?
> 
> Would it be possible to hide those accesses inside the LayoutObject
implementation so that we can get away with reference getters only (keeping the
assert in the ref-getter)?

[Julien - ping for review, 2015-02-10 23:10:43]

https://codereview.chromium.org/889563002/diff/140001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/889563002/diff/140001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.h:814: const LayoutStyle& styleRef(bool
firstLine) const;
On 2015/02/10 12:03:01, rune wrote:
> Do we really need the pointer-return variants?

We do. The reason is that there is a small window during which style() can
return nullptr.

> Which are the callers mentioned above that can expect a nullptr?

It should be any code that calls style() before the first setStyleInternal() is
called (in particular styleWillChange)

> Would it be possible to hide those accesses inside the LayoutObject
> implementation so that we can get away with reference getters only (keeping
the
> assert in the ref-getter)?

That's a good question and I don't have a definite answer to that. That's
because I haven't investigated all the callers to see if they *really* need a
nullptr check. I would expect most of these to be unneeded and your proposal
make sense but it's just a hunch based on the short window for a nullptr style.

This is a good follow-up that I would gladly try :)

https://codereview.chromium.org/889563002/diff/140001/Source/web/mac/WebSubst...
File Source/web/mac/WebSubstringUtil.mm (right):

https://codereview.chromium.org/889563002/diff/140001/Source/web/mac/WebSubst...
Source/web/mac/WebSubstringUtil.mm:87: font = [NSFont
systemFontOfSize:style.font().fontDescription().computedSize()];
On 2015/02/10 12:03:01, rune wrote:
> Correct indentation since you're changing this line?

Done.

[rune, 2015-02-10 23:31:20]

On 2015/02/10 at 23:10:43, jchaffraix wrote:
>
https://codereview.chromium.org/889563002/diff/140001/Source/core/layout/Layo...
> File Source/core/layout/LayoutObject.h (right):
> 
>
https://codereview.chromium.org/889563002/diff/140001/Source/core/layout/Layo...
> Source/core/layout/LayoutObject.h:814: const LayoutStyle& styleRef(bool
firstLine) const;
> On 2015/02/10 12:03:01, rune wrote:
> > Do we really need the pointer-return variants?
> 
> We do. The reason is that there is a small window during which style() can
return nullptr.
> 
> > Which are the callers mentioned above that can expect a nullptr?
> 
> It should be any code that calls style() before the first setStyleInternal()
is called (in particular styleWillChange)
> 
> > Would it be possible to hide those accesses inside the LayoutObject
> > implementation so that we can get away with reference getters only (keeping
the
> > assert in the ref-getter)?
> 
> That's a good question and I don't have a definite answer to that. That's
because I haven't investigated all the callers to see if they *really* need a
nullptr check. I would expect most of these to be unneeded and your proposal
make sense but it's just a hunch based on the short window for a nullptr style.
> 
> This is a good follow-up that I would gladly try :)

Great.

>
https://codereview.chromium.org/889563002/diff/140001/Source/web/mac/WebSubst...
> File Source/web/mac/WebSubstringUtil.mm (right):
> 
>
https://codereview.chromium.org/889563002/diff/140001/Source/web/mac/WebSubst...
> Source/web/mac/WebSubstringUtil.mm:87: font = [NSFont
systemFontOfSize:style.font().fontDescription().computedSize()];
> On 2015/02/10 12:03:01, rune wrote:
> > Correct indentation since you're changing this line?
> 
> Done.

lgtm

[rune, 2015-02-10 23:33:03]

On 2015/02/10 at 23:31:20, rune wrote:

> lgtm

You probably need stamping from someone else for modules + web?

[pdr., 2015-02-18 21:59:26]

pdr@chromium.org changed reviewers:
+ dstockwell@chromium.org, mikelawther@chromium.org

[pdr., 2015-02-18 22:30:57]

Source/web LGTM

+cc dstockwell, mikelawther.
Could you please wait for an LGTM from them as well?

https://codereview.chromium.org/889563002/diff/160001/Source/core/layout/styl...
File Source/core/layout/style/LayoutStyle.h (right):

https://codereview.chromium.org/889563002/diff/160001/Source/core/layout/styl...
Source/core/layout/style/LayoutStyle.h:770: ShadowList* boxShadow() const {
return rareNonInheritedData->m_boxShadow.get(); }
Returning non-const pointers from const objects can introduce tricky bugs. Do we
plan on constifying or removing these? (here, and elsewhere in this class)

[dstockwell, 2015-02-19 00:41:51]

lgtm

Would also like to see a list of what could be improved in the future. Are there
bugs filed already?

https://codereview.chromium.org/889563002/diff/160001/Source/core/css/Selecto...
File Source/core/css/SelectorChecker.cpp (right):

https://codereview.chromium.org/889563002/diff/160001/Source/core/css/Selecto...
Source/core/css/SelectorChecker.cpp:693: LayoutStyle* childStyle =
context.elementStyle ? context.elementStyle : element.mutableLayoutStyle();
Why this change? setUnique() is const now (given mutable m_unique)

https://codereview.chromium.org/889563002/diff/160001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolverState.h (left):

https://codereview.chromium.org/889563002/diff/160001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolverState.h:165: RefPtr<LayoutStyle>
m_parentStyle;
What was wrong with just RefPtr<LayoutStyle> m_parentStyle?

[Julien - ping for review, 2015-02-19 19:46:55]

New patchsets have been uploaded after l-g-t-m from
rune@opera.com,pdr@chromium.org,dstockwell@chromium.org

[Julien - ping for review, 2015-02-19 21:04:14]

> Would also like to see a list of what could be improved in the future. Are
there bugs filed already?

I just filed them, feel free to CC interested folks:
http://crbug.com/460226
http://crbug.com/460225
http://crbug.com/460219

https://codereview.chromium.org/889563002/diff/160001/Source/core/css/Selecto...
File Source/core/css/SelectorChecker.cpp (right):

https://codereview.chromium.org/889563002/diff/160001/Source/core/css/Selecto...
Source/core/css/SelectorChecker.cpp:693: LayoutStyle* childStyle =
context.elementStyle ? context.elementStyle : element.mutableLayoutStyle();
On 2015/02/19 00:41:50, dstockwell wrote:
> Why this change? setUnique() is const now (given mutable m_unique)

Good catch, updated.

https://codereview.chromium.org/889563002/diff/160001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolverState.h (left):

https://codereview.chromium.org/889563002/diff/160001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolverState.h:165: RefPtr<LayoutStyle>
m_parentStyle;
On 2015/02/19 00:41:50, dstockwell wrote:
> What was wrong with just RefPtr<LayoutStyle> m_parentStyle?

RefPtr mutates the underlying style and we wanted to pass a const |parentStyle|
so that's why this had to be changed.

[dstockwell, 2015-02-19 23:11:19]

lgtm

https://codereview.chromium.org/889563002/diff/160001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolverState.h (left):

https://codereview.chromium.org/889563002/diff/160001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolverState.h:165: RefPtr<LayoutStyle>
m_parentStyle;
On 2015/02/19 21:04:13, Julien Chaffraix - PST wrote:
> On 2015/02/19 00:41:50, dstockwell wrote:
> > What was wrong with just RefPtr<LayoutStyle> m_parentStyle?
> 
> RefPtr mutates the underlying style and we wanted to pass a const
|parentStyle|
> so that's why this had to be changed.

Ahh, right :(

I'm not sure there's actually a good reason why RefPtr's refcount isn't mutable,
we should be able to use RefPtr of a const -- but that's another change.

[Julien - ping for review, 2015-02-20 00:08:39]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2015-02-20 00:09:26]

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

[commit-bot: I haz the power, 2015-02-20 03:42:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-20 03:42:24]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/4...)

[Julien - ping for review, 2015-02-20 22:31:49]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2015-02-20 22:32:24]

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

[commit-bot: I haz the power, 2015-02-21 00:41:54]

Committed patchset #10 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190605