Bug 71249 - Switch RenderObject to layout abstraction
Summary: Switch RenderObject to layout abstraction
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-10-31 15:54 PDT by Emil A Eklund
Modified: 2011-11-01 16:02 PDT (History)
2 users (show)

See Also:


Attachments
Patch (18.35 KB, patch)
2011-10-31 15:56 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (17.46 KB, patch)
2011-10-31 16:47 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (16.66 KB, patch)
2011-10-31 17:32 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (16.23 KB, patch)
2011-11-01 10:38 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-10-31 15:54:59 PDT
Switch RenderObject to layout type abstraction.
Comment 1 Emil A Eklund 2011-10-31 15:56:40 PDT
Created attachment 113097 [details]
Patch
Comment 2 Darin Adler 2011-10-31 16:06:51 PDT
Comment on attachment 113097 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:794
> +    int width = roundf((side == BSTop || side == BSBottom ? y2 - y1 : x2 - x1));

The round function specifically takes a “float” so is not appropriate to round a LayoutUnit to an int. If LayoutUnit was a double or int it would be quite inefficient!

> Source/WebCore/rendering/RenderObject.h:1052
> -inline int adjustForAbsoluteZoom(int value, RenderObject* renderer)
> +inline float adjustForAbsoluteZoom(int value, RenderObject* renderer)

I think this change should be landed separately with its own review and rationale.
Comment 3 Emil A Eklund 2011-10-31 16:47:53 PDT
Created attachment 113107 [details]
Patch
Comment 4 Emil A Eklund 2011-10-31 16:49:12 PDT
(In reply to comment #2)
> The round function specifically takes a “float” so is not appropriate to round a LayoutUnit to an int. If LayoutUnit was a double or int it would be quite inefficient!

Good catch, I should have caught that. Thanks!

> 
> > Source/WebCore/rendering/RenderObject.h:1052
> > -inline int adjustForAbsoluteZoom(int value, RenderObject* renderer)
> > +inline float adjustForAbsoluteZoom(int value, RenderObject* renderer)
> 
> I think this change should be landed separately with its own review and rationale.

Reverted this change, leaving it for later.

Please take another look.
Comment 5 Darin Adler 2011-10-31 16:51:14 PDT
Comment on attachment 113107 [details]
Patch

Patch looks good, but does not apply in the EWS.
Comment 6 Emil A Eklund 2011-10-31 17:32:53 PDT
Created attachment 113115 [details]
Patch
Comment 7 Daniel Bates 2011-10-31 20:28:13 PDT
Comment on attachment 113115 [details]
Patch

Attachment 113115 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/10241949
Comment 8 Emil A Eklund 2011-11-01 10:38:41 PDT
Created attachment 113191 [details]
Patch
Comment 9 Emil A Eklund 2011-11-01 10:41:55 PDT
Thanks Darin!
Comment 10 WebKit Review Bot 2011-11-01 16:02:37 PDT
Comment on attachment 113191 [details]
Patch

Clearing flags on attachment: 113191

Committed r99008: <http://trac.webkit.org/changeset/99008>
Comment 11 WebKit Review Bot 2011-11-01 16:02:43 PDT
All reviewed patches have been landed.  Closing bug.