Bug 123860 – Widget's position change should not initiate layout, only when its size changes.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

SSIA

[zalan]

Created attachment 216121 [details]
Patch

[Andreas Kling]

Comment on attachment 216121 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:12
> +        Manual test added. Unfortunately we can't test against the number of layouts yet.

It would be good to do something about this. :)

> Source/WebCore/rendering/RenderWidget.cpp:147
> +    if (boundsChanged && hasLayer() && layer()->isComposited())
>          layer()->backing()->updateAfterWidgetResize();

The name updateAfterWidgetResize() makes it sound like we wouldn't need to call it after position-only changes.
If so, we should avoid the extra work. If not, we should rename it to something more accurate, e.g updateAfterWidgetBoundsChange().

> Source/WebCore/rendering/RenderWidget.cpp:335
> -    // if the frame bounds got changed, or if view needs layout (possibly indicating
> -    // content size is wrong) we have to do a layout to set the right widget size
> -    if (m_widget && m_widget->isFrameView()) {
> +    // if the frame size got changed, or if view needs layout (possibly indicating
> +    // content size is wrong) we have to do a layout to set the right widget size.
> +    if (m_widget->isFrameView()) {

I'm a bit spooked by removing the null check here, but I don't see how we would be here with a null widget.
If we were being destroyed, the weak pointer guard above would have taken care of it.
It would be nice if we could get rid of this possible state by enforcing presence of a Widget at compile-time.
Might be a bit non-trivial though.

[zalan]

> It would be good to do something about this. :)
> 
> > Source/WebCore/rendering/RenderWidget.cpp:147
> > +    if (boundsChanged && hasLayer() && layer()->isComposited())
> >          layer()->backing()->updateAfterWidgetResize();
> 
> The name updateAfterWidgetResize() makes it sound like we wouldn't need to call it after position-only changes.
> If so, we should avoid the extra work. If not, we should rename it to something more accurate, e.g updateAfterWidgetBoundsChange().
It is indeed wrong naming as it calls both position and size changes. Will fix it.

[WebKit Commit Bot]

Comment on attachment 216121 [details]
Patch

Clearing flags on attachment: 216121

Committed r158727: <http://trac.webkit.org/changeset/158727>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.