[webkit-reviews] review canceled: [Bug 93056] Reimplement RenderQuote placement algorithm : [Attachment 156244] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Elliott Sprehn
<esprehn at gmail.com>'s request for review:
Bug 93056: Reimplement RenderQuote placement algorithm
https://bugs.webkit.org/show_bug.cgi?id=93056

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=156244&action=review


It's very close to being ready for landing. I would like to see another round
though.

> Source/WebCore/ChangeLog:41
> +	   * rendering/RenderObjectChildList.cpp:
> +	   (WebCore::RenderObjectChildList::removeChildNode):
> +	   (WebCore::RenderObjectChildList::appendChildNode):
> +	   (WebCore::RenderObjectChildList::insertChildNode):
> +	   * rendering/RenderQuote.cpp:
> +	   (WebCore::RenderQuote::RenderQuote):
> +	   (WebCore::RenderQuote::~RenderQuote):
> +	   (WebCore::RenderQuote::willBeDestroyed):
> +	   (WebCore::RenderQuote::originalText):
> +	   (WebCore::RenderQuote::computePreferredLogicalWidths):
> +	   (WebCore::RenderQuote::styleDidChange):
> +	   (WebCore):
> +	   (WebCore::RenderQuote::attachQuote):
> +	   (WebCore::RenderQuote::detachQuote):
> +	   (WebCore::RenderQuote::updateDepth):
> +	   * rendering/RenderQuote.h:
> +	   (RenderQuote):
> +	   * rendering/RenderView.cpp:
> +	   (WebCore::RenderView::RenderView):
> +	   * rendering/RenderView.h:
> +	   (WebCore):
> +	   (WebCore::RenderView::setRenderQuoteHead):
> +	   (WebCore::RenderView::renderQuoteHead):
> +	   (RenderView):

The function-level part of the ChangeLog should be filled.

> Source/WebCore/rendering/RenderObjectChildList.cpp:121
> +	   if (oldChild->isQuote())
> +	       toRenderQuote(oldChild)->detachQuote();

I feel like we should add the counterpart to insertChildNode. It's unclear to
me if it's needed but it would be safer. The downside is that it would also
force the quotes to be attached before preferred widths computation.

> Source/WebCore/rendering/RenderQuote.cpp:-162
> -    ASSERT(m_depth != UNKNOWN_DEPTH);

Might be good to add ASSERT(m_attached) here to keep the existing assertion.

> Source/WebCore/rendering/RenderQuote.cpp:118
> +    if (!QuotesData::equals(newQuotes, oldQuotes))
> +	   setNeedsLayoutAndPrefWidthsRecalc();

I think we should just return StyleDifferenceLayout in RenderStyle::diff for
quotes differences and kill this function.

> Source/WebCore/rendering/RenderQuote.cpp:150
> +	   quote->updateDepth();

Calling setNeedsLayoutAndPrefWidthsRecalc() during layout is bad as I am pretty
sure it is the cause of some of our quote bugs. Please add a FIXME about
removing this anti-pattern.

> Source/WebCore/rendering/RenderQuote.h:39
>  protected:

I don't see the need for the protected section as we have no renderer
inheriting from us.

> Source/WebCore/rendering/RenderView.h:199
> +    RenderQuote* renderQuoteHead() { return m_renderQuoteHead; }

It should be a const function.