Bug 67858 – Pixel result shows that compositing/iframes/repaint-after-losing-scrollbars.html is failing

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

Loading compositing/iframes/repaint-after-losing-scrollbars.html shows that the test is failing; the scrollbars remain visible on Mac in Safari.

[Simon Fraser (smfr)]

I have non-overlay scrollbars and am testing on Lion.

[James Robinson]

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.

[Simon Fraser (smfr)]

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?

[Simon Fraser (smfr)]

I filed 67878 to track the pixel test issue.

[Simon Fraser (smfr)]

Created attachment 106952 [details]
Patch

[Darin Adler]

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.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/96070

[Csaba Osztrogonác]

(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

[Tor Arne Vestbø]

(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/

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

Comment on attachment 106952 [details]
Patch

remove r+, because it was landed and rolled out and the bug is reopened.

[Simon Fraser (smfr)]

Back in in http://trac.webkit.org/changeset/96138. r96130 fixed the crash.