Bug 31281 - r50665 broke around 100 Qt layout tests
Summary: r50665 broke around 100 Qt layout tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P1 Normal
Assignee: Nobody
URL:
Keywords: LayoutTestFailure, Qt, Regression
Depends on:
Blocks:
 
Reported: 2009-11-09 18:01 PST by Kenneth Rohde Christiansen
Modified: 2009-11-10 13:13 PST (History)
7 users (show)

See Also:


Attachments
proposed patch (1004 bytes, patch)
2009-11-10 06:46 PST, Csaba Osztrogonác
kenneth: review-
Details | Formatted Diff | Diff
proposed patch for removing policy settings from DRT (1.28 KB, patch)
2009-11-10 10:11 PST, Andras Becsi
abecsi: review-
abecsi: commit-queue-
Details | Formatted Diff | Diff
proposed fix for DRT and the test expected files (8.81 KB, patch)
2009-11-10 11:26 PST, Andras Becsi
ossy: review-
ossy: commit-queue-
Details | Formatted Diff | Diff
another try of creating a correct patch (1.06 MB, patch)
2009-11-10 11:59 PST, Andras Becsi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Rohde Christiansen 2009-11-09 18:01:55 PST
Judging from the Qt buildbot, r50665 (https://bugs.webkit.org/show_bug.cgi?id=29167) broke around 100 Qt layout tests.
Comment 1 Mark Mentovai 2009-11-09 18:31:32 PST
Ossy and I were looking at this earlier.
Comment 2 Csaba Osztrogonác 2009-11-10 06:46:10 PST
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?
Comment 3 Kenneth Rohde Christiansen 2009-11-10 06:55:32 PST
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.
Comment 4 Csaba Osztrogonác 2009-11-10 07:18:47 PST
(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.
Comment 5 Mark Mentovai 2009-11-10 07:27:25 PST
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?
Comment 6 Andras Becsi 2009-11-10 07:43:38 PST
(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?
Comment 7 Andras Becsi 2009-11-10 08:10:33 PST
(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.
Comment 8 Csaba Osztrogonác 2009-11-10 08:59:10 PST
> 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?
Comment 9 Kenneth Rohde Christiansen 2009-11-10 09:17:40 PST
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.
Comment 10 Kenneth Rohde Christiansen 2009-11-10 09:19:15 PST
Comment on attachment 42860 [details]
proposed patch

Wrong fix.
Comment 11 Andras Becsi 2009-11-10 10:11:15 PST
Created attachment 42872 [details]
proposed patch for removing policy settings from DRT
Comment 12 Andras Becsi 2009-11-10 10:31:11 PST
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.
Comment 13 Andras Becsi 2009-11-10 11:26:32 PST
Created attachment 42881 [details]
proposed fix for DRT and the test expected files
Comment 14 Csaba Osztrogonác 2009-11-10 11:48:45 PST
Comment on attachment 42881 [details]
proposed fix for DRT and the test expected files

You forgot to add modified expected files.
Comment 15 Andras Becsi 2009-11-10 11:59:32 PST
Created attachment 42884 [details]
another try of creating a correct patch

Thanks Ossy.
Comment 16 WebKit Commit Bot 2009-11-10 13:13:15 PST
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>
Comment 17 WebKit Commit Bot 2009-11-10 13:13:22 PST
All reviewed patches have been landed.  Closing bug.