Summary: | Update LayoutUnit usage in descendants of RenderReplaced | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Levi Weintraub <leviw> | ||||||
Component: | Layout and Rendering | Assignee: | Levi Weintraub <leviw> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin, eae, eric, jchaffraix, simon.fraser | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 60318 | ||||||||
Attachments: |
|
Description
Levi Weintraub
2012-03-12 18:05:36 PDT
Created attachment 131490 [details]
Patch
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. 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. 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. :) Created attachment 132788 [details]
Patch
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? (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. (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... Comment on attachment 132788 [details] Patch Clearing flags on attachment: 132788 Committed r111515: <http://trac.webkit.org/changeset/111515> All reviewed patches have been landed. Closing bug. |