[webkit-reviews] review granted: [Bug 52380] Lazily generate and store logical ordering of InlineBoxes : [Attachment 79005] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Levi Weintraub <leviw at google.com>'s
request for review:
Bug 52380: Lazily generate and store logical ordering of InlineBoxes
https://bugs.webkit.org/show_bug.cgi?id=52380

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=79005&action=review

Looks good.  tis OK as is, but we could go another round now, or we can clean
up more of this (old ugly!) code in later patches.

> Source/WebCore/rendering/RootInlineBox.cpp:209
> +void RootInlineBox::childAdded()

Seems if we're adding this method we might as well pass the InlineBox*, even if
it's ignored.

> Source/WebCore/rendering/RootInlineBox.cpp:587
> +	   if (r->bidiLevel() > maxLevel)
> +	       maxLevel = r->bidiLevel();
> +	   if (r->bidiLevel() < minLevel)
> +	       minLevel = r->bidiLevel();

Hah.  These are just std::min, std::max :)

> Source/WebCore/rendering/RootInlineBox.cpp:615
> +	       while (iter != end) {
> +		   if ((*iter)->bidiLevel() >= minLevel)
> +		       break;
> +		   ++iter;
> +	       }

This is repeated twice and should be an inline which returns a bool or similar
maybe?

> Source/WebCore/rendering/RootInlineBox.h:79
>      void childRemoved(InlineBox* box);

"box" can be removed.