Bug 120084 – Web Inspector: provide style invalidation reasons with Layout timeline records

• Component: Web Inspector
• Status: NEW

[Simon Fraser (smfr)]

Allow the inspector to show the reason for style invalidation

[Simon Fraser (smfr)]

Created attachment 209226 [details]
Patch (not for review but feedback welcome).

[Simon Fraser (smfr)]

Created attachment 209227 [details]
Screenshot

[Darin Adler]

Comment on attachment 209226 [details]
Patch (not for review but feedback welcome).

View in context: https://bugs.webkit.org/attachment.cgi?id=209226&action=review

> Source/WebCore/inspector/InspectorInstrumentation.h:177
> +    enum RecalcStyleReason {

Putting these inside the InspectorInstrumentation namespace makes use of them so wordy. Really wish they weren’t in the namespace.

> Source/WebCore/inspector/InspectorInstrumentation.h:196
> +    class RecalcStyleReasonSetter {

Would be nice to make this explicitly non-copyable.

> Source/WebCore/inspector/InspectorInstrumentation.h:551
> +    static RecalcStyleReason s_recalcStyleReason;

Is the cost for all this extra global variable access affordable?

[Radar WebKit Bug Importer]

<rdar://problem/14802834>

[Antti Koivisto]

Comment on attachment 209226 [details]
Patch (not for review but feedback welcome).

View in context: https://bugs.webkit.org/attachment.cgi?id=209226&action=review

> Source/WebCore/inspector/InspectorInstrumentation.h:191
> +    enum RecalcStyleReason {
> +        NoReason,
> +        CSSAnimation,
> +        Fullscreen,
> +        ActiveState,
> +        HoverState,
> +        FocusState,
> +        AttributeChange,
> +        ClassAttributeChange,
> +        InlineStyleChange,
> +        SelectorSensitiveStateChange,
> +        DOMTreeChange,
> +        FormControlChange,
> +        DragStateChange,
> +    };

It might be useful to make the reason tracking part of the style resolve code rather than being in Inspector land. We might have internal uses for this information. Specifically we might want to decide what sort of resolve to do at resolve time instead at invalidation time like we do now.

This is not really a show stopper for this patch (we can switch over later) but something to consider.

>> Source/WebCore/inspector/InspectorInstrumentation.h:196
>> +    class RecalcStyleReasonSetter {
> 
> Would be nice to make this explicitly non-copyable.

Global variable based hackery like this is pretty distasteful. It is probably ok for Inspector interface but at least we shouldn't propagate these elsewhere.

[Simon Fraser (smfr)]

> It might be useful to make the reason tracking part of the style resolve code rather than being in Inspector land. We might have internal uses for this information. Specifically we might want to decide what sort of resolve to do at resolve time instead at invalidation time like we do now.
> 
> This is not really a show stopper for this patch (we can switch over later) but something to consider.

Interesting, though in some cases it would require plumbing a "reason" param through several levels of calls.

> >> Source/WebCore/inspector/InspectorInstrumentation.h:196
> >> +    class RecalcStyleReasonSetter {
> > 
> > Would be nice to make this explicitly non-copyable.
> 
> Global variable based hackery like this is pretty distasteful. It is probably ok for Inspector interface but at least we shouldn't propagate these elsewhere.

InspectorInstrumentation consults a global to decide whether to take a fast return, so this isn't really any different, but I agree that global state is icky.

[Timothy Hatcher]

Sorry I didn't see this until now. I like where this is going. With the Timeline work I'm doing this would be great to get landed. Layout reasons would be good too.

[Blaze Burg]

<rdar://problem/25493105>

[Blaze Burg]

*** Bug 107667 has been marked as a duplicate of this bug. ***