Better factoring.
Created attachment 270431 [details] patch
Created attachment 270432 [details] patch
Created attachment 270435 [details] patch
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 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"?
> 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.
> 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.
(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.
https://trac.webkit.org/r196031