Bug 67858 - Pixel result shows that compositing/iframes/repaint-after-losing-scrollbars.html is failing
Summary: Pixel result shows that compositing/iframes/repaint-after-losing-scrollbars.h...
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: Simon Fraser (smfr)
URL:
Keywords:
Depends on: 68872 68893
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-09 11:26 PDT by Simon Fraser (smfr)
Modified: 2011-09-27 12:00 PDT (History)
4 users (show)

See Also:


Attachments
Patch (8.40 KB, patch)
2011-09-09 18:54 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2011-09-09 11:26:24 PDT
Loading compositing/iframes/repaint-after-losing-scrollbars.html shows that the test is failing; the scrollbars remain visible on Mac in Safari.
Comment 1 Simon Fraser (smfr) 2011-09-09 11:32:00 PDT
I have non-overlay scrollbars and am testing on Lion.
Comment 2 James Robinson 2011-09-09 11:45:22 PDT
The tests works as expected on chromium.  On mac it looks like it's a graphical issue (the scrollbar pixels are there, but there's no actual scrollbar widget).  Looks like we aren't routing an invalidation to the composited layer when the scrollbars vanish.  I suspect that either those invalidations are going nowhere or to the root.

Can you try on lion with overlay scrollbars?  Mac with overlay scrollbars behaves almost identically to chromium.
Comment 3 Simon Fraser (smfr) 2011-09-09 16:45:11 PDT
I don't see any code that does repaints when scrollbars come and go. Maybe we were just relying on the native widgets to repaint?
Comment 4 Simon Fraser (smfr) 2011-09-09 18:48:53 PDT
I filed 67878 to track the pixel test issue.
Comment 5 Simon Fraser (smfr) 2011-09-09 18:54:17 PDT
Created attachment 106952 [details]
Patch
Comment 6 Darin Adler 2011-09-09 19:02:24 PDT
Comment on attachment 106952 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        Tested by compositing/iframes/repaint-after-losing-scrollbars.html, even though the pixel
> +        result doesn't show the correct result (bug 67878).

Does the expected result of that test need to be updated, or is it already failing?

> Source/WebCore/platform/ScrollView.cpp:583
> +        if (!m_horizontalScrollbar && !m_verticalScrollbar && !oldScrollCornerRect.isEmpty())
> +            invalidateScrollCornerRect(oldScrollCornerRect);

I’m not sure I understand which call sites are responsible for checking if the rectangle is empty. There is a check here, but no check in RenderLayerCompositor::destroyRootLayer. Is it important to check everywhere? Or unnecessary to check anywhere? Or helpful to check in some places even if not in others?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1746
> +        m_renderView->frameView()->invalidateScrollCorner(m_renderView->frameView()->scrollCornerRect());

If frameView() is a non-trivial function it might be nice to call it only once.
Comment 7 Simon Fraser (smfr) 2011-09-26 22:49:49 PDT
http://trac.webkit.org/changeset/96070
Comment 8 Csaba Osztrogonác 2011-09-26 23:37:16 PDT
(In reply to comment #7)
> http://trac.webkit.org/changeset/96070

It made 2 tests crash on the Qt bot:
fast/loader/stateobjects/pushstate-clears-forward-history.html
fast/frames/frame-dead-region.html

http://build.webkit.org/results/Qt%20Linux%20Release/r96070%20%2837910%29/results.html
Comment 10 Csaba Osztrogonác 2011-09-27 04:12:05 PDT
(In reply to comment #9)
> (In reply to comment #7)
> > http://trac.webkit.org/changeset/96070
> 
> SL-WK2 tests started crashing after this it seems:
> 
> http://build.webkit.org/builders/SnowLeopard%20Intel%20Debug%20%28WebKit2%20Tests%29/builds/1939
> 
> Similar symptoms on the Qt -WK2 bot:
> 
> http://build.webkit.sed.hu/builders/x86-32%20Linux%20Qt%20Release%20WebKit2/builds/12394/

Here is the bugreport: https://bugs.webkit.org/show_bug.cgi?id=68872
Comment 11 Csaba Osztrogonác 2011-09-27 06:06:03 PDT
Comment on attachment 106952 [details]
Patch

remove r+, because it was landed and rolled out and the bug is reopened.
Comment 12 Simon Fraser (smfr) 2011-09-27 12:00:38 PDT
Back in in http://trac.webkit.org/changeset/96138. r96130 fixed the crash.