Bug 66338 – [chromium] 2 repaint test failures on mac

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Tony Chang]

fast/repaint/background-scaling.html and fast/repaint/scale-page-shrink.html started failing pixel results.

I think this is due to http://trac.webkit.org/changeset/93136 , but I'm bisecting just to be sure.

[Tony Chang]

Yup, I bisected the build and it's definitely r93136 that causes these 2 tests to fail.

asvitkine, can you debug this failure?

I added the failures to test_expectations.txt for now.  Please remove when you get the test passing.

I will debug tomorrow. I have an idea of what might be the cause in my patch.

So it looks like the tests use eventSender.scalePageBy(...), which is presumably meant to to perform the equivalent of Cmd- and Cmd+ (i.e. zooming the page).

It looks like this triggers the painting of the overhang area in the tests, which happened to pass because it would have been painted white before, but not anymore.

So I believe this is a problem with scalePageBy() (which is something exposed by the test harness) not doing the same thing as a Cmd- / Cmd+. It should be trigger the content area size to grow so that the page's background should fill the empty space - rather than the overhang area.

So there's a difference in behaviour between Chromium's DRT and the 'mac' platform DRT here.

Chromium's ends up painting the overhang areas as a result of ScrollView::paint() being called. Mac platform DRT instead calls ScrollView::paintContents(), which doesn't execute the path that paints the overhang areas.

The call stacks are as follows.

Chromium:

#0  WebCore::ScrollView::paint (this=0x1833200, context=0xbfffbff8, rect=@0xbfffbfa4) at /Users/asvitkine/WebKit/Source/WebCore/WebCore.gyp/../platform/ScrollView.cpp:988
#1  0x46ca872b in WebKit::WebFrameImpl::paintWithContext (this=0x24309fe0, gc=@0xbfffbff8, rect=@0xbfffc1a0) at /Users/asvitkine/WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp:1957
#2  0x46ca8862 in WebKit::WebFrameImpl::paint (this=0x24309fe0, canvas=0x24329cc0, rect=@0xbfffc1a0) at /Users/asvitkine/WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp:1969
#3  0x46cfe002 in WebKit::WebViewImpl::paint (this=0x24306b00, canvas=0x24329cc0, rect=@0xbfffc1a0) at /Users/asvitkine/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp:1135
#4  0x46c02031 in WebViewHost::paintRect (this=0x24306980, rect=@0xbfffc1a0) at /Users/asvitkine/WebKit/Source/WebKit/chromium/../../../Tools/DumpRenderTree/chromium/WebViewHost.cpp:1502
#5  0x46c0232e in WebViewHost::paintInvalidatedRegion (this=0x24306980) at /Users/asvitkine/WebKit/Source/WebKit/chromium/../../../Tools/DumpRenderTree/chromium/WebViewHost.cpp:1539
#6  0x46bcda74 in LayoutTestController::display (this=0x24303ad0, arguments=@0xbfffc368, result=0xbfffc35c) at /Users/asvitkine/WebKit/Source/WebKit/chromium/../../../Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1177

Mac:


#0  -[WebFrame(WebInternal) _drawRect:contentsOnly:] (self=0x12d309790, _cmd=0x7fff86c718b3, rect={origin = {x = 0, y = 0}, size = {width = 800, height = 600}}, contentsOnly=1 '\001') at /Users/asvitkine/WebKit/Source/WebKit/mac/WebView/WebFrame.mm:559
#1  0x0000000100ba655e in -[WebHTMLView drawSingleRect:] (self=0x12d326080, _cmd=0x7fff86c9b84c, rect={origin = {x = 0, y = 0}, size = {width = 800, height = 600}}) at /Users/asvitkine/WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:3221
#2  0x0000000100ba619c in -[WebHTMLView drawRect:] (self=0x12d326080, _cmd=0x7fff859df560, rect={origin = {x = 0, y = 0}, size = {width = 800, height = 600}}) at /Users/asvitkine/WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:3265
#3  0x00007fff853a0cc5 in -[NSView _drawRect:clip:] ()
#4  0x00007fff8539f938 in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] ()
#5  0x0000000100b9d85f in -[WebHTMLView(WebPrivate) _recursiveDisplayAllDirtyWithLockFocus:visRect:] (self=0x12d326080, _cmd=0x7fff859e012c, needsLockFocus=1 '\001', visRect={origin = {x = 0, y = 0}, size = {width = 800, height = 600}}) at /Users/asvitkine/WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:1369
#6  0x00007fff8539fca2 in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] ()
#7  0x00007fff8539fca2 in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] ()
#8  0x00007fff8539fca2 in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] ()
#9  0x00007fff8539fca2 in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] ()
#10 0x00007fff8539fca2 in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] ()
#11 0x00007fff8539fca2 in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] ()
#12 0x00007fff8539e00a in -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#13 0x00007fff854bc9c4 in -[NSNextStepFrame _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#14 0x00007fff8539a3de in -[NSView _displayRectIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:] ()
#15 0x00007fff85313c0e in -[NSView displayIfNeeded] ()
#16 0x00007fff852c95a4 in -[NSNextStepFrame displayIfNeeded] ()
#17 0x00007fff852dbc3b in -[NSWindow _reallyDoOrderWindow:relativeTo:findKey:forCounter:force:isModal:] ()
#18 0x00007fff852db7d2 in -[NSWindow orderWindow:relativeTo:] ()
#19 0x0000000100011d2d in createWebViewAndOffscreenWindow () at /Users/asvitkine/WebKit/Tools/DumpRenderTree/mac/DumpRenderTree.mm:325
#20 0x000000010001305b in dumpRenderTree (argc=3, argv=0x7fff5fbff788) at /Users/asvitkine/WebKit/Tools/DumpRenderTree/mac/DumpRenderTree.mm:664
#21 0x000000010001334c in main (argc=3, argv=0x7fff5fbff788) at /Users/asvitkine/WebKit/Tools/DumpRenderTree/mac/DumpRenderTree.mm:727

We could make Chromium's DRT call ScrollView::paintContents() instead of ScrollView::paint() by using WebViewImpl::createRootLayerPainter() from WebViewHost::paintRect() to match Apple's DRT, but I don't understand the implications of such a change.

+jamesr

James, do you have any thoughts on this? Do you know if there's a reason behind the difference in behaviour between the Chromium DRT vs. Apple's?

Correction, that should be FrameView::paintContents() rather than ScrollView::paintContents() which doesn't exist. But FrameView is a ScrollView, so otherwise my comment are still accurate.

Ignoring the difference between Mac platform DRT and Chromium's DRT, there's the question of why the ScrollView thinks there's scroll overhang.

When it checks:

if (physicalScrollX > contentsWidth() - visibleContentRect().width())

It ends up being true due to: 0 > 160 - 800

Of interest, this means that contentsSize().width() < visibleContentRect().width(), which contradicts the comment in ScrollView.cpp:

    IntSize contentsSize() const; // Always at least as big as the visibleWidth()/visibleHeight().


Investigating....

So it seems as a result of the scale contentsSize() goes from 800x600 to 160x120.  This is true even for Mac platform DRT.

The change in size is as a result of:


0  WebCore::ScrollView::setContentsSize (this=0x829c00, newSize=@0xbfffc028) at /Users/asvitkine/WebKit/Source/WebCore/WebCore.gyp/../platform/ScrollView.cpp:289
#1  0x1406f058 in WebCore::FrameView::setContentsSize (this=0x829c00, size=@0xbfffc028) at /Users/asvitkine/WebKit/Source/WebCore/WebCore.gyp/../page/FrameView.cpp:491
#2  0x1406cf23 in WebCore::FrameView::adjustViewSize (this=0x829c00) at /Users/asvitkine/WebKit/Source/WebCore/WebCore.gyp/../page/FrameView.cpp:517
#3  0x1406dc80 in WebCore::FrameView::layout (this=0x829c00, allowSubtree=true) at /Users/asvitkine/WebKit/Source/WebCore/WebCore.gyp/../page/FrameView.cpp:1089
#4  0x14053dd6 in WebCore::Frame::scalePage (this=0x3032600, scale=0.200000003, origin=@0xbfffc1e0) at /Users/asvitkine/WebKit/Source/WebCore/WebCore.gyp/../page/Frame.cpp:1109

I can confirm that RenderView::documentRect() doesn't get shrunk when using Cmd- in Chromium, so FrameView::contentsSize() doesn't end up getting smaller like it does in DRT.

So in DRT, documentRect() is smaller because hasTransform() is true due to:

#0  WebCore::RenderStyle::setPageScaleTransform (this=0x258028f0, scale=0.200000003) at WebKit/Source/WebCore/WebCore.gyp/../rendering/style/RenderStyle.cpp:756
#1  0x798ee6ba in WebCore::CSSStyleSelector::styleForDocument (document=0x180c600) at WebKit/Source/WebCore/WebCore.gyp/../css/CSSStyleSelector.cpp:1162
#2  0x791f5199 in WebCore::Document::recalcStyle (this=0x180c600, change=WebCore::Node::Force) at WebKit/Source/WebCore/WebCore.gyp/../dom/Document.cpp:1527
#3  0x79c34a62 in WebCore::Frame::scalePage (this=0x1010600, scale=0.200000003, origin=@0xbfffc1d0) at WebKit/Source/WebCore/WebCore.gyp/../page/Frame.cpp:1053
#4  0x77c64b52 in WebKit::WebViewImpl::scalePage (this=0x40ca50, scaleFactor=0.200000003, origin={x = 0, y = 0}) at WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp:1856


This is different than the zooming functionality (i.e. Cmd-/Cmd+), which in Chrome ends up hitting:

#0  WebCore::RenderStyle::setPageScaleTransform (this=0x61fc20, scale=1) at WebKit/Source/WebCore/WebCore.gyp/../rendering/style/RenderStyle.cpp:756
#1  0x3ae8ad96 in WebCore::CSSStyleSelector::styleForDocument (document=0x821800) at WebKit/Source/WebCore/WebCore.gyp/../css/CSSStyleSelector.cpp:1162
#2  0x3a78f101 in WebCore::Document::recalcStyle (this=0x821800, change=WebCore::Node::Force) at WebKit/Source/WebCore/WebCore.gyp/../dom/Document.cpp:1527
#3  0x3b1d7ba2 in WebCore::Frame::setPageAndTextZoomFactors (this=0x105ca00, pageZoomFactor=0.694444418, textZoomFactor=1) at WebKit/Source/WebCore/WebCore.gyp/../page/Frame.cpp:1064
#4  0x3a30346a in WebKit::WebViewImpl::setZoomLevel (this=0x415820, textOnly=false, zoomLevel=-2) at WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp:1800
#5  0x3c6d9002 in RenderView::OnZoom (this=0x1025c00, function=PageZoom::ZOOM_OUT) at src/content/renderer/render_view.cc:3409

The two appear to be orthogonal. Note that in the first case, WebCore::RenderStyle::setPageScaleTransform() is called with scale != 1, whereas in the second case, scale is always 1 (even with pageZoomFactor=0.69).

In the first case, the render style ends up having a page scale transform, whereas in the case of zooming, it doesn't. This explains the difference between Chromium zooming these tests with DRT.

I don't understand under what circumstances scalePage() code would be triggered in the context of a Chromium web browser, if ever, given that's its not used for zooming.

[Tony Chang]

(In reply to comment #11)
> I don't understand under what circumstances scalePage() code would be triggered in the context of a Chromium web browser, if ever, given that's its not used for zooming.

Random guess: Maybe printing?

(In reply to comment #12)
> (In reply to comment #11)
> > I don't understand under what circumstances scalePage() code would be triggered in the context of a Chromium web browser, if ever, given that's its not used for zooming.
> 
> Random guess: Maybe printing?

It seems from Chromium code, we do call scalePage() from RenderView::OnZoom(), but only under TOUCH_UI.

If that is being used for zooming-like behavior, it still seems wrong to have documentRect() shrink due to the transform...

RenderView::documentRect() was changed to be affected by the page transform in:

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

Regarding my earlier comments which found that Mac platform DRT uses FrameView::paintContents() whereas Chromium DRT uses FrameView::paint():

Making Chromium DRT use paintContents() does make the two tests pass again. But it fails many other tests - because this change causes scroll bars not to be drawn.

The Mac platform DRT can get away with this because it seems that it uses a separate view to draw its scroll bars (under Snow Leopard), rather than relying on ScrollView to do it.

So I see the following ways to resolve this:

1. Make scalePage() not result in documentRect() shrinking. This may require a lot of buy-in / contention from other users of scalePage() such as Safari.

2. Make our DRT use FrameView::paintContents() instead of FrameView::paint(). This doesn't seem feasible since, unlike Mac platform DRT, we can't rely on something else to paint the scrollbars.

3. Make ScrollView::paint() not think there's an overhang if visibleContentSize() > contentsSize(). This is pretty easy to do, but I'm not convinced that this makes sense. This will result in consistent results between Mac platform DRT and Chromium DRT, since neither will paint the overhang (though for different reasons).

4. Re-baseline the test results for Chromium or keep the tests excluded.

[Peter Kasting]

Pinging on this.  http://trac.webkit.org/changeset/100196/ affected how scaling works, and I'm needing to rebaseline some tests, but I can't rebaseline the Mac ones due to this issue.  I don't know how to move this forward, so I'm just asking as the current WK sheriff that Alexei or someone else take this up.

I believe when I had discussed this with Tony at the time, we decided it was best not to do any of the changes proposed in 1., 2., or 3. above.

Perhaps its best to just re-baseline the tests to the new behaviour?

[Tony Chang]

(In reply to comment #18)
> I believe when I had discussed this with Tony at the time, we decided it was best not to do any of the changes proposed in 1., 2., or 3. above.
> 
> Perhaps its best to just re-baseline the tests to the new behaviour?

Yeah, I think the conclusion was that this is an intentional difference between chromium DRT and Apple DRT because of how scrollbars are painted.  Seems fine to rebaseline.

[Peter Kasting]

OK.  I didn't get to this today.  If one of you wants to tackle this and rebaseline all the relevant tests, that'd be great.  Otherwise I'll try to make sure I don't lose this.

[Tony Chang]

Created attachment 119641 [details]
Patch

[Tony Chang]

Comment on attachment 119641 [details]
Patch

asvitkine, does this look correct to you?

LGTM

[Tony Chang]

Created attachment 119696 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 119696 [details]
Patch for landing

Clearing flags on attachment: 119696

Committed r103136: <http://trac.webkit.org/changeset/103136>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.