Bug 92154 – Add horizontal writing mode logic to FractionalLayoutRect and update RenderBox, RenderBlock

• Component: Layout and Rendering
• Status: RESOLVED INVALID

[Emil A Eklund]

Add logicalLeft/Right/Top/Bottom/Width/Height and setLogicalLeft/Right/Width/Height methods to FractionalLayoutRect and change RenderBox and RenderBlock to use them. This reduces the number of places where we have to do the mapping between logical and actual values.

[Emil A Eklund]

Created attachment 154133 [details]
Patch

[Emil A Eklund]

I'm not entirely convinced this is worth doing. The code size reduction isn't quite as big as I had hoped but it is likely that more classes can be moved over to use the horizontal writing mode logic in the rect class.

What do you think?

[Emil A Eklund]

Created attachment 154140 [details]
Patch

[WebKit Review Bot]

Comment on attachment 154140 [details]
Patch

Attachment 154140 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13346040

New failing tests:
fast/loader/loadInProgress.html
fast/inline/positionedLifetime.html
fast/loader/unload-form-post-about-blank.html

[WebKit Review Bot]

Created attachment 154187 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Julien Chaffraix]

Comment on attachment 154140 [details]
Patch

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

To make an educated decision here, we would need to know the potential for code sharing. The patch as-is doesn't increase the sharing much and could negatively impact the speed. IMHO it does hurt readability so we would need a strong win on the sharing side to offset the rest.

> Source/WebCore/platform/graphics/FractionalLayoutRect.cpp:50
> +FractionalLayoutUnit FractionalLayoutRect::logicalLeft(const RenderStyle* style) const

Not really a fan of passing around our style() everywhere :-/

On top of this being annoying, it's an extra function call (which is not inlined where most of the callers are) and an extra argument to pass so it's likely a performance loss. Considering how much those functions are called, the performance impact should be evaluated before proceeding.

[Simon Fraser (smfr)]

> > Source/WebCore/platform/graphics/FractionalLayoutRect.cpp:50
> > +FractionalLayoutUnit FractionalLayoutRect::logicalLeft(const RenderStyle* style) const
> 
> Not really a fan of passing around our style() everywhere :-/

platform/graphics/FractionalLayoutRect.cpp should know nothing about style. That's a layering violation.

[Emil A Eklund]

Thanks for your input, marking this as invalid.