[webkit-reviews] review denied: [Bug 116689] Regression(r145959) Convert <select> to new flex box causes strange positioning problems : [Attachment 203284] patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Roger Fong <roger_fong at apple.com>'s
request for review:
Bug 116689: Regression(r145959) Convert <select> to new flex box causes strange
positioning problems
https://bugs.webkit.org/show_bug.cgi?id=116689

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
This patch looks correct to me for the most part. It definitely seems wrong to
me that we clearLayoutOverflow in the case where we've delayed updating
scrollinfo.

-I'd prefer to see the clearLayoutOverflow call in
RenderBlock::updateScrollInfoAfterLayout in the !gDelayUpdateScrollInfo and
style()->isFlippedBlocksWritingMode() cases and removed from
RenderBlock::layout entirely. Keeps all the delayed update logic together.
-Would be good to add a ASSERT(!gDelayUpdateScrollInfo) to clearLayoutOverflow.
Related to this, as best I can tell, we only ever call this on RenderBlocks, so
we could move this function from RenderBox into RenderBlock to avoid adding
gDelayUpdateScrollInfo knowledge to RenderBox.
-Not directly related to your patch, but while we're in this code...it doesn't
seem like the hasControlClip check provides any value. clearLayoutOverflow
already early returns if !m_overflow. I might be missing something here though.

-Needs a test obviously. :)

Also, reading the backthread a bit, I'm surprised that removing the baseline
code in RenderFlexibleBox didn't cause any test failures. The patch that added
those definitely had tests and there are a bunch of baseline tests in
css3/flexobx. Maybe the relevant test is skipped?