Bug 152786 – Factor free standing tree style resolve functions into a class

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Antti Koivisto]

Add a class for resolving style for a document tree. This will enable future features and optimizations.

[Antti Koivisto]

Created attachment 268367 [details]
patch

[Build Bot]

Comment on attachment 268367 [details]
patch

Attachment 268367 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/657930

New failing tests:
printing/compositing-layer-printing.html
printing/setPrinting.html
printing/simple-lines-break.html
printing/width-overflow.html
printing/page-rule-in-media-query.html

[Build Bot]

Created attachment 268368 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268367 [details]
patch

Attachment 268367 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/657932

New failing tests:
printing/page-rule-in-media-query.html
fast/css/scrollbar-dynamic-style-change.html
printing/simple-lines-break.html
printing/width-overflow.html
printing/setPrinting.html
printing/compositing-layer-printing.html

[Build Bot]

Created attachment 268369 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268367 [details]
patch

Attachment 268367 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/657920

New failing tests:
printing/compositing-layer-printing.html
printing/page-rule-in-media-query.html
printing/simple-lines-break.html
printing/width-overflow.html
printing/setPrinting.html

[Build Bot]

Created attachment 268370 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Simon Fraser (smfr)]

Comment on attachment 268367 [details]
patch

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

> Source/WebCore/style/StyleResolveTree.h:50
> +class TreeResolver {

It's odd that the filename and class name don't match. It think you should rename one of them.

> Source/WebCore/style/StyleResolveTree.h:59
> +    void resolveShadowTree(Change, RenderStyle& inheritedStyle);

Unclear from the signature whether inheritedStyle is 'in' or 'out'.

> Source/WebCore/style/StyleResolveTree.h:61
> +    Ref<RenderStyle> styleForElement(Element&, RenderStyle& inheritedStyle);

Ditto. Can inheritedStyle be const? Can Element& be const? Same for most of the other functions.

[Antti Koivisto]

> It's odd that the filename and class name don't match. It think you should
> rename one of them.

I'm going to do renames separately.

> Ditto. Can inheritedStyle be const? Can Element& be const? Same for most of
> the other functions.

They should probably be const but that will require lots of changes (without introducing const_cast). It's better done separately.

[Antti Koivisto]

Created attachment 268392 [details]
patch

[Dave Hyatt]

Weird that the file name is StyleResolveTree, but the class name is TreeResolver. These should match.

I also don't think a Style namespace adds anything here. Just calling the class StyleTreeResolver is fine IMO.

[Simon Fraser (smfr)]

(In reply to comment #9)
> > It's odd that the filename and class name don't match. It think you should
> > rename one of them.
> 
> I'm going to do renames separately.
> 
> > Ditto. Can inheritedStyle be const? Can Element& be const? Same for most of
> > the other functions.
> 
> They should probably be const but that will require lots of changes (without
> introducing const_cast). It's better done separately.

These seem like two good changes to make before doing this one.

[Antti Koivisto]

> These seem like two good changes to make before doing this one.

Does the order really matter? There are many many things to improve here. I'd like to make progress.

[Sam Weinig]

Comment on attachment 268392 [details]
patch

I think either order is fine.

[Antti Koivisto]

Created attachment 268401 [details]
now including file renames due to popular demand

[WebKit Commit Bot]

Attachment 268401 [details] did not pass style-queue:


ERROR: Source/WebCore/style/StyleTreeResolver.cpp:56:  "CSSFontSelector.h" already included at Source/WebCore/style/StyleTreeResolver.cpp:32  [build/include] [4]
ERROR: Source/WebCore/style/StyleTreeResolver.cpp:221:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/style/StyleTreeResolver.cpp:858:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/style/StyleTreeResolver.cpp:860:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/style/StyleTreeResolver.cpp:864:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/style/StyleTreeResolver.cpp:878:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/style/StyleTreeResolver.h:26:  #ifndef header guard has wrong style, please use: StyleTreeResolver_h  [build/header_guard] [5]
ERROR: Source/WebCore/style/StyleTreeResolver.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 8 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

https://trac.webkit.org/r194691