Bug 153768 – Factor style sharing code out of StyleResolver

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Antti Koivisto]

Better factoring.

[Antti Koivisto]

Created attachment 270431 [details]
patch

[Antti Koivisto]

Created attachment 270432 [details]
patch

[Antti Koivisto]

Created attachment 270435 [details]
patch

[Darin Adler]

Comment on attachment 270435 [details]
patch

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

> Source/WebCore/style/StyleSharingResolver.cpp:65
> +static inline bool elementHasDirectionAuto(const Element& element)

Could take StyledElement& here, not that it would make things any faster.

> Source/WebCore/style/StyleTreeResolver.cpp:100
> +static RenderStyle* placeholderStyle;

Why is it OK if this is global, given that ensurePlaceholderStyle takes a document?

Also, extra blank line before this.

> Source/WebCore/style/StyleTreeResolver.cpp:976
> +bool isPlaceholderStyle(const RenderStyle& style)

Extra blank line before this.

[Ryosuke Niwa]

Comment on attachment 270435 [details]
patch

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

> Source/WebCore/style/StyleSharingResolver.cpp:50
> +    EInsideLink elementLinkState;

Should we lazily compute this value to avoid looking up the hash table proactively?

> Source/WebCore/style/StyleSharingResolver.cpp:77
> +    if (!element.parentElement())
> +        return nullptr;

You no longer check the existence of parent style.  Is that expected?

> Source/WebCore/style/StyleSharingResolver.cpp:362
> +    for (unsigned i = 0; i < classNames.size(); ++i) {

Should we store classNames.size() in a local variable instead?

> Source/WebCore/style/StyleSharingResolver.h:46
> +    const Element* searchSimilar(const Element&) const;

I got confused by this name because it sounded as if we need to do further verification to make sure we can share a style with it.
How about "findStyleSharableElement" or "findElementToShareStyle"?

[Antti Koivisto]

> Why is it OK if this is global, given that ensurePlaceholderStyle takes a
> document?

This object is not really used, it just needs to be minimally initialized. This is what the old code did but it is clearly ugly. There is probably a way to avoid the font initialization.

[Antti Koivisto]

> Should we lazily compute this value to avoid looking up the hash table
> proactively?

Probably though I'm not sure if it makes much difference in practice.

> > Source/WebCore/style/StyleSharingResolver.cpp:77
> > +    if (!element.parentElement())
> > +        return nullptr;
> 
> You no longer check the existence of parent style.  Is that expected?

I'll put the check back. I'm not sure if it ever matters but can't really prove it.

> > Source/WebCore/style/StyleSharingResolver.cpp:362
> > +    for (unsigned i = 0; i < classNames.size(); ++i) {
> 
> Should we store classNames.size() in a local variable instead?

Or rather use range-for.

> I got confused by this name because it sounded as if we need to do further
> verification to make sure we can share a style with it.
> How about "findStyleSharableElement" or "findElementToShareStyle"?

The class name is Style::SharingResolver so I feel "style" and "share" are redundant in the function name. I sort of like "similar" since it communicates why we can share style with it.

[Ryosuke Niwa]

(In reply to comment #7)
>
> > I got confused by this name because it sounded as if we need to do further
> > verification to make sure we can share a style with it.
> > How about "findStyleSharableElement" or "findElementToShareStyle"?
> 
> The class name is Style::SharingResolver so I feel "style" and "share" are
> redundant in the function name. I sort of like "similar" since it
> communicates why we can share style with it.

How about `findShared` or simply `resolve`?  I find the adjective `similar` to be more confusing because it sounds as if two elements share something but not identical in style.

[Antti Koivisto]

https://trac.webkit.org/r196031