Bug 63762 - Switch rendering tree selection code to to new layout types
Summary: Switch rendering tree selection code 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-06-30 15:51 PDT by Emil A Eklund
Modified: 2011-07-07 15:38 PDT (History)
4 users (show)

See Also:


Attachments
Patch (39.31 KB, patch)
2011-06-30 16:09 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (35.89 KB, patch)
2011-07-06 09:40 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (38.56 KB, patch)
2011-07-07 14:45 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-06-30 15:51:28 PDT
Convert selection handling code to new layout abstraction.
Comment 1 Emil A Eklund 2011-06-30 16:09:33 PDT
Created attachment 99389 [details]
Patch
Comment 2 Emil A Eklund 2011-06-30 16:11:27 PDT
No rounding methods where harmed in making this patch.
Comment 3 Eric Seidel (no email) 2011-06-30 16:43:23 PDT
Comment on attachment 99389 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:118
> +    LayoutUnit sPos = max(startPos - m_start, static_cast<LayoutUnit>(0));

I don't think you'll need this cast in the float case.  Will you?
Comment 4 Emil A Eklund 2011-06-30 16:46:18 PDT
(In reply to comment #3)
> (From update of attachment 99389 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99389&action=review
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:118
> > +    LayoutUnit sPos = max(startPos - m_start, static_cast<LayoutUnit>(0));
> 
> I don't think you'll need this cast in the float case.  Will you?

Sadly you do. The max method either takes int, int or float, float. In the float case without the cast this would result in a compiler error.
Comment 5 Levi Weintraub 2011-07-01 12:06:27 PDT
This was one of the most frustrating things to deal with when building our proof of concept. Since we max with zero all over the code, I'm thinking it could make sense to have an inline function clapToZero which would remove our need to cast zero.
Comment 6 Eric Seidel (no email) 2011-07-01 12:11:44 PDT
I thought the compiler was smart enough to treat '0' like whatever type it needed to.  I guess not.

I strongly support the addition of some sort of maxWithZero or whatever function and deploying it wherever you see fit.
Comment 7 Darin Adler 2011-07-01 16:28:25 PDT
Comment on attachment 99389 [details]
Patch

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

>>> Source/WebCore/rendering/InlineTextBox.cpp:118
>>> +    LayoutUnit sPos = max(startPos - m_start, static_cast<LayoutUnit>(0));
>> 
>> I don't think you'll need this cast in the float case.  Will you?
> 
> Sadly you do. The max method either takes int, int or float, float. In the float case without the cast this would result in a compiler error.

The good way to write this is:

    max<LayoutUnit>(startPos - m_start, 0);
Comment 8 Darin Adler 2011-07-01 16:29:16 PDT
(In reply to comment #5)
> I'm thinking it could make sense to have an inline function clapToZero which would remove our need to cast zero.

That would be fine. I would probably not name it maxWithZero or clapToZero or even clampToZero, though.
Comment 9 Emil A Eklund 2011-07-06 09:40:07 PDT
Created attachment 99843 [details]
Patch
Comment 10 Emil A Eklund 2011-07-06 09:44:37 PDT
Changed max/min to use the template form rather than relying on casts. Thanks for the tip!
Comment 11 Emil A Eklund 2011-07-07 14:45:39 PDT
Created attachment 100035 [details]
Patch
Comment 12 Eric Seidel (no email) 2011-07-07 14:56:08 PDT
Comment on attachment 100035 [details]
Patch

LGTM.
Comment 13 WebKit Review Bot 2011-07-07 15:38:10 PDT
Comment on attachment 100035 [details]
Patch

Clearing flags on attachment: 100035

Committed r90596: <http://trac.webkit.org/changeset/90596>
Comment 14 WebKit Review Bot 2011-07-07 15:38:15 PDT
All reviewed patches have been landed.  Closing bug.