[webkit-reviews] review denied: [Bug 122815] Optimize RenderBlock::lowestFloatLogicalBottom by caching previously calculated values : [Attachment 226311] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Laszlo Vidacs
<lvidacs.u-szeged at partner.samsung.com>'s request for review:
Bug 122815: Optimize RenderBlock::lowestFloatLogicalBottom by caching
previously calculated values
https://bugs.webkit.org/show_bug.cgi?id=122815

Attachment 226311: Patch
https://bugs.webkit.org/attachment.cgi?id=226311&action=review

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=226311&action=review


This looks like a good change, but I'd like some confirmation that we really
want to switch from using an '&' test to the equality. Also, the naming scheme
you've switched to is not acceptable.

> Source/WebCore/rendering/FloatingObjects.cpp:302
> +	   for (FloatingObjectSet::const_iterator it =
floatingObjectSet.begin(); it != end; ++it) {

This would read better as an 'auto'. The original code this is replacing was
using auto previously.

> Source/WebCore/rendering/FloatingObjects.cpp:303
> +	       const FloatingObject* r = it->get();

'floatingObject' is a much better name than 'r', and is what was used in the
code you are replacing.

> Source/WebCore/rendering/FloatingObjects.cpp:317
> +	   for (FloatingObjectSet::const_iterator it =
floatingObjectSet.begin(); it != end; ++it) {

Ditto above (auto).

> Source/WebCore/rendering/FloatingObjects.cpp:318
> +	       const FloatingObject* r = it->get();

Ditto above ('floatingObject' name).

> Source/WebCore/rendering/FloatingObjects.cpp:319
> +	       if (r->isPlaced() && r->type() == floatType)

We used to use an '& floatType' test here. Why do we want equality now? Do we
no longer need to handle the case where floatingObject->type() is a superset of
types that encompasses 'floatType'?