Bug 65396 - Switch RenderBlock to to new layout types
Summary: Switch RenderBlock to to new layout types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Emil A Eklund
URL:
Keywords:
Depends on:
Blocks: 63567
  Show dependency treegraph
 
Reported: 2011-07-29 15:01 PDT by Emil A Eklund
Modified: 2011-08-09 14:53 PDT (History)
5 users (show)

See Also:


Attachments
Patch (38.92 KB, patch)
2011-07-29 15:11 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (37.19 KB, patch)
2011-08-01 13:12 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 2011-07-29 15:01:06 PDT
Convert RenderBlock to new layout abstraction.
Comment 1 Emil A Eklund 2011-07-29 15:11:21 PDT
Created attachment 102403 [details]
Patch
Comment 2 Darin Adler 2011-07-30 11:01:52 PDT
Comment on attachment 102403 [details]
Patch

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

No EWS because the patch doesn’t apply.

I suggest putting in a patch that only adds the new typedef and is entirely mechanical, and leaving the part that introduces new functions as a separate far smaller patch.

> Source/WebCore/platform/graphics/IntRect.cpp:142
> +IntRect enclosingIntRect(const IntRect& rect)
> +{
> +    return IntRect(rect);
> +}

This must be inlined in the header. We do not want to pay a function call to convert an IntRect to an IntRect.

> Source/WebCore/rendering/LayoutTypes.h:89
> +inline bool layoutUnitEquals(LayoutUnit a, LayoutUnit b)

If the idea here is to use a definition that involves epsilon, then I think you need a different name. After all, == does not use epsilon, so the name "equal" is already taken.
Comment 3 Emil A Eklund 2011-08-01 13:12:13 PDT
Created attachment 102546 [details]
Patch
Comment 4 Emil A Eklund 2011-08-01 13:12:37 PDT
Thanks Darin, made the changes you suggested. Please take another look.
Comment 5 Eric Seidel (no email) 2011-08-08 09:28:01 PDT
Comment on attachment 102546 [details]
Patch

LGTM.  You may want to let Darin take another look too.
Comment 6 Emil A Eklund 2011-08-08 13:38:53 PDT
Thanks Eric,

I'll hold of landing this for another day or so to give Darin a chance to look at it.
Comment 7 WebKit Review Bot 2011-08-09 14:52:54 PDT
Comment on attachment 102546 [details]
Patch

Clearing flags on attachment: 102546

Committed r92712: <http://trac.webkit.org/changeset/92712>
Comment 8 WebKit Review Bot 2011-08-09 14:53:03 PDT
All reviewed patches have been landed.  Closing bug.