Delete AuthorStyleInfo and move its data into ComputedStyle.RareNonInheritedData.

Delete AuthorStyleInfo & move its data into ComputedStyle.RareNonInheritedData.

This allows caching of styles that may be affected by LayoutThemes as
the data is now available even when cached.

Now with shiny new layout tests to make sure http://crbug/517567 doesn't 
happen again

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

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

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

[meade_UTC10, 2015-07-27 04:25:47]

meade@chromium.org changed reviewers:
+ dstockwell@chromium.org, ojan@chromium.org, timloh@chromium.org

[Timothy Loh, 2015-07-29 03:37:08]

https://codereview.chromium.org/1251523003/diff/220001/Source/core/css/resolv...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/1251523003/diff/220001/Source/core/css/resolv...
Source/core/css/resolver/StyleResolver.cpp:1316: // Make a copy of the current
UA border and background so we can compare after matching other properties.
Can we just keep using CachedUAStyle and only create it if appearance is set at
this point?

https://codereview.chromium.org/1251523003/diff/220001/Source/core/layout/Lay...
File Source/core/layout/LayoutThemeMac.mm (right):

https://codereview.chromium.org/1251523003/diff/220001/Source/core/layout/Lay...
Source/core/layout/LayoutThemeMac.mm:788: style.setUnique();
We probably don't need to do this since the MPC is used before style adjustmemnt

https://codereview.chromium.org/1251523003/diff/220001/Source/core/style/Comp...
File Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1251523003/diff/220001/Source/core/style/Comp...
Source/core/style/ComputedStyle.h:1567: void setHasAuthorBackground(bool
authorBg) { SET_VAR(rareNonInheritedData, m_hasAuthorBackground, authorBg ? 1 :
0); }
No need to abbreviate background. Also does the ternary do anything here or can
we just pass the bool in?

https://codereview.chromium.org/1251523003/diff/220001/Source/core/style/Styl...
File Source/core/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/1251523003/diff/220001/Source/core/style/Styl...
Source/core/style/StyleRareNonInheritedData.h:207: // Previously stored in
AuthorStyleInfo.
This comment isn't useful. Maybe something like

// Style adjustment for appearance is disabled when certain properties are set

https://codereview.chromium.org/1251523003/diff/220001/Source/core/style/Styl...
Source/core/style/StyleRareNonInheritedData.h:208: unsigned
m_hasAuthorBackground : 1; // Whether there is a user-defined background.
user -> author

[meade_UTC10, 2015-07-30 05:38:18]

https://codereview.chromium.org/1251523003/diff/220001/Source/core/css/resolv...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/1251523003/diff/220001/Source/core/css/resolv...
Source/core/css/resolver/StyleResolver.cpp:1316: // Make a copy of the current
UA border and background so we can compare after matching other properties.
On 2015/07/29 at 03:37:07, Timothy Loh wrote:
> Can we just keep using CachedUAStyle and only create it if appearance is set
at this point?

Done.

https://codereview.chromium.org/1251523003/diff/220001/Source/core/layout/Lay...
File Source/core/layout/LayoutThemeMac.mm (right):

https://codereview.chromium.org/1251523003/diff/220001/Source/core/layout/Lay...
Source/core/layout/LayoutThemeMac.mm:788: style.setUnique();
On 2015/07/29 at 03:37:08, Timothy Loh wrote:
> We probably don't need to do this since the MPC is used before style
adjustmemnt

Removed

https://codereview.chromium.org/1251523003/diff/220001/Source/core/style/Comp...
File Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1251523003/diff/220001/Source/core/style/Comp...
Source/core/style/ComputedStyle.h:1567: void setHasAuthorBackground(bool
authorBg) { SET_VAR(rareNonInheritedData, m_hasAuthorBackground, authorBg ? 1 :
0); }
On 2015/07/29 at 03:37:08, Timothy Loh wrote:
> No need to abbreviate background. Also does the ternary do anything here or
can we just pass the bool in?

SET_VAR seems to do bitwise operations, and I didn't want to rely on the implict
0/1 value of bool. It will probably work by just passing in the bool. Changed.

https://codereview.chromium.org/1251523003/diff/220001/Source/core/style/Styl...
File Source/core/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/1251523003/diff/220001/Source/core/style/Styl...
Source/core/style/StyleRareNonInheritedData.h:207: // Previously stored in
AuthorStyleInfo.
On 2015/07/29 at 03:37:08, Timothy Loh wrote:
> This comment isn't useful. Maybe something like
> 
> // Style adjustment for appearance is disabled when certain properties are set

Done.

https://codereview.chromium.org/1251523003/diff/220001/Source/core/style/Styl...
Source/core/style/StyleRareNonInheritedData.h:208: unsigned
m_hasAuthorBackground : 1; // Whether there is a user-defined background.
On 2015/07/29 at 03:37:08, Timothy Loh wrote:
> user -> author

Done.

[Timothy Loh, 2015-07-30 05:44:34]

lgtm

[meade_UTC10, 2015-07-30 05:51:09]

The CQ bit was checked by meade@chromium.org

[commit-bot: I haz the power, 2015-07-30 05:51:22]

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

[commit-bot: I haz the power, 2015-07-30 05:54:05]

Committed patchset #15 (id:280001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199708

[Yuta Kitamura, 2015-07-30 06:52:35]

On 2015/07/30 05:54:05, commit-bot: I haz the power wrote:
> Committed patchset #15 (id:280001) as
> https://src.chromium.org/viewvc/blink?view=rev&revision=199708

Hi,

This patch has caused a massive number of failures on the Blink
MSAN bot:

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/bu...

I'm going to revert this patch for now, unless I hear from you.

A sample error log:

23:42:05.092 17653   ==4==WARNING: MemorySanitizer: use-of-uninitialized-value
23:42:05.092 17653       #0 0x7f83c3e357c7 in setHasAuthorBackground
third_party/WebKit/Source/core/style/ComputedStyle.h:1567:58
23:42:05.093 17653       #1 0x7f83c3e357c7 in
blink::StyleResolver::applyMatchedProperties(blink::StyleResolverState&,
blink::MatchResult const&)
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:1326:0
23:42:05.093 17653       #2 0x7f83c3e30d22 in
blink::StyleResolver::styleForElement(blink::Element*, blink::ComputedStyle
const*, blink::StyleSharingBehavior, blink::RuleMatchingBehavior)
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:589:9
23:42:05.093 17653       #3 0x7f83c2cf5061 in
blink::Element::originalStyleForLayoutObject()
third_party/WebKit/Source/core/dom/Element.cpp:1619:12
23:42:05.093 17653       #4 0x7f83c3450113 in
blink::HTMLInputElement::customStyleForLayoutObject()
third_party/WebKit/Source/core/html/HTMLInputElement.cpp:1902:56
<snip>
23:42:05.096 17653     Uninitialized value was stored to memory at
23:42:05.097 17653       #0 0x7f83c569d2f2 in
blink::StyleRareNonInheritedData::StyleRareNonInheritedData(blink::StyleRareNonInheritedData
const&) third_party/WebKit/Source/core/style/StyleRareNonInheritedData.cpp:202:7
23:42:05.097 17653       #1 0x7f83c53cdc24 in copy
third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h:81:78
23:42:05.097 17653       #2 0x7f83c53cdc24 in access
third_party/WebKit/Source/core/style/DataRef.h:41:0
23:42:05.097 17653       #3 0x7f83c53cdc24 in
blink::ComputedStyle::setAppearance(blink::ControlPart)
third_party/WebKit/Source/core/style/ComputedStyle.h:1256:0
23:42:05.097 17653       #4 0x7f83c3e49b01 in void
blink::StyleResolver::applyProperties<(blink::CSSPropertyPriority)1>(blink::StyleResolverState&,
blink::StylePropertySet const*, bool, bool, blink::PropertyWhitelistType)
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:1198:9
<snip>
23:42:05.098 17653     Uninitialized value was created by a heap allocation
23:42:05.098 17653       #0 0x7f83bcaa26e2 in __interceptor_malloc ??:0:0
23:42:05.098 17653       #1 0x7f83bf208872 in partitionAllocGenericFlags
third_party/WebKit/Source/wtf/PartitionAlloc.h:676:20
23:42:05.098 17653       #2 0x7f83bf208872 in partitionAllocGeneric
third_party/WebKit/Source/wtf/PartitionAlloc.h:692:0
23:42:05.098 17653       #3 0x7f83bf208872 in WTF::fastMalloc(unsigned long)
third_party/WebKit/Source/wtf/FastMalloc.cpp:56:0
23:42:05.098 17653       #4 0x7f83c53cdbd9 in operator new
third_party/WebKit/Source/wtf/RefCounted.h:166:5
23:42:05.098 17653       #5 0x7f83c53cdbd9 in copy
third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h:81:0
23:42:05.098 17653       #6 0x7f83c53cdbd9 in access
third_party/WebKit/Source/core/style/DataRef.h:41:0
23:42:05.098 17653       #7 0x7f83c53cdbd9 in
blink::ComputedStyle::setAppearance(blink::ControlPart)
third_party/WebKit/Source/core/style/ComputedStyle.h:1256:0
23:42:05.098 17653       #8 0x7f83c3e49b01 in void
blink::StyleResolver::applyProperties<(blink::CSSPropertyPriority)1>(blink::StyleResolverState&,
blink::StylePropertySet const*, bool, bool, blink::PropertyWhitelistType)
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:1198:9
23:42:05.098 17653       #9 0x7f83c3e3dd6b in void
blink::StyleResolver::applyMatchedProperties<(blink::CSSPropertyPriority)1>(blink::StyleResolverState&,
blink::MatchResult const&, bool, unsigned int, unsigned int, bool)
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:1226:9
23:42:05.098 17653       #10 0x7f83c3e344b9 in
blink::StyleResolver::applyMatchedProperties(blink::StyleResolverState&,
blink::MatchResult const&)
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:1314:5
23:42:05.098 17653       #11 0x7f83c3e30d22 in
blink::StyleResolver::styleForElement(blink::Element*, blink::ComputedStyle
const*, blink::StyleSharingBehavior, blink::RuleMatchingBehavior)
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:589:9
23:42:05.098 17653       #12 0x7f83c2cf5061 in
blink::Element::originalStyleForLayoutObject()
third_party/WebKit/Source/core/dom/Element.cpp:1619:12
<snip>
23:42:05.099 17653   SUMMARY: MemorySanitizer: use-of-uninitialized-value
(/b/build/slave/WebKit_Linux_MSAN/build/src/out/Release/content_shell+0x7a2c7c7)
23:42:05.099 17653   Exiting

[Yuta Kitamura, 2015-07-30 07:16:02]

A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/1266723004/ by yutak@chromium.org.

The reason for reverting is: Broke Blink MSAN bot.

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/bu....

[meade_UTC10, 2015-07-31 05:59:48]

Hi Tim, Doug, Ojan

Retrying this without the uninitialized variable (whoops!). PTAL!

Thanks
--Eddy

[Timothy Loh, 2015-08-03 04:45:15]

On 2015/07/31 05:59:48, Eddy wrote:
> Hi Tim, Doug, Ojan
> 
> Retrying this without the uninitialized variable (whoops!). PTAL!
> 
> Thanks
> --Eddy

still lgtm

[meade_UTC10, 2015-08-03 04:47:05]

The CQ bit was checked by meade@chromium.org

[commit-bot: I haz the power, 2015-08-03 04:47:13]

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

[commit-bot: I haz the power, 2015-08-03 06:30:28]

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

[commit-bot: I haz the power, 2015-08-03 06:30:29]

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

[meade_UTC10, 2015-08-03 06:40:45]

The CQ bit was checked by meade@chromium.org

[commit-bot: I haz the power, 2015-08-03 06:40:58]

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

[commit-bot: I haz the power, 2015-08-03 10:34:17]

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

[commit-bot: I haz the power, 2015-08-03 10:34:18]

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

[meade_UTC10, 2015-08-06 04:00:13]

The CQ bit was checked by meade@chromium.org

[meade_UTC10, 2015-08-06 04:00:13]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1251523003/#ps380001
(title: " ")

[commit-bot: I haz the power, 2015-08-06 04:00:36]

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

[commit-bot: I haz the power, 2015-08-06 04:45:32]

Committed patchset #20 (id:380001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200094

[caseq, 2015-08-06 18:37:36]

A revert of this CL (patchset #20 id:380001) has been created in
https://codereview.chromium.org/1274263003/ by caseq@chromium.org.

The reason for reverting is: This breaks button styling in DevTools in a lot of
fancy ways.

BUG=517567.

[meade_UTC10, 2015-08-25 05:51:49]

The CQ bit was checked by meade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-25 05:52:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1251523003/530001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1251523003/530001

[commit-bot: I haz the power, 2015-08-25 06:57:00]

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

[commit-bot: I haz the power, 2015-08-25 06:57:01]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Timothy Loh, 2015-08-25 08:08:20]

lgtm

https://codereview.chromium.org/1251523003/diff/530001/LayoutTests/fast/css/c...
File LayoutTests/fast/css/cache/matched-properties-cache-partial-match.html
(right):

https://codereview.chromium.org/1251523003/diff/530001/LayoutTests/fast/css/c...
LayoutTests/fast/css/cache/matched-properties-cache-partial-match.html:1: <html>
usually we use <!doctype html> and leave out the html, head, body tags

https://codereview.chromium.org/1251523003/diff/530001/LayoutTests/fast/css/c...
LayoutTests/fast/css/cache/matched-properties-cache-partial-match.html:3: <style
type="text/css">
<style>

https://codereview.chromium.org/1251523003/diff/530001/LayoutTests/fast/css/c...
LayoutTests/fast/css/cache/matched-properties-cache-partial-match.html:7:
.button {
could just do a tag name selector, i.e. button { .. }

https://codereview.chromium.org/1251523003/diff/530001/Source/core/css/resolv...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/1251523003/diff/530001/Source/core/css/resolv...
Source/core/css/resolver/StyleResolver.cpp:1398: // When there is a partial
match in the MatchedPropertiesCache, the non-inherited values are copied from
This comment took me a couple of reads to get, maybe just saying
// When there is a partial match in the MatchedPropertiesCache, these flags
// will already be set correctly and the computation here would be incorrect.
is enough? Since there's a test, it should be easy enough for someone to see
that the check is at least needed.

[meade_UTC10, 2015-08-25 08:22:51]

PTAL, thanks!

https://codereview.chromium.org/1251523003/diff/530001/LayoutTests/fast/css/c...
File LayoutTests/fast/css/cache/matched-properties-cache-partial-match.html
(right):

https://codereview.chromium.org/1251523003/diff/530001/LayoutTests/fast/css/c...
LayoutTests/fast/css/cache/matched-properties-cache-partial-match.html:1: <html>
On 2015/08/25 at 08:08:20, Timothy Loh wrote:
> usually we use <!doctype html> and leave out the html, head, body tags

Done.

https://codereview.chromium.org/1251523003/diff/530001/LayoutTests/fast/css/c...
LayoutTests/fast/css/cache/matched-properties-cache-partial-match.html:3: <style
type="text/css">
On 2015/08/25 at 08:08:20, Timothy Loh wrote:
> <style>

Done.

https://codereview.chromium.org/1251523003/diff/530001/LayoutTests/fast/css/c...
LayoutTests/fast/css/cache/matched-properties-cache-partial-match.html:7:
.button {
On 2015/08/25 at 08:08:20, Timothy Loh wrote:
> could just do a tag name selector, i.e. button { .. }

Done.

https://codereview.chromium.org/1251523003/diff/530001/Source/core/css/resolv...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/1251523003/diff/530001/Source/core/css/resolv...
Source/core/css/resolver/StyleResolver.cpp:1398: // When there is a partial
match in the MatchedPropertiesCache, the non-inherited values are copied from
On 2015/08/25 at 08:08:20, Timothy Loh wrote:
> This comment took me a couple of reads to get, maybe just saying
> // When there is a partial match in the MatchedPropertiesCache, these flags
> // will already be set correctly and the computation here would be incorrect.
> is enough? Since there's a test, it should be easy enough for someone to see
that the check is at least needed.

The comment about what happens in cacheUserAgentBorderAndBackground was the key
to understanding why this is the case for me. How about this?

[meade_UTC10, 2015-08-26 06:44:35]

The CQ bit was checked by meade@chromium.org

[meade_UTC10, 2015-08-26 06:44:36]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1251523003/#ps570001
(title: "update comment")

[commit-bot: I haz the power, 2015-08-26 06:44:44]

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

[commit-bot: I haz the power, 2015-08-26 06:48:45]

Committed patchset #31 (id:570001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201208