Bug 61896 – Create intermediate classes as a path towards getting off of pixel offsets

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Levi Weintraub]

In an effort to solve a number of issues that crop up when zooming the DOM, we want to move off of integer layout values that correspond directly to pixels. Ideally, we want to test multiple replacements such as floats and twips. We've already begun moving these values off of the int integral type and onto classes such as IntPoint, IntSize, and IntRect. Ideally, we'd use a typedeffed intermediate instead so we could quickly iterate over the different options; smfr threw out "RenderPoint."

I'm creating this bug to start the discussion. The sooner we reach a conclusion, the sooner we can begin using this class in our transition off of integral types.

Hyatt, your feedback is particularly welcome on all parts of this plan :)

[Darin Adler]

We’ll need four types, right? The scalar, the point, the size, and the rect types.

[Levi Weintraub]

(In reply to comment #1)
> We’ll need four types, right? The scalar, the point, the size, and the rect types.

That's exactly right. We're wrestling with what to call the scalar...

[Darin Adler]

The words for the scalar would be “coordinate” and “distance” if you thought in terms of point and size.

[Levi Weintraub]

How does "LayoutPoint" and "LayoutSize" sound? Internally they would use "Coordinate" and "Distance" respectively.

[Darin Adler]

I’m not sure we really need distinct types for layout coordinates and layout distances.

[Levi Weintraub]

Good. That simplifies things g(In reply to comment #5)
> I’m not sure we really need distinct types for layout coordinates and layout distances.

I certainly find it preferable to not have distinct types for them since it seems like it would lead to confusion when they're interchanged, and I don't believe we'd ever want to use different types for them. They still need a name though, and coordinate and distance both imply a narrower usage than what is proposed...

[Darin Adler]

Maybe “scalar”?

[Levi Weintraub]

What about "LayoutUnit"?

[Simon Fraser (smfr)]

(In reply to comment #8)
> What about "LayoutUnit"?

It sucks trying to pick a name before we've decided what it will be. If we do, we'll end up with some generic-sounding thing that we may decide to rename again later.

[Levi Weintraub]

(In reply to comment #9)
> (In reply to comment #8)
> > What about "LayoutUnit"?
> 
> It sucks trying to pick a name before we've decided what it will be. If we do, we'll end up with some generic-sounding thing that we may decide to rename again later.

That's a fair critique, but I'll volunteer to rename it to something less detached from the implementation if that really becomes a point of contention.

Personally, I kind of like the abstraction. It certainly makes the specific usage of these integral types more explicit.

[Levi Weintraub]

We're nearly done transitioning off of tx/ty, which means this naming is going to be our next hurtle. I think there's value in trying out the different options for layout units (floats, fixed point, etc.) and an intermediary class seems to be the best way to enable that. If you can suggest a different course to avoid this I'm all ears, otherwise I'd love your opinion in what to call this thing :)

[Simon Fraser (smfr)]

I wonder if it makes sense to do the more disruptive experimentation on a branch, and then touch trunk when you know which approach to take?

[Levi Weintraub]

Created attachment 98277 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 98277 [details]
Patch

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

> Source/JavaScriptCore/wtf/Platform.h:1013
> +#define WTF_USE_FLOAT_OFFSETS 0

I think this needs to be a bit more specific. WTF_LAYOUT_USES_FLOAT_OFFSETS ?

> Source/WebCore/rendering/LayoutTypes.h:53
> +typedef int LayoutNumber;

Not a huge fan of "LayoutNumber". LayoutValue?

[WebKit Review Bot]

Comment on attachment 98277 [details]
Patch

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

[Dimitri Glazkov (Google)]

Comment on attachment 98277 [details]
Patch

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

>> Source/WebCore/rendering/LayoutTypes.h:53
>> +typedef int LayoutNumber;
> 
> Not a huge fan of "LayoutNumber". LayoutValue?

LayoutUnit?

[Eric Seidel (no email)]

Comment on attachment 98277 [details]
Patch

Does this actually move us closer?  Isn't there going to need to be an insane amount of rounding code added as well?  Can that rounding code exist in parallel in both paths?  It's likely it can, but it would be useful to see what other (rounding) abstracitons you're going to add with this?

[Simon Fraser (smfr)]

I also think this is starting to be invasive in a way that will be annoying to the rest of us trying to hack layout code on TOT. I foresee an extended set of pages to move from IntRect -> LayoutRect, before it's even known whether floats can be used at all.

I'd still suggest prototyping on a branch to the point where you've chosen a method to move forward with.

BTW, you'll also have to ensure that IntRect and FloatRect have matching methods with this patch; currently they do not.

[Levi Weintraub]

(In reply to comment #18)
> I also think this is starting to be invasive in a way that will be annoying to the rest of us trying to hack layout code on TOT. I foresee an extended set of pages to move from IntRect -> LayoutRect, before it's even known whether floats can be used at all.
> 
> I'd still suggest prototyping on a branch to the point where you've chosen a method to move forward with.

We made a working prototype which passes most of our zooming tests and proves float's are viable. You can see it attached to the meta bug 60318.

> BTW, you'll also have to ensure that IntRect and FloatRect have matching methods with this patch; currently they do not.

[Levi Weintraub]

(In reply to comment #17)
> (From update of attachment 98277 [details])
> Does this actually move us closer?  Isn't there going to need to be an insane amount of rounding code added as well?  Can that rounding code exist in parallel in both paths?  It's likely it can, but it would be useful to see what other (rounding) abstracitons you're going to add with this?

Our plan is to implement this rounding functionality behind the USE(FLOAT_OFFSETS) flag -- or whatever name we settle on. We'll do this as non-intrusively as possible and in parallel.

[Levi Weintraub]

(In reply to comment #16)
> (From update of attachment 98277 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98277&action=review
> 
> >> Source/WebCore/rendering/LayoutTypes.h:53
> >> +typedef int LayoutNumber;
> > 
> > Not a huge fan of "LayoutNumber". LayoutValue?
> 
> LayoutUnit?

It seems to me that LayoutValue tells us little about what the variables we're representing really are. Number or Unit seem more informative, but I'm not wedded to any name in particular.

[Eric Seidel (no email)]

Although I've seen the impressive demo's of Levi's functional float-based rendering tree, others have not.

I think it would help your case levi/emil were you to announce your full plan either in a bug or on webkit-dev, describing how this transition will work.

They've done the "branch" work (in their own local git branch).  One way to show that to others might be to check it into an SVN branch, but I'm not sure that's actually that helpful.

I'm not sure this is the best first step for integration, but I'm also not sure which is.

Levi and Emil explored both a float-based rendering tree, and a int-with-accumulated-rounding-hacks approach.  The float approach ended up more complete with about an equal sized patch, is my understanding.  They did this parallel exploration for about a week, after which declaring the float approach the winner.  Last I saw their build, the web rendered fine, the test cases attached to bug 60318 rendered fine, but there were still some rounding corner-cases to work through.

It seems the next step forward is to work on the plan of how to educate the community of their findings, as well as find a smooth path for transitioning TOT to their work (which appears to be what Levi is doing here).

[Levi Weintraub]

(In reply to comment #14)
> (From update of attachment 98277 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98277&action=review
> 
> > Source/JavaScriptCore/wtf/Platform.h:1013
> > +#define WTF_USE_FLOAT_OFFSETS 0
> 
> I think this needs to be a bit more specific. WTF_LAYOUT_USES_FLOAT_OFFSETS ?
> 
> > Source/WebCore/rendering/LayoutTypes.h:53
> > +typedef int LayoutNumber;
> 
> Not a huge fan of "LayoutNumber". LayoutValue?

What about LayoutUnit and WTF_USE_FLOAT_LAYOUT_OFFSETS? WTF_USE_ is the necessary prefix for the USE() macro.

[Levi Weintraub]

Created attachment 98429 [details]
Patch

[Levi Weintraub]

Comment on attachment 98429 [details]
Patch

Whoops, missed one of my changes.

[Levi Weintraub]

Comment on attachment 98429 [details]
Patch

Scratch that... just being neurotic :p

[Levi Weintraub]

Created attachment 98435 [details]
Patch

[Levi Weintraub]

(In reply to comment #27)
> Created an attachment (id=98435) [details]
> Patch

Updated to patch to ToT

[Levi Weintraub]

Anyone willing to review?

[Darin Adler]

Comment on attachment 98435 [details]
Patch

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

I think this is a low-risk way to make the further float experiments practical. Even if there is some disaster, it's easy to back this out with a global replace since the new typedefs are unique words that do not appear elsewhere in the code.

> Source/JavaScriptCore/wtf/Platform.h:1014
> +#if !defined(WTF_USE_FLOAT_LAYOUT_OFFSETS)
> +#define WTF_USE_FLOAT_LAYOUT_OFFSETS 0
> +#endif

I don’t think the first patch should add this switch. Lets just land the typedefs without any #if at first.

Please do not touch Platform.h at all. I do not think this will ever need to have something in Platform.h.

> Source/WebCore/rendering/LayoutState.h:89
> -    IntRect m_clipRect;
> -    IntSize m_paintOffset; // x/y offset from container.  Includes relative positioning and scroll offsets.
> -    IntSize m_layoutOffset; // x/y offset from container.  Does not include relative positioning or scroll offsets.
> -    IntSize m_layoutDelta; // Transient offset from the final position of the object
> +    LayoutRect m_clipRect;
> +    LayoutSize m_paintOffset; // x/y offset from container. Includes relative positioning and scroll offsets.
> +    LayoutSize m_layoutOffset; // x/y offset from container. Does not include relative positioning or scroll offsets.
> +    LayoutSize m_layoutDelta; // Transient offset from the final position of the object
>                             // used to ensure that repaints happen in the correct place.
>                             // This is a total delta accumulated from the root.

If you're changing this, you need to get rid of the "lined up" comment lines below. A rename like this shows why we don't line things up normally in WebKit. Just indent the other lines by 4 or figure out some other commenting style.

> Source/WebCore/rendering/LayoutTypes.h:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

This is 2011.

> Source/WebCore/rendering/LayoutTypes.h:43
> +#if USE(FLOAT_LAYOUT_OFFSETS)
> +#include "FloatRect.h"
> +#else
> +#include "IntRect.h"
> +#endif

Lets just land the all int version of this for now. No need for the "if float" to be checked in. I definitely don’t think we need a Platform.h switch for this. We do *not* want this different on different platforms.

[Levi Weintraub]

Created attachment 98946 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 98946 [details]
Patch for landing

Clearing flags on attachment: 98946

Committed r89945: <http://trac.webkit.org/changeset/89945>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Levi Weintraub]

Thanks again for the review, Darin.