[webkit-reviews] review denied: [Bug 53642] pop-up button text not rendered correctly according to its direction in <option> : [Attachment 81007] patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Xiaomei Ji <xji at chromium.org>'s request for review:
Bug 53642: pop-up button text not rendered correctly according to its direction
in <option>
https://bugs.webkit.org/show_bug.cgi?id=53642

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

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=81007&action=review

Good catch! I thought I had this covered by the change to RenderStyle::diff(),
but of course I didn’t, because adjustInnerStyle() just pokes the style
directly.

> Source/WebCore/rendering/RenderMenuList.cpp:216
>	       m_buttonText->setText(s.impl());
> +	       if (!m_buttonText->selfNeedsLayout() &&
document()->page()->chrome()->selectItemAlignmentFollowsMenuWritingDirection()
&& m_optionStyle
> +		   && (m_optionStyle->direction() !=
m_innerBlock->style()->direction() || m_optionStyle->unicodeBidi() !=
m_innerBlock->style()->unicodeBidi()))
> +		   m_buttonText->setNeedsLayout(true);

I don’t think this careful checking of the conditions is really necessary, so
you could just accomplish this by calling setText(s.imply(), true) above. But
really the right thing to do is to do a RenderStyle::diff() in
adjustInnerStyle() and if you get a layout difference, call
setNeedsLayoutAndPrefWidthsRecalc().