Bug 71412 - Move LayoutTypes.h from rendering/ to platform/
Summary: Move LayoutTypes.h from rendering/ to platform/
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-02 15:45 PDT by Emil A Eklund
Modified: 2011-11-02 16:41 PDT (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 2011-11-02 15:45:01 PDT
Source/WebCore/rendering/LayoutTypes.h is used in several places outside of the rendering directory, including in the platform one. As it provides an abstraction for types defined in platform it seems like the file should be moved there.
Comment 1 Darin Adler 2011-11-02 15:46:49 PDT
Could you be a little more specific about where in the platform directory the layout types need to be used?
Comment 2 Darin Adler 2011-11-02 15:47:17 PDT
I don’t like the idea of the platform layer needing to know about the concept of a layout unit, but maybe I’m missing something.
Comment 3 Emil A Eklund 2011-11-02 15:58:18 PDT
The platform layer in itself doesn't really need to know about Layout Units or LayoutRect/Point/Size. However the platform directory also contains base classes, such as ScrollableArea, that define a couple of pure virtual methods that are implemented in rendering.

To move those methods over to the layout unit abstraction we'd either have to introduce a dependency on a header file in the rendering directory or keep the definitions as IntRect/Point/Size.

As we already have a couple of files in the platform directory using the layout types this seemed like the best option but I'm open to suggestions.
Comment 4 Darin Adler 2011-11-02 16:03:38 PDT
(In reply to comment #3)
> the platform directory also contains base classes, such as ScrollableArea

What else besides ScrollableArea?

> To move those methods over to the layout unit abstraction we'd either have to introduce a dependency on a header file in the rendering directory or keep the definitions as IntRect/Point/Size.

I think it would not be helpful to have ScrollableArea use LayoutUnit if the callers using ScrollableArea are using LayoutUnit. If the callers are using LayoutUnit, then they must not be in the platform layer.

So far it sounds like these should not be moved. But maybe there’s more to the story.
Comment 5 Emil A Eklund 2011-11-02 16:11:50 PDT
(In reply to comment #4)
> What else besides ScrollableArea?

ScrollView and Scrollbar.


> I think it would not be helpful to have ScrollableArea use LayoutUnit if the callers using ScrollableArea are using LayoutUnit. If the callers are using LayoutUnit, then they must not be in the platform layer.

I'm not sure I'm following. Are you suggesting that we should keep it as ints for now and switch it over directly to fixed as a part of the big patch that switches from ints to fixed?

Or are you suggesting that we keep the base classes defined in platform/ using ints but have the subclasses in rendering use layout types?
Comment 6 Darin Adler 2011-11-02 16:22:55 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > What else besides ScrollableArea?
> 
> ScrollView and Scrollbar.
> 
> > I think it would not be helpful to have ScrollableArea use LayoutUnit if the callers using ScrollableArea are using LayoutUnit. If the callers are using LayoutUnit, then they must not be in the platform layer.
> 
> I'm not sure I'm following. Are you suggesting that we should keep it as ints for now and switch it over directly to fixed as a part of the big patch that switches from ints to fixed?

No. I think platform should not be built on top of the fractional system we invent for layout.

> Or are you suggesting that we keep the base classes defined in platform/ using ints but have the subclasses in rendering use layout types?

We can have scrolling machinery in platform that works without knowing that the thing its scrolling is using some different type for its internal layout. I don’t see why the scrolling machinery itself needs to deal with fractional pixels.
Comment 7 Emil A Eklund 2011-11-02 16:41:49 PDT
(In reply to comment #6)
> No. I think platform should not be built on top of the fractional system we invent for layout.
> We can have scrolling machinery in platform that works without knowing that the thing its scrolling is using some different type for its internal layout. I don’t see why the scrolling machinery itself needs to deal with fractional pixels.

Right, we currently (on our branch) have the fractional pixels passed down into the first layer of the platform code in a few places where it is then converted to the relevant type for the platform. It sounds like you'd like this to be done entirely outside of platform.

That seems perfectly reasonable and is in many ways a better solution.