Bug 60490 - Create LayerOffset class
Summary: Create LayerOffset class
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Levi Weintraub
URL:
Keywords:
Depends on: 60586 60597
Blocks: 60318 60408 60578
  Show dependency treegraph
 
Reported: 2011-05-09 11:49 PDT by Levi Weintraub
Modified: 2011-05-11 12:09 PDT (History)
7 users (show)

See Also:


Attachments
Patch (9.61 KB, patch)
2011-05-09 13:53 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (11.09 KB, patch)
2011-05-09 14:56 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (15.98 KB, patch)
2011-05-09 16:47 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (16.08 KB, patch)
2011-05-09 17:12 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch for landing (25.71 KB, patch)
2011-05-10 13:32 PDT, Levi Weintraub
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (27.61 KB, patch)
2011-05-10 14:35 PDT, Levi Weintraub
simon.fraser: review-
simon.fraser: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 2011-05-09 11:49:04 PDT
The tx/ty offsets passed throughout painting code is neither fully a point nor a size. I propose the creation of a new class called PaintOffset that allows us to stop passing raw ints, and better represents this use case instead of abusing IntSize or IntPoint
Comment 1 Levi Weintraub 2011-05-09 13:53:17 PDT
Created attachment 92842 [details]
Patch
Comment 2 Levi Weintraub 2011-05-09 13:54:22 PDT
I'm not necessarily totally sold on the members being tx/ty. Maybe just x and y?
Comment 3 Eric Seidel (no email) 2011-05-09 13:56:22 PDT
Comment on attachment 92842 [details]
Patch

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

A few comments
1.  I suspect you missed at least one of the 8 build systems. :)
2.  It might be confusing that this is used in more than just painting.  Maybe OffsetFromContainingBlock?  I'm not sure what this offset actually is anyway.

> Source/WebCore/platform/graphics/PaintOffset.h:51
> +    void expand(const IntSize& size)

Is "expand" really what we're doing to this ofset here?

> Source/WebCore/platform/graphics/PaintOffset.h:61
> +    int m_tx, m_ty;

I think we normally put each declaration on its own line.
Comment 4 Levi Weintraub 2011-05-09 14:29:39 PDT
(In reply to comment #3)
> (From update of attachment 92842 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92842&action=review
> 
> A few comments
> 1.  I suspect you missed at least one of the 8 build systems. :)
I believe I updated the build systems that keep track of headers, but I could totally be wrong... I'll take another look.
> 2.  It might be confusing that this is used in more than just painting.  Maybe OffsetFromContainingBlock?  I'm not sure what this offset actually is anyway.
I believe tx/ty is passed in as the offset of the parent in the containing RenderLayer's coordinates. Perhaps LayerOffset?
> 
> > Source/WebCore/platform/graphics/PaintOffset.h:51
> > +    void expand(const IntSize& size)
> 
> Is "expand" really what we're doing to this ofset here?
I think rather than go with confusing names here I may remove this function altogether and rely on overloaded operators.
> 
> > Source/WebCore/platform/graphics/PaintOffset.h:61
> > +    int m_tx, m_ty;
> 
> I think we normally put each declaration on its own line.

I (probably naively) followed IntPoint here :p
Comment 5 Levi Weintraub 2011-05-09 14:38:44 PDT
Renaming
Comment 6 Levi Weintraub 2011-05-09 14:56:37 PDT
Created attachment 92857 [details]
Patch
Comment 7 Eric Seidel (no email) 2011-05-09 15:05:41 PDT
Comment on attachment 92857 [details]
Patch

it would be nice to see you deploy this in one place as part of this patch.  The new class looks fine though.  You might want to add a comment in the .h explaining what it's for.
Comment 8 Levi Weintraub 2011-05-09 16:47:09 PDT
Created attachment 92884 [details]
Patch
Comment 9 Levi Weintraub 2011-05-09 17:12:44 PDT
Created attachment 92889 [details]
Patch
Comment 10 Levi Weintraub 2011-05-09 17:14:11 PDT
(In reply to comment #9)
> Created an attachment (id=92889) [details]
> Patch

Reuploading so it passes the EWS bots
Comment 11 Eric Seidel (no email) 2011-05-09 17:29:18 PDT
Comment on attachment 92889 [details]
Patch

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

I think this is good.  I think we can land this as-is.  But I think you should be sure to wave this in front of maciej's nose at some point (that can be after landing), just to see what his thoughts are on introducing this point/size in-between.

> Source/WebCore/platform/graphics/LayerOffset.h:34
> +// LayerOffset represents the off

you trailed off...

> Source/WebCore/platform/graphics/LayerOffset.h:42
> +    LayerOffset(const IntSize& size) : m_x(size.width()), m_y(size.height()) { }
> +    LayerOffset(const IntPoint& point) : m_x(point.x()), m_y(point.y()) { }

I wonder if it's a good idea to have implicit constructors here.

> Source/WebCore/rendering/RenderScrollbar.cpp:278
> +    partRenderer->paintIntoRect(graphicsContext, pos(), rect);

seems pos() needs to be renamed in some later patch. :)

> Source/WebCore/rendering/RenderScrollbarPart.cpp:165
> +    setLocation(rect.location() - layerOffset.toSize());

I wonder if there is a moveLocation or something that takes a size.
Comment 12 Levi Weintraub 2011-05-10 11:44:16 PDT
(In reply to comment #11)
> (From update of attachment 92889 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92889&action=review
> 
> I think this is good.  I think we can land this as-is.  But I think you should be sure to wave this in front of maciej's nose at some point (that can be after landing), just to see what his thoughts are on introducing this point/size in-between.

I'd also love some feedback from mjs. Any chanced you can take a look Maciej?

Otherwise I'll land this later this afternoon so I can start uploading patches that make use of this class!

> > Source/WebCore/platform/graphics/LayerOffset.h:34
> > +// LayerOffset represents the off
> 
> you trailed off...

Fixed this locally

> > Source/WebCore/platform/graphics/LayerOffset.h:42
> > +    LayerOffset(const IntSize& size) : m_x(size.width()), m_y(size.height()) { }
> > +    LayerOffset(const IntPoint& point) : m_x(point.x()), m_y(point.y()) { }
> 
> I wonder if it's a good idea to have implicit constructors here.
> 
> > Source/WebCore/rendering/RenderScrollbar.cpp:278
> > +    partRenderer->paintIntoRect(graphicsContext, pos(), rect);
> 
> seems pos() needs to be renamed in some later patch. :)

It should really be location
IntSize size() const { return frameRect().size(); }
IntPoint pos() const { return frameRect().location(); }

> > Source/WebCore/rendering/RenderScrollbarPart.cpp:165
> > +    setLocation(rect.location() - layerOffset.toSize());
> 
> I wonder if there is a moveLocation or something that takes a size.

There could be :)
Comment 13 Levi Weintraub 2011-05-10 13:32:32 PDT
Created attachment 93001 [details]
Patch for landing
Comment 14 Levi Weintraub 2011-05-10 14:35:26 PDT
Created attachment 93008 [details]
Patch for landing
Comment 15 Levi Weintraub 2011-05-10 14:36:56 PDT
(In reply to comment #14)
> Created an attachment (id=93008) [details]
> Patch for landing

Fixed a problem in the xcodeproj (wasn't putting LayerOffset.h into the private headers)
Comment 16 WebKit Review Bot 2011-05-10 15:13:08 PDT
Comment on attachment 93001 [details]
Patch for landing

Attachment 93001 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8687077
Comment 17 WebKit Commit Bot 2011-05-10 16:45:15 PDT
Comment on attachment 93008 [details]
Patch for landing

Clearing flags on attachment: 93008

Committed r86195: <http://trac.webkit.org/changeset/86195>
Comment 18 WebKit Commit Bot 2011-05-10 16:45:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 WebKit Commit Bot 2011-05-10 16:48:57 PDT
The commit-queue encountered the following flaky tests while processing attachment 93008 [details]:

http/tests/xmlhttprequest/logout.html bug 52047 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 20 Simon Fraser (smfr) 2011-05-10 16:50:45 PDT
Comment on attachment 93008 [details]
Patch for landing

I veto this patch. LayerOffset makes no sense; there is no class called Layer. tx and ty are not RenderLayer-relative.

I think you should pick IntSize or IntPoint and deal with it.
Comment 21 Levi Weintraub 2011-05-11 11:17:54 PDT
Marking this Invalid per our IRC discussion.