[webkit-reviews] review granted: [Bug 123860] Widget's position change should not initiate layout, only when its size changes. : [Attachment 216121] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <akling at apple.com> has granted Zalan Bujtas <zalan at apple.com>'s
request for review:
Bug 123860: Widget's position change should not initiate layout, only when its
size changes.
https://bugs.webkit.org/show_bug.cgi?id=123860

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

------- Additional Comments from Andreas Kling <akling at apple.com>
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.