Bug 31281 – r50665 broke around 100 Qt layout tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Kenneth Rohde Christiansen]

Judging from the Qt buildbot, r50665 (https://bugs.webkit.org/show_bug.cgi?id=29167) broke around 100 Qt layout tests.

[Mark Mentovai]

Ossy and I were looking at this earlier.

[Csaba Osztrogonác]

Created attachment 42860 [details]
proposed patch

FrameView::FrameView() set initial value of m_canHaveScrollbars(true), but DumpRenderTree of Qt disable scrollbars by default:

WebKitTools/DumpRenderTree/qt/DumpRenderTree.cpp :
252	    m_page->mainFrame()->setScrollBarPolicy(Qt::Horizontal, Qt::ScrollBarAlwaysOff);
253	    m_page->mainFrame()->setScrollBarPolicy(Qt::Vertical, Qt::ScrollBarAlwaysOff);

If we call updateCanHaveScrollbars(), it makes QtBuildBot happy. :)

Only we have only one problem: The new fast/overflow/scrollbar-restored.html won't work on Qt, because scrollbars are disabled by DumpRenderTree. After resizing scrollbars won't be appear if it is disabled ...

Should we enable scrollbars? It would make some tests fail ...
Or should we skip only this new test?

[Kenneth Rohde Christiansen]

So you are making a crossplatform change that is only needed by Qt. I'm not sure that is correct, so someone else should probably comment on this.

Btw, you say we disable the scrollbars by default, do the other DRT's not do that?

If they don't, it might be better to change that.

[Csaba Osztrogonác]

(In reply to comment #3)
> So you are making a crossplatform change that is only needed by Qt. I'm not
> sure that is correct, so someone else should probably comment on this.

Mark, Dan, what do you think?
Dan, could you r? 

> Btw, you say we disable the scrollbars by default, do the other DRT's not do
> that?
> 
> If they don't, it might be better to change that.
AFAIK other DRT's don't disable scrollbars. I have no idea why we disable them. Maybe Simon or Adam could answer it, because they did it a long time ago: http://trac.webkit.org/changeset/25722

If we would like to enable scrollbars, we will have to fix circa 100 expected files.

[Mark Mentovai]

My opinion?  I didn't know that Qt DumpRenderTree disabled scrollbars.  That certainly casts doubt on the validity of any test involving scrollbars.  The new test added in bug 29167 certainly isn't the only one that depends on showing or hiding scrollbars.

It does seem like the proposed patch tries to paper over something in mainline WebCore that's only needed to support a questionable test environment.

What sorts of failures that you see with Qt DumpRenderTree when it doesn't disable scrollbars?  Is it just a matter of needing to rebaseline?

[Andras Becsi]

(In reply to comment #2)
> Created an attachment (id=42860) [details]
> proposed patch
> 
> FrameView::FrameView() set initial value of m_canHaveScrollbars(true), but
> DumpRenderTree of Qt disable scrollbars by default:
> 
> WebKitTools/DumpRenderTree/qt/DumpRenderTree.cpp :
> 252        m_page->mainFrame()->setScrollBarPolicy(Qt::Horizontal,
> Qt::ScrollBarAlwaysOff);
> 253        m_page->mainFrame()->setScrollBarPolicy(Qt::Vertical,
> Qt::ScrollBarAlwaysOff);

Actually it is only set to ScrollBarAlwaysOff in the constructor of DRT (as some kind of initial setting), and then reset if css or some change needs it, so it is actually _not_ disabled. 
It does not change the failing tests if we remove this two lines from DRT.cpp.
The real problem is that the failing tests do not call FrameView::updateCanHaveScrollbars() and that is why they fail. The question is why update is not called.

> If we call updateCanHaveScrollbars(), it makes QtBuildBot happy. :)
> 
> Only we have only one problem: The new fast/overflow/scrollbar-restored.html
> won't work on Qt, because scrollbars are disabled by DumpRenderTree. After
> resizing scrollbars won't be appear if it is disabled ...
> 
> Should we enable scrollbars? It would make some tests fail ...
> Or should we skip only this new test?

[Andras Becsi]

(In reply to comment #6)
> (In reply to comment #2)
> > Created an attachment (id=42860) [details] [details]
> > proposed patch
> > 
> > FrameView::FrameView() set initial value of m_canHaveScrollbars(true), but
> > DumpRenderTree of Qt disable scrollbars by default:
> > 
> > WebKitTools/DumpRenderTree/qt/DumpRenderTree.cpp :
> > 252        m_page->mainFrame()->setScrollBarPolicy(Qt::Horizontal,
> > Qt::ScrollBarAlwaysOff);
> > 253        m_page->mainFrame()->setScrollBarPolicy(Qt::Vertical,
> > Qt::ScrollBarAlwaysOff);
> 
> Actually it is only set to ScrollBarAlwaysOff in the constructor of DRT (as
> some kind of initial setting), and then reset if css or some change needs it,
> so it is actually _not_ disabled. 
> It does not change the failing tests if we remove this two lines from DRT.cpp.
> The real problem is that the failing tests do not call
> FrameView::updateCanHaveScrollbars() and that is why they fail. The question is
> why update is not called.
> 
> > If we call updateCanHaveScrollbars(), it makes QtBuildBot happy. :)
> > 
> > Only we have only one problem: The new fast/overflow/scrollbar-restored.html
> > won't work on Qt, because scrollbars are disabled by DumpRenderTree. After
> > resizing scrollbars won't be appear if it is disabled ...
> > 
> > Should we enable scrollbars? It would make some tests fail ...
> > Or should we skip only this new test?

Looks like

trunk/WebKit/qt/Api/qwebframe.cpp:870 and 876
d->frame->view()->updateCanHaveScrollbars();

are misplaced, cause QWebFrame::setScrollBarPolicy(Qt::Orientation orientation, Qt::ScrollBarPolicy policy) seems to be explicitly called only.

[Csaba Osztrogonác]

> AFAIK other DRT's don't disable scrollbars. I have no idea why we disable them.
> Maybe Simon or Adam could answer it, because they did it a long time ago:
> http://trac.webkit.org/changeset/25722
> 
> If we would like to enable scrollbars, we will have to fix circa 100 expected
> files.

Adam or Simon, could you tell us why our DRT disable scrollbars by default?

[Kenneth Rohde Christiansen]

If the other DRT doesn't "disable" the scrollbars, we shouldn't do so either.

The code:

118 m_page->mainFrame()->setHorizontalScrollBarPolicy(Qt::ScrollBarAlwaysOff); 
119 m_page->mainFrame()->setVerticalScrollBarPolicy(Qt::ScrollBarAlwaysOff); 

wasn't explicitly added. It was just part of a larger improvement of our DRT.

This indeed seems wrong as it would make it impossible to check the scrollbars for the mainframe, so I opt for changing it.

Yes, you are right, we will have to change 100 tests results, but our results should also become more like that of the other platforms, which is a good thing.

[Kenneth Rohde Christiansen]

Comment on attachment 42860 [details]
proposed patch

Wrong fix.

[Andras Becsi]

Created attachment 42872 [details]
proposed patch for removing policy settings from DRT

[Andras Becsi]

Comment on attachment 42872 [details]
proposed patch for removing policy settings from DRT

I'll do the DRT change together with the expected file changes.

[Andras Becsi]

Created attachment 42881 [details]
proposed fix for DRT and the test expected files

[Csaba Osztrogonác]

Comment on attachment 42881 [details]
proposed fix for DRT and the test expected files

You forgot to add modified expected files.

[Andras Becsi]

Created attachment 42884 [details]
another try of creating a correct patch

Thanks Ossy.

[WebKit Commit Bot]

Comment on attachment 42884 [details]
another try of creating a correct patch

Clearing flags on attachment: 42884

Committed r50757: <http://trac.webkit.org/changeset/50757>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.