Bug 144035 – Create RenderRubyText for <rt> only when the parent renderer is a RenderRuby.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

Otherwise create a generic renderer.

[alan]

Created attachment 251299 [details]
Patch

[alan]

EWSing.

[alan]

Created attachment 251301 [details]
Patch

[alan]

Created attachment 251317 [details]
Patch

[alan]

Created attachment 251326 [details]
Patch

[Darin Adler]

Comment on attachment 251326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251326&action=review

We have similar requirements in tables, but this seems like a different way of resolving the problem. Could you double check that we can’t do this the way we do it for tables? I guess in the case of tables, if there is no table for the cell, it creates an anonymous table.

> Source/WebCore/ChangeLog:8
> +        RenderRubyText requires its parent to be RenderRubyRun.

Sounds like a simple straightforward fix. You’d just have to pass the context in to the function creating the renderer.

> Source/WebCore/ChangeLog:10
> +        This patch also moves ruby renderer initialization logic from RenderElement::createFor() to
> +        HTMLRTElement::createElementRenderer() and HTMLRubyElement::createElementRenderer.

Doing this seems to have a significant additional refactoring cost, not required for the bug fix above. Are you sure this is the best way to accomplish this? Why do we need to combine these two changes? Could we get the insertion position in to RenderElement::createFor without changing the arguments to createElementRenderer? If not, could we at least do it without adding the two new DOM classes? That seems like an acceptable additional change, but easily separable from the main change. Is that simply to avoid adding an argument to RenderElement::createFor?

> Source/WebCore/html/HTMLRTElement.h:37
> +class HTMLRTElement final : public HTMLElement {

I know this sounds strange, but generally if the element class is not visible in the DOM, then we don’t use the HTML prefix on it. We’d just call this RubyTextElement as a way to remind us that there is no DOM class named HTMLRTElement. I guess it’s OK to not do that here.

> Source/WebCore/html/HTMLRTElement.h:55
> +inline RenderPtr<RenderElement> HTMLRTElement::createElementRenderer(Ref<RenderStyle>&& style, const RenderTreePosition& insertionPosition)

Given that this is a virtual function, it doesn’t seem valuable to mark it inline and put it in a header. I guess you just wanted to save the trouble of having a .cpp file?

> Source/WebCore/html/HTMLRTElement.h:58
> +    if (isRuby(&insertionPosition.parent()))

Why is the "&" needed here? Can we overload isRuby so it works on a reference?

> Source/WebCore/html/HTMLRubyElement.h:34
> +class HTMLRubyElement final : public HTMLElement {

I know this sounds strange, but generally if the element class is not visible in the DOM, then we don’t use the HTML prefix on it. We’d just call this RubyElement as a way to remind us that there is no DOM class named HTMLRubyElement. I guess it’s OK to not do that here.

> Source/WebCore/html/HTMLRubyElement.h:52
> +inline RenderPtr<RenderElement> HTMLRubyElement::createElementRenderer(Ref<RenderStyle>&& style, const RenderTreePosition& insertionPosition)

Given that this is a virtual function, it doesn’t seem valuable to mark it inline and put it in a header. I guess you just wanted to save the trouble of having a .cpp file?

> Source/WebCore/html/HTMLRubyElement.h:54
> +    if (style.get().display() == INLINE)

The syntax style->display() also works on a Ref.

> Source/WebCore/style/RenderTreePosition.cpp:53
> +RenderTreePosition::RenderTreePosition(RenderView& root)
> +    : m_parent(root)
> +    , m_hasValidNextSibling(true)
> +{
> +}
> +
> +RenderTreePosition::RenderTreePosition(RenderElement& parent)
> +    : m_parent(parent)
> +{
> +}
> +
> +RenderTreePosition::RenderTreePosition(RenderElement& parent, RenderObject* nextSibling)
> +    : m_parent(parent)
> +    , m_nextSibling(nextSibling)
> +    , m_hasValidNextSibling(true)
> +{
> +}

I think these constructors should be inlined.

> Source/WebCore/style/RenderTreePosition.h:41
> +    RenderElement& parent() { return m_parent; }
> +    const RenderElement& parent() const { return m_parent; }

Doesn’t make sense that a const RenderTreePosition yields up a const RenderElement&; I think even a const RenderTreePosition could just yield a RenderElement&. After all, the const-ness of the position is about whether the position is the same, and seems to have no bearing on whether the element itself can be modified.

> Source/WebCore/style/RenderTreePosition.h:64
> +};
> +
> +
> +inline bool RenderTreePosition::canInsert(RenderElement& renderer) const

Extra blank line here. Should just be one.

[Antti Koivisto]

Having these ruby-specific cases in RenderElement::createFor is already pretty ugly and passing the context uglifies it further. That's why I suggested that we might want to refactor along these lines. If we need a minimal easy-to-merge fix then this really should be done in two steps.

You should add radar number (if any) here and to the ChangeLog.

[alan]

> Doing this seems to have a significant additional refactoring cost, not
> required for the bug fix above. Are you sure this is the best way to
> accomplish this? Why do we need to combine these two changes? Could we get
bug 144058

> I know this sounds strange, but generally if the element class is not
> visible in the DOM, then we don’t use the HTML prefix on it. We’d just call
> this RubyTextElement as a way to remind us that there is no DOM class named
> HTMLRTElement. I guess it’s OK to not do that here.
Fixed. I actually prefer RubyTextElement to HTMLRTElement.

> 
> > Source/WebCore/html/HTMLRTElement.h:55
> > +inline RenderPtr<RenderElement> HTMLRTElement::createElementRenderer(Ref<RenderStyle>&& style, const RenderTreePosition& insertionPosition)
> 
> Given that this is a virtual function, it doesn’t seem valuable to mark it
> inline and put it in a header. I guess you just wanted to save the trouble
> of having a .cpp file?
Fixed. (added .cpp)

> > Source/WebCore/html/HTMLRubyElement.h:34
> > +class HTMLRubyElement final : public HTMLElement {
> 
> I know this sounds strange, but generally if the element class is not
> visible in the DOM, then we don’t use the HTML prefix on it. We’d just call
> this RubyElement as a way to remind us that there is no DOM class named
> HTMLRubyElement. I guess it’s OK to not do that here.
Fixed.

> 
> > Source/WebCore/html/HTMLRubyElement.h:52
> > +inline RenderPtr<RenderElement> HTMLRubyElement::createElementRenderer(Ref<RenderStyle>&& style, const RenderTreePosition& insertionPosition)
> 
> Given that this is a virtual function, it doesn’t seem valuable to mark it
> inline and put it in a header. I guess you just wanted to save the trouble
> of having a .cpp file?
Fixed. (added .cpp)

> 
> > Source/WebCore/html/HTMLRubyElement.h:54
> > +    if (style.get().display() == INLINE)
> 
> The syntax style->display() also works on a Ref.
Fixed.

[alan]

Created attachment 251374 [details]
Patch

[alan]

> > Source/WebCore/html/HTMLRTElement.h:58
> > +    if (isRuby(&insertionPosition.parent()))
> 
> Why is the "&" needed here? Can we overload isRuby so it works on a
> reference?
Fixed.

> > Source/WebCore/style/RenderTreePosition.cpp:53
> > +RenderTreePosition::RenderTreePosition(RenderView& root)
> > +    : m_parent(root)
> > +    , m_hasValidNextSibling(true)
> > +{
> > +}
> > +
> > +RenderTreePosition::RenderTreePosition(RenderElement& parent)
> > +    : m_parent(parent)
> > +{
> > +}
> > +
> > +RenderTreePosition::RenderTreePosition(RenderElement& parent, RenderObject* nextSibling)
> > +    : m_parent(parent)
> > +    , m_nextSibling(nextSibling)
> > +    , m_hasValidNextSibling(true)
> > +{
> > +}
> 
> I think these constructors should be inlined.
Fixed.

> 
> > Source/WebCore/style/RenderTreePosition.h:41
> > +    RenderElement& parent() { return m_parent; }
> > +    const RenderElement& parent() const { return m_parent; }
> 
> Doesn’t make sense that a const RenderTreePosition yields up a const
> RenderElement&; I think even a const RenderTreePosition could just yield a
> RenderElement&. After all, the const-ness of the position is about whether
> the position is the same, and seems to have no bearing on whether the
> element itself can be modified.
Fixed.

> 
> > Source/WebCore/style/RenderTreePosition.h:64
> > +};
> > +
> > +
> > +inline bool RenderTreePosition::canInsert(RenderElement& renderer) const
> 
> Extra blank line here. Should just be one.
Fixed.

[WebKit Commit Bot]

Comment on attachment 251374 [details]
Patch

Clearing flags on attachment: 251374

Committed r183160: <http://trac.webkit.org/changeset/183160>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.