Bug 62032 - Convert RenderBox::overflowClipRect to IntPoint
Summary: Convert RenderBox::overflowClipRect to IntPoint
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: 60318
  Show dependency treegraph
 
Reported: 2011-06-03 11:46 PDT by Emil A Eklund
Modified: 2011-06-04 02:53 PDT (History)
4 users (show)

See Also:


Attachments
Patch (16.58 KB, patch)
2011-06-03 12:36 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (16.62 KB, patch)
2011-06-03 13:17 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (14.52 KB, patch)
2011-06-03 15:05 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch for landing (14.51 KB, patch)
2011-06-03 16:08 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-03 11:46:38 PDT
Ongoing tx, ty to IntPoint conversion
Comment 1 Emil A Eklund 2011-06-03 12:36:02 PDT
Created attachment 95948 [details]
Patch
Comment 2 Emil A Eklund 2011-06-03 13:17:47 PDT
Created attachment 95954 [details]
Patch

Fixed typo in ChangeLog
Comment 3 Eric Seidel (no email) 2011-06-03 13:42:15 PDT
Comment on attachment 95954 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:1145
> +    IntRect clipRect(location + borderTopLeft(), size() - borderSize());

borderSize() = IntSize(borderRight(), borderBottom)?

> Source/WebCore/rendering/RenderBoxModelObject.h:95
> +    virtual IntSize borderSize() const { return IntSize(borderLeft() + borderRight(), borderTop() + borderBottom()); }

Hmmm... Seems we should get this from the borderRect(), I'm not sure borderSize is meaningful.

> Source/WebCore/rendering/RenderLayer.h:258
> +    IntSize scrollbarSize(OverlayScrollbarSizeRelevancy = IgnoreOverlayScrollbarSize) const;

This doesn't make any sense.  The vertical and horizontal scrollbars are indepenent.  How can we put them both into a single size?
Comment 4 Emil A Eklund 2011-06-03 15:05:40 PDT
Created attachment 95970 [details]
Patch
Comment 5 Emil A Eklund 2011-06-03 15:06:41 PDT
Thanks Eric, I agree that the scrollbarSize method was a bit weird.
Please take another look and see if you like this approach.
Comment 6 Eric Seidel (no email) 2011-06-03 15:39:26 PDT
Comment on attachment 95970 [details]
Patch

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

LGTM.

> Source/WebCore/rendering/RenderBox.cpp:1148
> +        clipRect.contract(layer()->verticalScrollbarWidth(relevancy),
> +            layer()->horizontalScrollbarHeight(relevancy));

I wouldn't have bothered wrapping. 1. cause we have no column limit in webkit.  and 2. because as soon as you make it 2 lines, we technically would require { } around the if block (even though the executed code is one line).
Comment 7 Emil A Eklund 2011-06-03 16:08:08 PDT
Created attachment 95981 [details]
Patch for landing
Comment 8 WebKit Review Bot 2011-06-04 02:53:49 PDT
Comment on attachment 95981 [details]
Patch for landing

Clearing flags on attachment: 95981

Committed r88102: <http://trac.webkit.org/changeset/88102>
Comment 9 WebKit Review Bot 2011-06-04 02:53:54 PDT
All reviewed patches have been landed.  Closing bug.