[webkit-reviews] review granted: [Bug 61896] Create intermediate classes as a path towards getting off of pixel offsets : [Attachment 98435] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Levi Weintraub <leviw at chromium.org>'s
request for review:
Bug 61896: Create intermediate classes as a path towards getting off of pixel
offsets
https://bugs.webkit.org/show_bug.cgi?id=61896

Attachment 98435: Patch
https://bugs.webkit.org/attachment.cgi?id=98435&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.