Summary: | r50665 broke around 100 Qt layout tests | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kenneth Rohde Christiansen <kenneth> | ||||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abecsi, commit-queue, hausmann, manyoso, mark, mitz, ossy | ||||||||||
Priority: | P1 | Keywords: | LayoutTestFailure, Qt, Regression | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Attachments: |
|
Description
Kenneth Rohde Christiansen
2009-11-09 18:01:55 PST
Ossy and I were looking at this earlier. 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?
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. (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. 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? (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? (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. > 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?
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 on attachment 42860 [details]
proposed patch
Wrong fix.
Created attachment 42872 [details]
proposed patch for removing policy settings from DRT
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.
Created attachment 42881 [details]
proposed fix for DRT and the test expected files
Comment on attachment 42881 [details]
proposed fix for DRT and the test expected files
You forgot to add modified expected files.
Created attachment 42884 [details]
another try of creating a correct patch
Thanks Ossy.
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> All reviewed patches have been landed. Closing bug. |