Track reasons for |Node::SetNeedsStyleRecalc|

Track reasons for |Node::SetNeedsStyleRecalc|

Based on Eric Williger's patch: https://crrev.com/89783003

This CL tracks reason for invalidating style on each |Node::SetNeedsStyleRecalc|
call. This enables devtools/about:tracing to show the Blink internal reason why a
style invalidation was triggered.

This patch introduces a new |reason| argument to |Node::SetNeedsStyleRecalc|
method. The argument type |StyleChangeReasonForTracing| is actually a typedef
to |const char[]|. The string itself is instantiated on separate cpp file for minimal
binary size. This pattern is borrowed from
src/extensions/common/manifest_constants.h (Thanks jyasskin@!), and similar
pattern can be found in Blink Source/core/inspector/InspectorInstrumentation.h,

BUG=316388, 410701
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182824

[kouhei (in TOK), 2014-09-05 23:19:13]

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

[kouhei (in TOK), 2014-09-05 23:21:43]

Finally done rebasing. Does the StyleChangeReasons look right to you? or should
we track more coarcely/finer-grained?

[pdr., 2014-09-06 22:36:28]

This looks great! Some general comments and questions below.

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/ContainerNod...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/ContainerNod...
Source/core/dom/ContainerNode.cpp:1046:
setNeedsStyleRecalc(StyleRecalcDueToActivePseudoClass, SubtreeStyleChange);
We may want to call this StyleRecalcDueToActiveAndFirstLetterPseudoClasses
(here, and for focus and hover).

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:2092:
(*it)->setNeedsStyleRecalc(StyleRecalcDueToSVGFilterLayerUpdateHack,
LocalStyleChange);
We should probably just call this one StyleRecalcDueToSVGFilterLayerUpdate.

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/Node.h
File Source/core/dom/Node.h (right):

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/Node.h#newco...
Source/core/dom/Node.h:358: void setNeedsStyleRecalc(StyleChangeReason,
StyleChangeType);
Do we actually need both StyleChangeReason and StyleChangeType?

There is nearly a 1:1 mapping between the two already (only 6 exceptions and
they are easy to work around with more detailed reasons). This would just
require a few helper static functions in StyleChangeReasons such as
isNoStyleChange(StyleChangeReason), isLocalStyleChange(StyleChangeReason), etc.

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/StyleChangeR...
File Source/core/dom/StyleChangeReason.h (right):

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/StyleChangeR...
Source/core/dom/StyleChangeReason.h:12: enum StyleChangeReason {
Please add a comment here about what this is for, noting that we should not use
this for comparisons.

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/StyleChangeR...
Source/core/dom/StyleChangeReason.h:13: StyleRecalcDueToActivePseudoClass,
Ultimately we will need to send this data via a trace event. A simple way to do
this is to use strings and provide a helper static on StyleChangeReason for
getting the string representation.

Another way to do this is to just use integers and keep a ReasonInt->String list
up to date in devtools. If we go this route, we will need to use numbers here,
similar to UseCounter.h's Feature enum which prevents people from adding new
values in the middle.

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/StyleChangeR...
Source/core/dom/StyleChangeReason.h:73: #define
DISALLOW_ALL_COMPARISON_OPERATORS(TYPE)    \
Clever!

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/StyleChangeR...
Source/core/dom/StyleChangeReason.h:80:
DISALLOW_ALL_COMPARISON_OPERATORS(StyleChangeReason)
We may end up needing to make these operators private so we can use them in this
file.

[pdr., 2014-09-06 22:43:53]

What do you think about using a similar approach for setNeedsLayout()? It has
many of the same issues (MarkingBehavior is analogous to StyleChangeType) and we
could clean up the many setNeedsLayout* versions
(setNeedsLayoutAndFullPaintInvalidation,
setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation, etc).

[kouhei (in TOK), 2014-09-06 23:10:34]

Thanks for your comments! Hotel internet connection too slow for goma so I'll
address comments on Monday.

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/Node.h
File Source/core/dom/Node.h (right):

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/Node.h#newco...
Source/core/dom/Node.h:358: void setNeedsStyleRecalc(StyleChangeReason,
StyleChangeType);
On 2014/09/06 22:36:27, pdr wrote:
> Do we actually need both StyleChangeReason and StyleChangeType?
> 
> There is nearly a 1:1 mapping between the two already (only 6 exceptions and
> they are easy to work around with more detailed reasons). This would just
> require a few helper static functions in StyleChangeReasons such as
> isNoStyleChange(StyleChangeReason), isLocalStyleChange(StyleChangeReason),
etc.

Hmm... this is a hard question.
I think we should have both to avoid repeating mistakes of InvalidationReason
and DocumentLifeCycle. By having both, we keep instrumentation flags and logic
flags apart.

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/StyleChangeR...
File Source/core/dom/StyleChangeReason.h (right):

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/StyleChangeR...
Source/core/dom/StyleChangeReason.h:12: enum StyleChangeReason {
On 2014/09/06 22:36:28, pdr wrote:
> Please add a comment here about what this is for, noting that we should not
use
> this for comparisons.

Ack. I think we should rename the type too so that its obvious.
StyleChangeReasonForTracing(Only)?

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/StyleChangeR...
Source/core/dom/StyleChangeReason.h:13: StyleRecalcDueToActivePseudoClass,
On 2014/09/06 22:36:28, pdr wrote:
> Ultimately we will need to send this data via a trace event. A simple way to
do
> this is to use strings and provide a helper static on StyleChangeReason for
> getting the string representation.
> 
> Another way to do this is to just use integers and keep a ReasonInt->String
list
> up to date in devtools. If we go this route, we will need to use numbers here,
> similar to UseCounter.h's Feature enum which prevents people from adding new
> values in the middle.

Integers route seems better to me, as it would likely have less overhead and we
would not need to violate the no comparison rule.

[kouhei (in TOK), 2014-09-06 23:16:51]

On 2014/09/06 22:43:53, pdr wrote:
> What do you think about using a similar approach for setNeedsLayout()? It has
> many of the same issues (MarkingBehavior is analogous to StyleChangeType) and
we
> could clean up the many setNeedsLayout* versions
> (setNeedsLayoutAndFullPaintInvalidation,
> setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation, etc).

Eventually we should do the same for layout, but I'm not sure if now is the best
timing for it.
The code is currently actively being changed by the repaint effort team.

AFAIK those long setNeedsLayout* are intentionally long named, so that their
large overhead is obvious.
We are minimizing the usage of long named ones for fewer invalidation == less
repaint storms.

[pdr., 2014-09-06 23:22:07]

On 2014/09/06 23:10:34, kouhei wrote:
> Thanks for your comments! Hotel internet connection too slow for goma so I'll
> address comments on Monday.
> 
> https://codereview.chromium.org/547823002/diff/1/Source/core/dom/Node.h
> File Source/core/dom/Node.h (right):
> 
>
https://codereview.chromium.org/547823002/diff/1/Source/core/dom/Node.h#newco...
> Source/core/dom/Node.h:358: void setNeedsStyleRecalc(StyleChangeReason,
> StyleChangeType);
> On 2014/09/06 22:36:27, pdr wrote:
> > Do we actually need both StyleChangeReason and StyleChangeType?
> > 
> > There is nearly a 1:1 mapping between the two already (only 6 exceptions and
> > they are easy to work around with more detailed reasons). This would just
> > require a few helper static functions in StyleChangeReasons such as
> > isNoStyleChange(StyleChangeReason), isLocalStyleChange(StyleChangeReason),
> etc.
> 
> Hmm... this is a hard question.
> I think we should have both to avoid repeating mistakes of InvalidationReason
> and DocumentLifeCycle. By having both, we keep instrumentation flags and logic
> flags apart.

sgtm

> 
>
https://codereview.chromium.org/547823002/diff/1/Source/core/dom/StyleChangeR...
> File Source/core/dom/StyleChangeReason.h (right):
> 
>
https://codereview.chromium.org/547823002/diff/1/Source/core/dom/StyleChangeR...
> Source/core/dom/StyleChangeReason.h:12: enum StyleChangeReason {
> On 2014/09/06 22:36:28, pdr wrote:
> > Please add a comment here about what this is for, noting that we should not
> use
> > this for comparisons.
> 
> Ack. I think we should rename the type too so that its obvious.
> StyleChangeReasonForTracing(Only)?

Sounds good to me.

> 
>
https://codereview.chromium.org/547823002/diff/1/Source/core/dom/StyleChangeR...
> Source/core/dom/StyleChangeReason.h:13: StyleRecalcDueToActivePseudoClass,
> On 2014/09/06 22:36:28, pdr wrote:
> > Ultimately we will need to send this data via a trace event. A simple way to
> do
> > this is to use strings and provide a helper static on StyleChangeReason for
> > getting the string representation.
> > 
> > Another way to do this is to just use integers and keep a ReasonInt->String
> list
> > up to date in devtools. If we go this route, we will need to use numbers
here,
> > similar to UseCounter.h's Feature enum which prevents people from adding new
> > values in the middle.
> 
> Integers route seems better to me, as it would likely have less overhead and
we
> would not need to violate the no comparison rule.

The big downside will be maintaining a separate list which we have to keep up to
date. In my experience with similar systems (UseCounter), no amount of comments
will actually make people update a separate file (histograms.xml in UseCounter's
case). Can you think of a way to do this automatically?

I'm sort of leaning towards the string approach for simplicity.

[kouhei (in TOK), 2014-09-06 23:28:11]

>
https://codereview.chromium.org/547823002/diff/1/Source/core/dom/StyleChangeR...
> > Source/core/dom/StyleChangeReason.h:13: StyleRecalcDueToActivePseudoClass,
> > On 2014/09/06 22:36:28, pdr wrote:
> > > Ultimately we will need to send this data via a trace event. A simple way
to
> > do
> > > this is to use strings and provide a helper static on StyleChangeReason
for
> > > getting the string representation.
> > > 
> > > Another way to do this is to just use integers and keep a
ReasonInt->String
> > list
> > > up to date in devtools. If we go this route, we will need to use numbers
> here,
> > > similar to UseCounter.h's Feature enum which prevents people from adding
new
> > > values in the middle.
> > 
> > Integers route seems better to me, as it would likely have less overhead and
> we
> > would not need to violate the no comparison rule.
> 
> The big downside will be maintaining a separate list which we have to keep up
to
> date. In my experience with similar systems (UseCounter), no amount of
comments
> will actually make people update a separate file (histograms.xml in
UseCounter's
> case). Can you think of a way to do this automatically?
> 
> I'm sort of leaning towards the string approach for simplicity.

We can use a PRESUBMIT and a code generator here. In my opinion, what makes
histograms.xml hard is that UseCounter.h lives on blink repo, but histograms.xml
lives in chromium repo. Fortunately, this is not the case for inspector, which
lives in the same blink repo.

[rune, 2014-09-08 08:27:57]

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/StyleChangeR...
File Source/core/dom/StyleChangeReason.h (right):

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/StyleChangeR...
Source/core/dom/StyleChangeReason.h:64: StyleRecalcDueToZoom,
These constants are very tied up to implementation details. Are they supposed to
be useful for authors? The fact that they're tied up to implementation details.
It means the use of these constants are subject to change, and the need for many
of them will hopefully not be there in the near future. More and more of this
should be moved to happen inside the StyleInvalidator which means
StyleRecalcDueToSelector with the current code?

I think they can be quite useful for blink developers, but I'm not so sure about
authors.

There already exists inspector functionality for finding the source of e.g.
style invalidation due to class changes which gives you a javascript call stack
of the site of setting className that is more informative than getting a
StyleRecalcDueToSelector. I don't know that code, so I don't know if it's
special for class.

https://codereview.chromium.org/547823002/diff/1/Source/core/html/HTMLFormCon...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/547823002/diff/1/Source/core/html/HTMLFormCon...
Source/core/html/HTMLFormControlElement.cpp:190:
setNeedsStyleRecalc(StyleRecalcDueToRequiredOptionalPseudoClass,
SubtreeStyleChange);
This is an example of a case that should happen during style invalidation
through invalidation sets for the mentioned pseudo classes. Each of these cases
need to be analyzed to find out if there are style adjustments which are done
without involving selectors.

[rune, 2014-09-08 08:33:23]

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

[rune, 2014-09-08 08:33:24]

https://codereview.chromium.org/547823002/diff/1/Source/core/html/HTMLObjectE...
File Source/core/html/HTMLObjectElement.cpp (right):

https://codereview.chromium.org/547823002/diff/1/Source/core/html/HTMLObjectE...
Source/core/html/HTMLObjectElement.cpp:260:
setNeedsStyleRecalc(StyleRecalcDueToPlugin, SubtreeStyleChange);
This SubtreeStyleChange is a hack to trigger a re-attach from
HTMLPlugInElement::willRecalcStyle. There is no style recalc needed as such.
See: https://codereview.chromium.org/521603002/

[kouhei (in TOK), 2014-09-08 23:29:13]

Thanks for your comments! Updated CL.

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/ContainerNod...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/ContainerNod...
Source/core/dom/ContainerNode.cpp:1046:
setNeedsStyleRecalc(StyleRecalcDueToActivePseudoClass, SubtreeStyleChange);
On 2014/09/06 22:36:27, pdr wrote:
> We may want to call this StyleRecalcDueToActiveAndFirstLetterPseudoClasses
> (here, and for focus and hover).

I think the existing names are correct here. We get here only on :active pseudo
class change. IIUC, the if condition is checking if the RenderStyle also has a
:first-letter as we can't use a delayed style invalidation in that case.

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:2092:
(*it)->setNeedsStyleRecalc(StyleRecalcDueToSVGFilterLayerUpdateHack,
LocalStyleChange);
On 2014/09/06 22:36:27, pdr wrote:
> We should probably just call this one StyleRecalcDueToSVGFilterLayerUpdate.

Done.

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/StyleChangeR...
File Source/core/dom/StyleChangeReason.h (right):

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/StyleChangeR...
Source/core/dom/StyleChangeReason.h:12: enum StyleChangeReason {
On 2014/09/06 23:10:34, kouhei wrote:
> On 2014/09/06 22:36:28, pdr wrote:
> > Please add a comment here about what this is for, noting that we should not
> use
> > this for comparisons.
> 
> Ack. I think we should rename the type too so that its obvious.
> StyleChangeReasonForTracing(Only)?

Done.

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/StyleChangeR...
Source/core/dom/StyleChangeReason.h:13: StyleRecalcDueToActivePseudoClass,
On 2014/09/06 23:10:34, kouhei wrote:
> On 2014/09/06 22:36:28, pdr wrote:
> > Ultimately we will need to send this data via a trace event. A simple way to
> do
> > this is to use strings and provide a helper static on StyleChangeReason for
> > getting the string representation.
> > 
> > Another way to do this is to just use integers and keep a ReasonInt->String
> list
> > up to date in devtools. If we go this route, we will need to use numbers
here,
> > similar to UseCounter.h's Feature enum which prevents people from adding new
> > values in the middle.
> 
> Integers route seems better to me, as it would likely have less overhead and
we
> would not need to violate the no comparison rule.

Done.

https://codereview.chromium.org/547823002/diff/1/Source/core/dom/StyleChangeR...
Source/core/dom/StyleChangeReason.h:64: StyleRecalcDueToZoom,
On 2014/09/08 08:27:57, rune wrote:
> These constants are very tied up to implementation details. Are they supposed
to
> be useful for authors? The fact that they're tied up to implementation
details.
> It means the use of these constants are subject to change, and the need for
many
> of them will hopefully not be there in the near future. More and more of this
> should be moved to happen inside the StyleInvalidator which means
> StyleRecalcDueToSelector with the current code?
> I think they can be quite useful for blink developers, but I'm not so sure
about
> authors.
These are internal reasons, but we still want to have these information
available to page authors for inspecting.
This is not exposed as WebAPI but only visible via devtools/about:tracing, so we
want to show as much as we can, just like we show compositing layer info in
devtools.

Yes, StyleRecalcs from StyleInvalidator should have more info.
I'm planning to also add trace event to setNeedsStyleInvalidation from
RuleFeatureSet::scheduleStyleInvalidationForX, then connect those events from
post-processing in devtools.

> There already exists inspector functionality for finding the source of e.g.
> style invalidation due to class changes which gives you a javascript call
stack
> of the site of setting className that is more informative than getting a
> StyleRecalcDueToSelector. I don't know that code, so I don't know if it's
> special for class.
I didn't know about the feature. Would you tell me how I can try it?

[rune, 2014-09-09 08:11:27]

On 2014/09/08 23:29:13, kouhei wrote:

> > There already exists inspector functionality for finding the source of e.g.
> > style invalidation due to class changes which gives you a javascript call
> stack
> > of the site of setting className that is more informative than getting a
> > StyleRecalcDueToSelector. I don't know that code, so I don't know if it's
> > special for class.
> I didn't know about the feature. Would you tell me how I can try it?

If you make an html test-case like [1] and record its timeline in inspector
while loading, there is a "Recalculate Style" event just after the onload event
which has a "Call Site stack" pointing to the 'x.className = "x";' line.

[1] http://jsfiddle.net/dwfq0wj0/

[rune, 2014-09-09 08:16:25]

On 2014/09/09 08:11:27, rune wrote:

> If you make an html test-case like [1] and record its timeline in inspector
> while loading, there is a "Recalculate Style" event just after the onload
event
> which has a "Call Site stack" pointing to the 'x.className = "x";' line.

If there are multiple changes, it looks like it points to the first change.

[kouhei (in TOK), 2014-09-09 17:59:13]

kouhei@chromium.org changed reviewers:
+ esprehn@chromium.org

[kouhei (in TOK), 2014-09-09 17:59:40]

Updated CL. I think now its ready for broader review. Would you take a look?

On 2014/09/09 08:11:27, rune wrote:
> On 2014/09/08 23:29:13, kouhei wrote:
> 
> > > There already exists inspector functionality for finding the source of
e.g.
> > > style invalidation due to class changes which gives you a javascript call
> > stack
> > > of the site of setting className that is more informative than getting a
> > > StyleRecalcDueToSelector. I don't know that code, so I don't know if it's
> > > special for class.
> > I didn't know about the feature. Would you tell me how I can try it?
> 
> If you make an html test-case like [1] and record its timeline in inspector
> while loading, there is a "Recalculate Style" event just after the onload
event
> which has a "Call Site stack" pointing to the 'x.className = "x";' line.
> 
> [1] http://jsfiddle.net/dwfq0wj0/

Thanks for the info! We are in progress of improving the timeline to show more
information about each invalidation and how they relate to each other:
https://docs.google.com/a/google.com/document/d/1BXT3_gD--YdbIYMlTzNGsyUUMBCZ...
This CL is a part of the work, and a TRACE_EVENT using the
|StyleChangeReasonForTracing| will be added in next CL.

[pdr., 2014-09-09 23:11:24]

@rune, would you be up for commenting on our doc as well? We also have a
prototype at https://codereview.chromium.org/465223002

[pdr., 2014-09-09 23:19:06]

I think this patch looks good but I'd like for rune's OK before committing.

@esprehn, would you like to comment as well?

Offline, we discussed several approaches for how to handle the actual reason and
I think the approach in this patch is the one we should take. A future patch may
change the strings to be short english phrases but the UX hasn't been decided on
yet.

[rune, 2014-09-10 07:12:43]

On 2014/09/09 23:19:06, pdr wrote:
> I think this patch looks good but I'd like for rune's OK before committing.
> 
> @esprehn, would you like to comment as well?
> 
> Offline, we discussed several approaches for how to handle the actual reason
and
> I think the approach in this patch is the one we should take. A future patch
may
> change the strings to be short english phrases but the UX hasn't been decided
on
> yet.

rslgtm the approach. Needs to be iterated on, like tracing scheduling of
invalidation sets. More of these reasons will/should be moved to scheduling
invalidation sets instead of SubtreeStyleChange like I'm trying to do here:
https://codereview.chromium.org/540533004/

[kouhei (in TOK), 2014-09-11 18:24:44]

esprehn: Would you take a look?

On 2014/09/10 07:12:43, rune wrote:
> On 2014/09/09 23:19:06, pdr wrote:
> > I think this patch looks good but I'd like for rune's OK before committing.
> > 
> > @esprehn, would you like to comment as well?
> > 
> > Offline, we discussed several approaches for how to handle the actual reason
> and
> > I think the approach in this patch is the one we should take. A future patch
> may
> > change the strings to be short english phrases but the UX hasn't been
decided
> on
> > yet.
> 
> rslgtm the approach. Needs to be iterated on, like tracing scheduling of
> invalidation sets. More of these reasons will/should be moved to scheduling
> invalidation sets instead of SubtreeStyleChange like I'm trying to do here:
> https://codereview.chromium.org/540533004/

Thanks!

[pdr., 2014-09-11 18:32:11]

LGTM.

@esprehn, could you take a look?

[esprehn, 2014-09-11 19:24:40]

I'll review ASAP.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[kouhei (in TOK), 2014-09-17 23:34:15]

On 2014/09/11 19:24:40, esprehn wrote:
> I'll review ASAP.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

friendly ping :)

[esprehn, 2014-09-19 04:53:10]

I think we want this class to be a pair of strings, first is the type, the
second is the extra data. Then use more category like things AttributeChange,
PseudoClassChange, etc.

(AttributeChange, name)
(PseudoClassChange, name)

I also think the StyleInvalidator types are going to be really confusing to
developers since the invalidator will power more and more things and you're not
reporting what actually caused the invalidation. You might need to add a record
to the StyleInvalidator when we schedule the invalidation sets instead? We can
probably start with this, but the invalidator records are going to be really
common and not very useful ex. class change, id change will all just say
"invalidator".

https://codereview.chromium.org/547823002/diff/40001/Source/core/css/invalida...
File Source/core/css/invalidation/StyleInvalidator.cpp (right):

https://codereview.chromium.org/547823002/diff/40001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidator.cpp:148:
element.setNeedsStyleRecalc(StyleChangeReasonForTracing::StyleInvalidator,
recursionData.wholeSubtreeInvalid() ? SubtreeStyleChange : LocalStyleChange);
I'm not sure this is really adding value for developers. StyleInvalidator could
be class, id, pseudo or any number of other changes.

https://codereview.chromium.org/547823002/diff/40001/Source/core/dom/Containe...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/547823002/diff/40001/Source/core/dom/Containe...
Source/core/dom/ContainerNode.cpp:1009:
setNeedsStyleRecalc(StyleChangeReasonForTracing::FocusPseudoClass,
LocalStyleChange);
ex. Here changing :focus can either by StyleInvalidator or FocusPseudoClass.

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

https://codereview.chromium.org/547823002/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.cpp:962:
setNeedsStyleRecalc(StyleChangeReasonForTracing::Attribute, LocalStyleChange);
Shouldn't we pass along the name of the attribute too?

https://codereview.chromium.org/547823002/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.cpp:1609:
setNeedsStyleRecalc(StyleChangeReasonForTracing::Selector, SubtreeStyleChange);
which selector?

https://codereview.chromium.org/547823002/diff/40001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/547823002/diff/40001/Source/core/dom/Node.cpp...
Source/core/dom/Node.cpp:2429:
setNeedsStyleRecalc(StyleChangeReasonForTracing::CustomElement,
SubtreeStyleChange); // :unresolved has changed
UnresolvedPseudoClass ? Seems like it should be consistent with focus, hover
etc.

https://codereview.chromium.org/547823002/diff/40001/Source/core/dom/shadow/I...
File Source/core/dom/shadow/InsertionPoint.cpp (right):

https://codereview.chromium.org/547823002/diff/40001/Source/core/dom/shadow/I...
Source/core/dom/shadow/InsertionPoint.cpp:126:
m_distribution.at(i)->setNeedsStyleRecalc(StyleChangeReasonForTracing::Shadow,
SubtreeStyleChange);
This is going to confuse people, this happens inside recalcStyle we're
scheduling a recalc to propagate the Inherit change to the distributed nodes.
It's not related to shadows at all, the user already caused a huge recalc by
causing an inherit change (ex. changing color) so it's not clear what this
record will tell them, especially since it came out of the inside of recalc
style.

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:155:
setNeedsStyleRecalc(StyleChangeReasonForTracing::Control, SubtreeStyleChange);
It's weird to have a special control thing here, I think you want to make
StyleChangeReasonForTracing be a pair of strings, the first one is the category
and the second one is the extra data. This should be
StyleChangeReasonForTracing(AttributeChange, "readonly")

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:190:
setNeedsStyleRecalc(StyleChangeReasonForTracing::RequiredOptionalPseudoClass,
SubtreeStyleChange);
This is confusing, this one is PseudoClass but readonly is Control. I think you
want a general AttributeChange type and use "required" as the second arg here.

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:397:
setNeedsStyleRecalc(StyleChangeReasonForTracing::Control, SubtreeStyleChange);
Validate?

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:492:
setNeedsStyleRecalc(StyleChangeReasonForTracing::Control, SubtreeStyleChange);
ditto

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLFra...
File Source/core/html/HTMLFrameSetElement.cpp (right):

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameSetElement.cpp:77:
setNeedsStyleRecalc(StyleChangeReasonForTracing::FrameSet, SubtreeStyleChange);
(AttributeChange, "rows")

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameSetElement.cpp:82:
setNeedsStyleRecalc(StyleChangeReasonForTracing::FrameSet, SubtreeStyleChange);
(AttributeChange, "cols")

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:662:
setNeedsStyleRecalc(StyleChangeReasonForTracing::ControlValue,
SubtreeStyleChange);
ditto for all of these.

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/shadow/...
File Source/core/html/shadow/DateTimeFieldElement.cpp (right):

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/shadow/...
Source/core/html/shadow/DateTimeFieldElement.cpp:205:
setNeedsStyleRecalc(StyleChangeReasonForTracing::DisabledPseudoClass,
SubtreeStyleChange);
I think you want to generalize this too (PseudoClass, "disabled") and use that
for all the pseudos.

[kouhei (in TOK), 2014-09-22 09:03:55]

PTAL.

Changed StyleChangeReasonForTracing to track extraData string which may be const
char* or attribute name.
This involves more instructions (3 push and a mov) compared to passing just
"const char*", so we might want to optimize this more by preparing
StyleChangeReasonForTracing structs beforehand.

https://codereview.chromium.org/547823002/diff/40001/Source/core/css/invalida...
File Source/core/css/invalidation/StyleInvalidator.cpp (right):

https://codereview.chromium.org/547823002/diff/40001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidator.cpp:148:
element.setNeedsStyleRecalc(StyleChangeReasonForTracing::StyleInvalidator,
recursionData.wholeSubtreeInvalid() ? SubtreeStyleChange : LocalStyleChange);
On 2014/09/19 04:53:09, esprehn wrote:
> I'm not sure this is really adding value for developers. StyleInvalidator
could
> be class, id, pseudo or any number of other changes.

This is to be addressed in next CL. I’m adding TraceEvents in StyleInvalidator
and setNeedsStyleInvalidation too. When the devtools post processing finds this
TraceEvent, it will search for matching StyleInvalidator TraceEvent with the
class/id/pseudo/attr info.

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

https://codereview.chromium.org/547823002/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.cpp:962:
setNeedsStyleRecalc(StyleChangeReasonForTracing::Attribute, LocalStyleChange);
On 2014/09/19 04:53:09, esprehn wrote:
> Shouldn't we pass along the name of the attribute too?

Done.

https://codereview.chromium.org/547823002/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.cpp:1609:
setNeedsStyleRecalc(StyleChangeReasonForTracing::Selector, SubtreeStyleChange);
On 2014/09/19 04:53:09, esprehn wrote:
> which selector?

Done.

https://codereview.chromium.org/547823002/diff/40001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/547823002/diff/40001/Source/core/dom/Node.cpp...
Source/core/dom/Node.cpp:2429:
setNeedsStyleRecalc(StyleChangeReasonForTracing::CustomElement,
SubtreeStyleChange); // :unresolved has changed
On 2014/09/19 04:53:09, esprehn wrote:
> UnresolvedPseudoClass ? Seems like it should be consistent with focus, hover
> etc.

Done.

https://codereview.chromium.org/547823002/diff/40001/Source/core/dom/shadow/I...
File Source/core/dom/shadow/InsertionPoint.cpp (right):

https://codereview.chromium.org/547823002/diff/40001/Source/core/dom/shadow/I...
Source/core/dom/shadow/InsertionPoint.cpp:126:
m_distribution.at(i)->setNeedsStyleRecalc(StyleChangeReasonForTracing::Shadow,
SubtreeStyleChange);
On 2014/09/19 04:53:09, esprehn wrote:
> This is going to confuse people, this happens inside recalcStyle we're
> scheduling a recalc to propagate the Inherit change to the distributed nodes.
> It's not related to shadows at all, the user already caused a huge recalc by
> causing an inherit change (ex. changing color) so it's not clear what this
> record will tell them, especially since it came out of the inside of recalc
> style.

Done.

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:155:
setNeedsStyleRecalc(StyleChangeReasonForTracing::Control, SubtreeStyleChange);
On 2014/09/19 04:53:10, esprehn wrote:
> It's weird to have a special control thing here, I think you want to make
> StyleChangeReasonForTracing be a pair of strings, the first one is the
category
> and the second one is the extra data. This should be
> StyleChangeReasonForTracing(AttributeChange, "readonly")

Changed to StyleChangeReasonForTracing::ReadOnlyAttribute.

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:190:
setNeedsStyleRecalc(StyleChangeReasonForTracing::RequiredOptionalPseudoClass,
SubtreeStyleChange);
On 2014/09/19 04:53:10, esprehn wrote:
> This is confusing, this one is PseudoClass but readonly is Control. I think
you
> want a general AttributeChange type and use "required" as the second arg here.

Done.

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:397:
setNeedsStyleRecalc(StyleChangeReasonForTracing::Control, SubtreeStyleChange);
On 2014/09/19 04:53:10, esprehn wrote:
> Validate?

Done.

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:492:
setNeedsStyleRecalc(StyleChangeReasonForTracing::Control, SubtreeStyleChange);
On 2014/09/19 04:53:10, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLFra...
File Source/core/html/HTMLFrameSetElement.cpp (right):

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameSetElement.cpp:77:
setNeedsStyleRecalc(StyleChangeReasonForTracing::FrameSet, SubtreeStyleChange);
On 2014/09/19 04:53:10, esprehn wrote:
> (AttributeChange, "rows")
Changed to RowsAttribute

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameSetElement.cpp:82:
setNeedsStyleRecalc(StyleChangeReasonForTracing::FrameSet, SubtreeStyleChange);
On 2014/09/19 04:53:10, esprehn wrote:
> (AttributeChange, "cols")
ColsAttribute

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:662:
setNeedsStyleRecalc(StyleChangeReasonForTracing::ControlValue,
SubtreeStyleChange);
On 2014/09/19 04:53:10, esprehn wrote:
> ditto for all of these.

Done.

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/shadow/...
File Source/core/html/shadow/DateTimeFieldElement.cpp (right):

https://codereview.chromium.org/547823002/diff/40001/Source/core/html/shadow/...
Source/core/html/shadow/DateTimeFieldElement.cpp:205:
setNeedsStyleRecalc(StyleChangeReasonForTracing::DisabledPseudoClass,
SubtreeStyleChange);
On 2014/09/19 04:53:10, esprehn wrote:
> I think you want to generalize this too (PseudoClass, "disabled") and use that
> for all the pseudos.

Done.

[pdr., 2014-09-24 17:21:34]

Esprehn, ping? The invalidation tracking project is now blocked on this review.

[esprehn, 2014-09-26 03:53:02]

https://codereview.chromium.org/547823002/diff/60001/Source/core/dom/StyleCha...
File Source/core/dom/StyleChangeReason.cpp (right):

https://codereview.chromium.org/547823002/diff/60001/Source/core/dom/StyleCha...
Source/core/dom/StyleChangeReason.cpp:63: return String(m_reason);
These have hidden mallocs. Maybe that's okay.

https://codereview.chromium.org/547823002/diff/60001/Source/core/dom/StyleCha...
File Source/core/dom/StyleChangeReason.h (right):

https://codereview.chromium.org/547823002/diff/60001/Source/core/dom/StyleCha...
Source/core/dom/StyleChangeReason.h:93: static StyleChangeReasonForTracing
fromAttribute(const QualifiedName& attributeName)
qualified name is just namespace and a string, I'd store just the name
(developers don't understand namespaces anyway, and very few pages depend on
them). So you don't need m_attributeName and m_extraData, you only need
m_extraData.

https://codereview.chromium.org/547823002/diff/60001/Source/core/dom/StyleCha...
Source/core/dom/StyleChangeReason.h:103: String getExtraDataString() const;
no get* on getters.

https://codereview.chromium.org/547823002/diff/60001/Source/core/dom/StyleCha...
Source/core/dom/StyleChangeReason.h:114: const QualifiedName* m_attributeName;
remove this. Also QualifiedName is not safe to store in raw ptr form like this,
if the element got removed and gc'ed you'd keep alive the attr qualified name
and have a use after free.

https://codereview.chromium.org/547823002/diff/60001/Source/core/dom/TreeScop...
File Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/547823002/diff/60001/Source/core/dom/TreeScop...
Source/core/dom/TreeScope.cpp:545:
element->setNeedsStyleRecalc(StyleChangeReasonForTracing::create(StyleChangeReason::ViewportUnits),
LocalStyleChange);
Can we make this the second argument instead? Now when I'm reading the
setNeedsStyleRecalc callsites the important behavioral code is all the way at
the far end of the screen.

[kouhei (in TOK), 2014-09-29 02:04:28]

Patchset #5 (id:80001) has been deleted

[kouhei (in TOK), 2014-09-29 02:05:32]

Sorry for late reply. PTAL.

https://codereview.chromium.org/547823002/diff/60001/Source/core/dom/StyleCha...
File Source/core/dom/StyleChangeReason.cpp (right):

https://codereview.chromium.org/547823002/diff/60001/Source/core/dom/StyleCha...
Source/core/dom/StyleChangeReason.cpp:63: return String(m_reason);
On 2014/09/26 03:53:01, esprehn wrote:
> These have hidden mallocs. Maybe that's okay.

This getter is only called when invalidation tracing is enabled.

https://codereview.chromium.org/547823002/diff/60001/Source/core/dom/StyleCha...
File Source/core/dom/StyleChangeReason.h (right):

https://codereview.chromium.org/547823002/diff/60001/Source/core/dom/StyleCha...
Source/core/dom/StyleChangeReason.h:93: static StyleChangeReasonForTracing
fromAttribute(const QualifiedName& attributeName)
On 2014/09/26 03:53:01, esprehn wrote:
> qualified name is just namespace and a string, I'd store just the name
> (developers don't understand namespaces anyway, and very few pages depend on
> them). So you don't need m_attributeName and m_extraData, you only need
> m_extraData.

Done.

https://codereview.chromium.org/547823002/diff/60001/Source/core/dom/StyleCha...
Source/core/dom/StyleChangeReason.h:103: String getExtraDataString() const;
On 2014/09/26 03:53:01, esprehn wrote:
> no get* on getters.

Done.

https://codereview.chromium.org/547823002/diff/60001/Source/core/dom/StyleCha...
Source/core/dom/StyleChangeReason.h:114: const QualifiedName* m_attributeName;
On 2014/09/26 03:53:01, esprehn wrote:
> remove this. Also QualifiedName is not safe to store in raw ptr form like
this,
> if the element got removed and gc'ed you'd keep alive the attr qualified name
> and have a use after free.

Done.

https://codereview.chromium.org/547823002/diff/60001/Source/core/dom/TreeScop...
File Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/547823002/diff/60001/Source/core/dom/TreeScop...
Source/core/dom/TreeScope.cpp:545:
element->setNeedsStyleRecalc(StyleChangeReasonForTracing::create(StyleChangeReason::ViewportUnits),
LocalStyleChange);
On 2014/09/26 03:53:01, esprehn wrote:
> Can we make this the second argument instead? Now when I'm reading the
> setNeedsStyleRecalc callsites the important behavioral code is all the way at
> the far end of the screen.

Done.

[esprehn, 2014-09-29 02:30:23]

lgtm

https://codereview.chromium.org/547823002/diff/120001/Source/core/dom/StyleCh...
File Source/core/dom/StyleChangeReason.cpp (right):

https://codereview.chromium.org/547823002/diff/120001/Source/core/dom/StyleCh...
Source/core/dom/StyleChangeReason.cpp:60: new (NotNull, (void*)&Active)
String(":active");
I think there's some kind of special static string thing you can use here.

[kouhei (in TOK), 2014-09-29 03:24:45]

https://codereview.chromium.org/547823002/diff/120001/Source/core/dom/StyleCh...
File Source/core/dom/StyleChangeReason.cpp (right):

https://codereview.chromium.org/547823002/diff/120001/Source/core/dom/StyleCh...
Source/core/dom/StyleChangeReason.cpp:60: new (NotNull, (void*)&Active)
String(":active");
On 2014/09/29 02:30:23, esprehn wrote:
> I think there's some kind of special static string thing you can use here.

Changed to AtomicString(ConstructFromLiteral)

[kouhei (in TOK), 2014-09-29 03:24:57]

Thanks for review!

[kouhei (in TOK), 2014-09-29 03:25:06]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-09-29 03:26:19]

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

[commit-bot: I haz the power, 2014-09-29 09:45:12]

Committed patchset #7 (id:140001) as 182824