Bug 80918 – Update LayoutUnit usage in descendants of RenderReplaced

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

[Levi Weintraub]

Replaced elements have to flow in the new sub-pixel Render Tree, but since the rendering of these often takes place outside of WebCore (or in cases such as foreign objects, in WebCore after passing through platform code), care must be taken to determine the final rendered size and location before render time. This patch brings these classes up to the latest and greatest in the subpixellayout branch.

[Levi Weintraub]

Created attachment 131490 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 131490 [details]
Patch

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

> Source/WebCore/rendering/RenderVideo.cpp:169
> +    IntRect contentRect = pixelSnappedIntRect(contentBoxRect());

Sigh.  I dislike all these free functions. :(

> Source/WebCore/rendering/RenderWidget.cpp:147
> +static inline IntRect roundedIntRect(const LayoutRect& rect)
> +{
> +    return IntRect(roundedIntPoint(rect.location()), roundedIntSize(rect.size()));
> +}

why can't this be a method on LayoutRect (and IntRect for hte time being?)

> Source/WebCore/rendering/RenderWidget.cpp:165
> +    m_widget->setFrameRect(IntRect(roundedIntRect(frame)));

IntRect() isn't needed here, is it?  Also again, why not a method on the rect instead of a free function?

> Source/WebCore/rendering/RenderWidget.cpp:181
> +    LayoutRect absoluteContentBox = LayoutRect(localToAbsoluteQuad(FloatQuad(contentBox)).boundingBox());

The constructor on the right side isnt' neded.  If it is, you can just remove the = () and construct directly in place.

[Levi Weintraub]

Comment on attachment 131490 [details]
Patch

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

>> Source/WebCore/rendering/RenderVideo.cpp:169
>> +    IntRect contentRect = pixelSnappedIntRect(contentBoxRect());
> 
> Sigh.  I dislike all these free functions. :(

So you'd rather have a LayoutRect::pixelSnap() function? It'd definitely be shorter to type :) Since this would be a pretty large refactor at this point, I'd propose it for a separate patch if you feel strongly.

>> Source/WebCore/rendering/RenderWidget.cpp:147
>> +}
> 
> why can't this be a method on LayoutRect (and IntRect for hte time being?)

I put it here to discourage usage since this is seldom what you want. I'm happy to move it.

>> Source/WebCore/rendering/RenderWidget.cpp:165
>> +    m_widget->setFrameRect(IntRect(roundedIntRect(frame)));
> 
> IntRect() isn't needed here, is it?  Also again, why not a method on the rect instead of a free function?

The IntRect definitely isn't needed. Good catch.

[Eric Seidel (no email)]

Comment on attachment 131490 [details]
Patch

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

>>> Source/WebCore/rendering/RenderWidget.cpp:147
>>> +}
>> 
>> why can't this be a method on LayoutRect (and IntRect for hte time being?)
> 
> I put it here to discourage usage since this is seldom what you want. I'm happy to move it.

Ah, OK, that part wasn't immediately clear to me.  Might want to comment to that effect. :)

[Levi Weintraub]

Created attachment 132788 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 132788 [details]
Patch

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

ok.

> Source/WebCore/rendering/RenderImage.cpp:530
> +            containerSize.setWidth(roundToInt(containerSize.width() * style()->effectiveZoom()));
> +            containerSize.setHeight(roundToInt(containerSize.height() * style()->effectiveZoom()));

Don't we have a method on size to do thos multiply already?

[Emil A Eklund]

(In reply to comment #6)
> (From update of attachment 132788 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132788&action=review
> 
> ok.
> 
> > Source/WebCore/rendering/RenderImage.cpp:530
> > +            containerSize.setWidth(roundToInt(containerSize.width() * style()->effectiveZoom()));
> > +            containerSize.setHeight(roundToInt(containerSize.height() * style()->effectiveZoom()));
> 
> Don't we have a method on size to do thos multiply already?

We have a scale method (on IntSize and FractionalLayoutSize) but they floor rather than round.

If flooring is acceptable here containerSize.scale(style()->effectiveZoom()) should do the trick.

[Levi Weintraub]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 132788 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=132788&action=review
> > 
> > ok.
> > 
> > > Source/WebCore/rendering/RenderImage.cpp:530
> > > +            containerSize.setWidth(roundToInt(containerSize.width() * style()->effectiveZoom()));
> > > +            containerSize.setHeight(roundToInt(containerSize.height() * style()->effectiveZoom()));
> > 
> > Don't we have a method on size to do thos multiply already?
> 
> We have a scale method (on IntSize and FractionalLayoutSize) but they floor rather than round.
> 
> If flooring is acceptable here containerSize.scale(style()->effectiveZoom()) should do the trick.

Perhaps FractionalLayoutSize::scale shouldn't floor? A thought...

[Levi Weintraub]

Comment on attachment 132788 [details]
Patch

Clearing flags on attachment: 132788

Committed r111515: <http://trac.webkit.org/changeset/111515>

[Levi Weintraub]

All reviewed patches have been landed.  Closing bug.