framesets force full-view repaint unnecessarily

[Dex Deacon]

Visit the sample URL, and click in the entry box to focus it.  As the cursor flashes, repaints are issued.  However, because the entry box is within a frameset, the repaint is applied to the full contents of the frameset rather than just the entry box.

(You can check this by adding a printf() in WebFrame::paint() to print out ps.rcPaint.  Compare that to what happens when you just navigate to the frame with the entry box - http://stein.cshl.org/WWW/CGI/examples/frameset.cgi/query).

This happens because on every call to FrameView::layout, the following code is executed:
                if (body->hasTagName(framesetTag)) {
                    body->renderer()->setNeedsLayout(true);
This seems unnecessary and wrong.  I believe the correct fix is to remove the call to setNeedsLayout.  I'll make sure that doesn't break the layout tests, and submit a patch.

[Dave Hyatt]

Layout is only done if needed.  You're looking at buggy Windows code.  Repaints do not trigger a relayout normally, but the buggy windows code always calls layout over and over when painting (even when no layout is necesary).  This will lead to many strange bugs.

That said, I do agree that setNeedsLayout here is wrong.  It should be setChildNeedsLayout just like the body tag code path.

[Dex Deacon]

Created attachment 11941 [details]
only call setNeedsLayout(true) if the view's size has changed

[Dex Deacon]

Sorry Dave, I didn't see your comment until now.

Do you mean to simply replace the call to setNeedsLayout() with setChildNeedsLayout()?  That does seem to resolve the speed issues I was noticing as a result of this bug.

Or do you mean to perform the same checks as the body tag path as well?

Either way, I don't understand why setChildNeedsLayout() is better here :).

[Dave Hyatt]

Comment on attachment 11941 [details]
only call setNeedsLayout(true) if the view's size has changed

The WebKit side of the WIndows code is buggy and it is causing layout() on FrameView to get called when it shouldn't.

Cursor blinking should not cause you to re-enter layout, but it does so because WebView on Win32 is buggy.

You should not technically even have to patch any cross-platform code to fix the Windows bug.

[Dex Deacon]

Shouldn't the code still be fixed to use setChildNeedsLayout() instead of setNeedsLayout()?  That's still a bug with the WebCore code (it also has the benefit of helping the buggy Windows repaint issue).

[Dex Deacon]

Created attachment 12308 [details]
call setChildNeedsLayout on framesets

[Maciej Stachowiak]

Comment on attachment 12308 [details]
call setChildNeedsLayout on framesets

Hyatt says this is ok. r=him.

[Sam Weinig]

Landed in r19170.