Bug 60578 - Switch paintMask and paintMaskImages off of ints
Summary: Switch paintMask and paintMaskImages off of ints
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: Levi Weintraub
URL:
Keywords:
Depends on: 60490 60679
Blocks: 60408
  Show dependency treegraph
 
Reported: 2011-05-10 13:59 PDT by Levi Weintraub
Modified: 2011-05-17 06:07 PDT (History)
6 users (show)

See Also:


Attachments
Patch (17.67 KB, patch)
2011-05-12 11:02 PDT, Levi Weintraub
eric: review+
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-10 13:59:58 PDT
Part of ongoing refactoring.
Comment 1 Levi Weintraub 2011-05-11 16:47:02 PDT
Updating name.
Comment 2 Levi Weintraub 2011-05-12 11:02:41 PDT
Created attachment 93306 [details]
Patch
Comment 3 Eric Seidel (no email) 2011-05-12 11:30:48 PDT
Comment on attachment 93306 [details]
Patch

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

> Source/WebCore/platform/graphics/IntRect.h:113
> +    void expand(const IntSize& s) { m_size += s; }

I would have used "size" for the argument name

> Source/WebCore/rendering/RenderBox.cpp:883
> +void RenderBox::paintMask(PaintInfo& paintInfo, IntSize paintOffset)

What's the offset from?  Would be nice to explain that in the name.
Comment 4 Levi Weintraub 2011-05-12 11:38:28 PDT
Comment on attachment 93306 [details]
Patch

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

>> Source/WebCore/platform/graphics/IntRect.h:113
>> +    void expand(const IntSize& s) { m_size += s; }
> 
> I would have used "size" for the argument name

Sure.

>> Source/WebCore/rendering/RenderBox.cpp:883
>> +void RenderBox::paintMask(PaintInfo& paintInfo, IntSize paintOffset)
> 
> What's the offset from?  Would be nice to explain that in the name.

I'm unsure of how to name this in a way that satisfies everyone. I'd go with something like offsetFromPaintingRoot but I'm worried it may not be accurate for all cases. Would love smfr's advice (he suggested an IntSize named paintOffset in IRC IIRC).
Comment 5 Levi Weintraub 2011-05-12 13:51:01 PDT
(In reply to comment #3)
> (From update of attachment 93306 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93306&action=review
> 
> > Source/WebCore/platform/graphics/IntRect.h:113
> > +    void expand(const IntSize& s) { m_size += s; }
> 
> I would have used "size" for the argument name
> 
> > Source/WebCore/rendering/RenderBox.cpp:883
> > +void RenderBox::paintMask(PaintInfo& paintInfo, IntSize paintOffset)
> 
> What's the offset from?  Would be nice to explain that in the name.

Simon confirmed he agrees that paintOffset is the best we can do given how this currently behaves. If I switch the s to size can I get an r+?
Comment 6 Eric Seidel (no email) 2011-05-12 13:53:36 PDT
Comment on attachment 93306 [details]
Patch

Sure.
Comment 7 Levi Weintraub 2011-05-12 14:05:20 PDT
Committed r86377: <http://trac.webkit.org/changeset/86377>
Comment 8 Alexis Menard (darktears) 2011-05-17 06:07:25 PDT
(In reply to comment #7)
> Committed r86377: <http://trac.webkit.org/changeset/86377>

Committed r86670: <http://trac.webkit.org/changeset/86670> fixes a warning.