Bug 119075 - With frame flattening on, too many resize events fired if document is resized in onresize handler.
Summary: With frame flattening on, too many resize events fired if document is resized...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-24 22:44 PDT by Yongjun Zhang
Modified: 2013-07-26 18:15 PDT (History)
11 users (show)

See Also:


Attachments
Don't send resize event when widget frame is being changed in RenderFrameBase::layoutWithFlattening. (3.67 KB, patch)
2013-07-26 15:16 PDT, Yongjun Zhang
simon.fraser: review+
Details | Formatted Diff | Diff
Address review comments for commit. (3.83 KB, patch)
2013-07-26 17:28 PDT, Yongjun Zhang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yongjun Zhang 2013-07-24 22:44:04 PDT
This is exposed by http://trac.webkit.org/changeset/149382.  When frame flattening is on, if we change an iframe's document size in its resize handler, we could get excessive number of resize events.
Comment 1 zalan 2013-07-25 02:34:27 PDT
I think http://trac.webkit.org/changeset/149287 made the major logic change by moving 'resize event' sending from performPostLayout() to setFrameRect() and making it sync, while http://trac.webkit.org/changeset/149382 tried to ease on that by checking against isInLayout() and sending the resize event off async.
What I think what happens is that http://trac.webkit.org/changeset/150011 copied the resize event sending back to the performPostLayout() (now we are sending it off at 2 places), but since peformPostLayout() can be invoke by a timer, the isInLayout() guarding does not help there anymore and we are sending the resize events sync.

Do you have a test case on this?
Comment 2 Yongjun Zhang 2013-07-26 14:53:25 PDT
(In reply to comment #1)
> I think http://trac.webkit.org/changeset/149287 made the major logic change by moving 'resize event' sending from performPostLayout() to setFrameRect() and making it sync, while http://trac.webkit.org/changeset/149382 tried to ease on that by checking against isInLayout() and sending the resize event off async.
> What I think what happens is that http://trac.webkit.org/changeset/150011 copied the resize event sending back to the performPostLayout() (now we are sending it off at 2 places), but since peformPostLayout() can be invoke by a timer, the isInLayout() guarding does not help there anymore and we are sending the resize events sync.
>
The problems is actually not caused by sync sending resize events.  Rather, it is caused by queueing extra resize events in DocumentEventQueue when it is still dispatching the previous resize event in DocumentEventQueue::pendingEventTimerFired, and this makes a loop in the same runloop.  Before 149287, we were dispatching the resize event in performPostLayout() in another runloop.
Comment 3 Yongjun Zhang 2013-07-26 15:16:15 PDT
Created attachment 207557 [details]
Don't send resize event when widget frame is being changed in RenderFrameBase::layoutWithFlattening.
Comment 4 Simon Fraser (smfr) 2013-07-26 17:02:25 PDT
Comment on attachment 207557 [details]
Don't send resize event when widget frame is being changed in RenderFrameBase::layoutWithFlattening. 

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

> Source/WebCore/ChangeLog:8
> +        With http://trac.webkit.org/changeset/149287, WebCore also send resize event in FrameView::setFrameRect.  When

sends

> Source/WebCore/ChangeLog:12
> +        is done layout.

is done laying out

> Source/WebCore/page/FrameView.h:441
> +    void setResizeEventAllowed(bool resizeEventAllowed) { m_resizeEventAllowed = resizeEventAllowed; }

Might as well add a const inline setter for this too.

This currently doesn't allow nesting. Does it need to (using a counter)?

> Source/WebCore/rendering/RenderFrameBase.cpp:68
> +    childFrameView->setResizeEventAllowed(false);

Could TemporaryChange<> help you here?
Comment 5 Yongjun Zhang 2013-07-26 17:15:54 PDT
(In reply to comment #4)
> (From update of attachment 207557 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207557&action=review

> > Source/WebCore/page/FrameView.h:441
> > +    void setResizeEventAllowed(bool resizeEventAllowed) { m_resizeEventAllowed = resizeEventAllowed; }
> 
> Might as well add a const inline setter for this too.

Did you mean getter?  I will add it.

> 
> This currently doesn't allow nesting. Does it need to (using a counter)?
> 
> > Source/WebCore/rendering/RenderFrameBase.cpp:68
> > +    childFrameView->setResizeEventAllowed(false);
> 
> Could TemporaryChange<> help you here?
Seems like if the method and member variable is in the same class, TemporaryChange<> would work.  In this patch, the method and member variable are from different classes.

thanks for the review.
Comment 6 Yongjun Zhang 2013-07-26 17:28:33 PDT
Created attachment 207567 [details]
Address review comments for commit.
Comment 7 WebKit Commit Bot 2013-07-26 18:15:52 PDT
Comment on attachment 207567 [details]
Address review comments for commit.

Clearing flags on attachment: 207567

Committed r153397: <http://trac.webkit.org/changeset/153397>
Comment 8 WebKit Commit Bot 2013-07-26 18:15:54 PDT
All reviewed patches have been landed.  Closing bug.