Summary: | Create intermediate classes as a path towards getting off of pixel offsets | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Levi Weintraub <leviw> | ||||||||||
Component: | Layout and Rendering | Assignee: | Levi Weintraub <leviw> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | darin, eae, eric, hyatt, mitz, simon.fraser, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 63273 | ||||||||||||
Bug Blocks: | 60318 | ||||||||||||
Attachments: |
|
Description
Levi Weintraub
2011-06-01 17:32:07 PDT
We’ll need four types, right? The scalar, the point, the size, and the rect types. (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... The words for the scalar would be “coordinate” and “distance” if you thought in terms of point and size. How does "LayoutPoint" and "LayoutSize" sound? Internally they would use "Coordinate" and "Distance" respectively. I’m not sure we really need distinct types for layout coordinates and layout distances. 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... Maybe “scalar”? What about "LayoutUnit"? (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. (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. 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 :) 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? Created attachment 98277 [details]
Patch
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? Comment on attachment 98277 [details] Patch Attachment 98277 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8918905 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? 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?
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. (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. (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. (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. 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). (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. Created attachment 98429 [details]
Patch
Comment on attachment 98429 [details]
Patch
Whoops, missed one of my changes.
Comment on attachment 98429 [details]
Patch
Scratch that... just being neurotic :p
Created attachment 98435 [details]
Patch
(In reply to comment #27) > Created an attachment (id=98435) [details] > Patch Updated to patch to ToT Anyone willing to review? 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. Created attachment 98946 [details]
Patch for landing
Comment on attachment 98946 [details] Patch for landing Clearing flags on attachment: 98946 Committed r89945: <http://trac.webkit.org/changeset/89945> All reviewed patches have been landed. Closing bug. Thanks again for the review, Darin. |