Bug 60490 – Create LayerOffset class

• Status: RESOLVED
• Resolution: INVALID
• Component: Layout and Rendering

[Levi Weintraub]

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

[Levi Weintraub]

Created attachment 92842 [details]
Patch

[Levi Weintraub]

I'm not necessarily totally sold on the members being tx/ty. Maybe just x and y?

[Eric Seidel (no email)]

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.

[Levi Weintraub]

(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

[Levi Weintraub]

Renaming

[Levi Weintraub]

Created attachment 92857 [details]
Patch

[Eric Seidel (no email)]

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.

[Levi Weintraub]

Created attachment 92884 [details]
Patch

[Levi Weintraub]

Created attachment 92889 [details]
Patch

[Levi Weintraub]

(In reply to comment #9)
> Created an attachment (id=92889) [details]
> Patch

Reuploading so it passes the EWS bots

[Eric Seidel (no email)]

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.

[Levi Weintraub]

(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 :)

[Levi Weintraub]

Created attachment 93001 [details]
Patch for landing

[Levi Weintraub]

Created attachment 93008 [details]
Patch for landing

[Levi Weintraub]

(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)

[WebKit Review Bot]

Comment on attachment 93001 [details]
Patch for landing

Attachment 93001 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8687077

[WebKit Commit Bot]

Comment on attachment 93008 [details]
Patch for landing

Clearing flags on attachment: 93008

Committed r86195: <http://trac.webkit.org/changeset/86195>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 93008 [details]:

http/tests/xmlhttprequest/logout.html bug 52047 (author: [e-mail])
The commit-queue is continuing to process your patch.

[Simon Fraser (smfr)]

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.

[Levi Weintraub]

Marking this Invalid per our IRC discussion.