[webkit-reviews] review denied: [Bug 53642] pop-up button text not rendered correctly according to its direction in <option> : [Attachment 81017] 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 81017: patch
https://bugs.webkit.org/attachment.cgi?id=81017&action=review

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

> Source/WebCore/rendering/RenderMenuList.cpp:106
> +	   StyleDifference diff = StyleDifferenceEqual;
> +	   unsigned contextSensitiveProperties = ContextSensitivePropertyNone;
> +	   diff = m_optionStyle->diff(innerStyle, contextSensitiveProperties);
> +	   if (diff == StyleDifferenceLayout)
> +	       m_innerBlock->setNeedsLayoutAndPrefWidthsRecalc();
> +	   

I think I have led you down the wrong path. My apologies. Comparing innerStyle
to m_optionStyle is likely to always result in a layout difference, since the
inner style mostly inherits from the button, and the option style is quite
different (for one, I think it has display: none). I initially intended for the
comparison to be between the inner style as it was and the adjusted inner
style, but since we don’t clone the style but rather just modify it in-place,
we can’t make this comparison. I still think this is the right place to decide
whether to call setNeedsLayoutAndPrefWidthsRecalc(), but the condition should
be more along the lines of what you initially had, i.e. if the alignment, the
direction, or the unicode-bidi changed, then layout is needed.

Sorry about misleading you.