Bug 153768 - Factor style sharing code out of StyleResolver
Summary: Factor style sharing code out of StyleResolver
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-01 14:46 PST by Antti Koivisto
Modified: 2016-02-02 14:41 PST (History)
3 users (show)

See Also:


Attachments
patch (60.33 KB, patch)
2016-02-01 15:04 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (60.36 KB, patch)
2016-02-01 15:09 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (61.90 KB, patch)
2016-02-01 15:23 PST, Antti Koivisto
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2016-02-01 14:46:04 PST
Better factoring.
Comment 1 Antti Koivisto 2016-02-01 15:04:11 PST
Created attachment 270431 [details]
patch
Comment 2 Antti Koivisto 2016-02-01 15:09:17 PST
Created attachment 270432 [details]
patch
Comment 3 Antti Koivisto 2016-02-01 15:23:32 PST
Created attachment 270435 [details]
patch
Comment 4 Darin Adler 2016-02-01 18:18:26 PST
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.
Comment 5 Ryosuke Niwa 2016-02-01 18:33:18 PST
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"?
Comment 6 Antti Koivisto 2016-02-01 19:31:52 PST
> 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.
Comment 7 Antti Koivisto 2016-02-01 19:44:11 PST
> 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.
Comment 8 Ryosuke Niwa 2016-02-02 07:28:48 PST
(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.
Comment 9 Antti Koivisto 2016-02-02 14:41:22 PST
https://trac.webkit.org/r196031