[webkit-reviews] review denied: [Bug 102562] Text Autosizing: Clusters should use width of LCA of their text nodes : [Attachment 176221] Patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Anton Vayvod
<avayvod at chromium.org>'s request for review:
Bug 102562: Text Autosizing: Clusters should use width of LCA of their text
nodes
https://bugs.webkit.org/show_bug.cgi?id=102562

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

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=176221&action=review


Please fix style first, so style comments for now

> Source/WebCore/rendering/TextAutosizer.cpp:345
> +	   blockWithAllText = lower->containingBlock();
> +    ASSERT(blockWithAllText->isDescendantOf(cluster));
> +    return blockWithAllText;

I would add a newline after the if-else block and one before return

> Source/WebCore/rendering/TextAutosizer.cpp:351
> +    return findFinalTextLeafNotInCluster(
> +	   parent, depth, &RenderObject::firstChild,
&RenderObject::nextSibling);

put that on one line, following normal coding style

> Source/WebCore/rendering/TextAutosizer.cpp:357
> +    return findFinalTextLeafNotInCluster(
> +	   parent, depth, &RenderObject::lastChild,
&RenderObject::previousSibling);

same

> Source/WebCore/rendering/TextAutosizer.cpp:364
> +RenderObject* TextAutosizer::findFinalTextLeafNotInCluster(
> +    RenderObject* parent,
> +    size_t& depth,
> +    RenderObject* (RenderObject::*getFirstChildMethod)() const,
> +    RenderObject* (RenderObject::*getNextSiblingMethod)() const)

this HAS to be on one line according to style

> Source/WebCore/rendering/TextAutosizer.cpp:375
> +	   RenderObject* leaf = findFinalTextLeafNotInCluster(
> +	       child, depth, getFirstChildMethod, getNextSiblingMethod);

same

> Source/WebCore/rendering/TextAutosizer.h:88
> +    static RenderObject* findFirstTextLeafNotInCluster(
> +	   RenderObject* parent, size_t& depth);
> +    // Finds the last leaf text node child that doesn't belong to any
cluster.
> +    static RenderObject* findLastTextLeafNotInCluster(
> +	   RenderObject* parent, size_t& depth);
> +    static RenderObject* findFinalTextLeafNotInCluster(
> +	   RenderObject* parent,
> +	   size_t& depth,
> +	   RenderObject* (RenderObject::*getFirstChildMethod)() const,
> +	   RenderObject* (RenderObject::*getNextSiblingMethod)() const);

Wrong coding style. These should be on one line.