[webkit-reviews] review denied: [Bug 25648] When the video controls are updated the whole video is redisplayed (RenderSlider repaint). : [Attachment 41612] Prevent excessive relayouts

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Maciej Stachowiak <mjs at apple.com>'s request for
review:
Bug 25648: When the video controls are updated the whole video is  redisplayed
(RenderSlider repaint).
https://bugs.webkit.org/show_bug.cgi?id=25648

Attachment 41612: Prevent excessive relayouts
https://bugs.webkit.org/attachment.cgi?id=41612&action=review

------- Additional Comments from mitz at webkit.org
(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > If you mark thumb to relayout, the request will propogate to RenderView
and
> > > full relayout is resulted.
> > > 
> > > If you set RenderSlider as relayour boudnary and mark thumb to relayout,
the
> > > request will stop at RenderSlider. But there will be no relayout at all
because
> > > RenderSlider::needsRelayout() returns false.
> > 
> > Do you mean needsLayout()? Why would it still return false?
> 
> Yes, I meant needsLayout().
> 
> RenderSlider is a relayout boundary, thumb which is its child is marked for
> relayout but RenderSlider is not marked so.
> 
> See RenderObject::markContainingBlocksForLayout() in RenderObject.h
> 
> 996:	      last = o;
> 997:	      if (scheduleRelayout && objectIsRelayoutBoundary(last))
> 998:		  break;

By this time, o has been marked for layout.

You should only do the following:
1. Make the slider a relayout boundary, like you did
2. Short-circuit the width calculation in RenderSlider::layout(), like you did
3. In updateFromElement(), mark the thumb’s renderer for layout. Do not mark
the slider itself. Also, the assertion you added there is pointless.
4. In setValueForPosition(), mark the thumb’s renderer for layout. Again, do
not mark the slider itself.

Finally, when I tried doing the above I ran into a repaint issue, which is
caused by RenderSlider::layout() pushing (wrong) layout state. The
LayoutStateMaintainer initialization strangely passes size() as the offset. I
don’t think there’s any need to push layout state there.