Bug 123860 - Widget's position change should not initiate layout, only when its size changes.
Summary: Widget's position change should not initiate layout, only when its size changes.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-05 20:47 PST by zalan
Modified: 2013-11-05 23:29 PST (History)
5 users (show)

See Also:


Attachments
Patch (6.02 KB, patch)
2013-11-05 21:17 PST, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2013-11-05 20:47:17 PST
SSIA
Comment 1 zalan 2013-11-05 21:17:14 PST
Created attachment 216121 [details]
Patch
Comment 2 Andreas Kling 2013-11-05 21:42:08 PST
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.
Comment 3 zalan 2013-11-05 21:45:10 PST
> 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.
Comment 4 WebKit Commit Bot 2013-11-05 23:29:55 PST
Comment on attachment 216121 [details]
Patch

Clearing flags on attachment: 216121

Committed r158727: <http://trac.webkit.org/changeset/158727>
Comment 5 WebKit Commit Bot 2013-11-05 23:29:57 PST
All reviewed patches have been landed.  Closing bug.