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
Created attachment 92842 [details] Patch
I'm not necessarily totally sold on the members being tx/ty. Maybe just x and y?
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.
(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
Renaming
Created attachment 92857 [details] Patch
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.
Created attachment 92884 [details] Patch
Created attachment 92889 [details] Patch
(In reply to comment #9) > Created an attachment (id=92889) [details] > Patch Reuploading so it passes the EWS bots
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.
(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 :)
Created attachment 93001 [details] Patch for landing
Created attachment 93008 [details] Patch for landing
(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 on attachment 93001 [details] Patch for landing Attachment 93001 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8687077
Comment on attachment 93008 [details] Patch for landing Clearing flags on attachment: 93008 Committed r86195: <http://trac.webkit.org/changeset/86195>
All reviewed patches have been landed. Closing bug.
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 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.
Marking this Invalid per our IRC discussion.