Summary: | [Qt] REGRESSION:(r50665) QWebFrame::setScrollBarPolicy(Qt::Vertical,Qt::ScrollBarAlwaysOff) has no effect. | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tor Arne Vestbø <vestbo> | ||||||||||||||||||||
Component: | WebKit Qt | Assignee: | Antonio Gomes <tonikitoo> | ||||||||||||||||||||
Status: | CLOSED FIXED | ||||||||||||||||||||||
Severity: | Major | CC: | alice.barraclough, benjamin, eric, gustavo, hausmann, hyatt, ismail, jesus, jturcotte, kenneth, luiz, mark, tonikitoo, webkit.review.bot, xan.lopez | ||||||||||||||||||||
Priority: | P1 | Keywords: | Qt | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Other | ||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||
Bug Blocks: | 35784 | ||||||||||||||||||||||
Attachments: |
|
Description
Tor Arne Vestbø
2009-09-18 07:45:20 PDT
I can reproduce this with latest qtwebkit 4.6 snapshot. raising the importance, since it regresses a previously working API-related code. I can investigate ... (In reply to comment #2) > raising the importance, since it regresses a previously working API-related > code. > > I can investigate ... just found the guilt commit (by git bisect'ing 480 commits) ... what broke it was b0444cef4e8bb4265a2b6d67e1f7d891680ec082 Please write revision instead. It seems to be r50665 void FrameView::layout(bool allowSubtree) { (...) if (!m_canHaveScrollbars) { hMode = ScrollbarAlwaysOff; vMode = ScrollbarAlwaysOff; } else { scrollbarModes(hMode, vMode); if (hMode == ScrollbarAlwaysOff) hMode = ScrollbarAuto; if (vMode == ScrollbarAlwaysOff) vMode = ScrollbarAuto; } ... this else block broke us Created attachment 52071 [details]
WIP - it works but is not to be reviewed or landed
adding alice (Apple) and mark (chromium) who were the reporters of the regressions that ended up causing this problem, as well as hyatt the reviewer of one of them. Created attachment 52325 [details]
patch_1 - v1 - adds lock scrollbars concept to ScrollView
Patch introduces a lock scrollbars concept to ScrollView, as in WebDynamicScrollBarsView.mm/h on WebKit/mac. It is needed because in QtWebKit, we have Api for setting both vertical and horizontal scrollbars on/off/auto. When it is set to off, for example, it should remain as such, unless unset.
@hyatt, do you think this approach is feasible?
Attachment 52325 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/1572150 Comment on attachment 52325 [details]
patch_1 - v1 - adds lock scrollbars concept to ScrollView
I would not alter any of the mode-setting functions. Just add the lock functions separately. The mode-setting functions can of course check the locks, but I don't think you should mix mode setting and locking in the same functions.
Created attachment 52517 [details]
patch 1 - v2 - addressed feedback from hyatt on IRC
In IRC, it was agreed w/ hyatt to remove the optional 'lock' parameter from ScrollView::setScrollingModes, since both set{Horizontal,Vertical}ScrollingMode methods have the 'lock' parameter, which is enough to fixing the issue. The former *has* honors the current lock state though.
It was also agreed Frame::createView could get two additional parameters for vertical scrollbar lock and horizontal scrollbar lock respectively, to persist the current lock state cross page loads.
patch 2 addresses these.
Created attachment 52567 [details]
patch_1 - v2.1 - same as "patch_1 - v2" rebased with ToT
Created attachment 52568 [details] (committed r57278) patch_2 - v1 - add stubs for Mac, Gtk, Wx and Win DRTs to implement this method @hyatt, could you help me w/ this two (complementary) patches? Created attachment 52641 [details]
patch_1 - v2.2 - attempt to build on Mac (same as "patch_1 - v2.1")
Comment on attachment 52641 [details]
patch_1 - v2.2 - attempt to build on Mac (same as "patch_1 - v2.1")
(1) For the createView function, having two calls to set scrollbar modes is problematic when each one results in an updateScrollers call underneath, e.g., on Mac. You could just do:
frameView->setHorizontalLock(horizontalLock);
frameView->setVerticalLock(verticalLock);
frameView->setScrollbarModes(horizontalScrollbarMode, verticalScrollbarMode);
(2) Dump the lock argument from setHorizontalScrollbarMode and setVerticalScrollbarMode. It's just inconsistent with setScrollbarModes, and you've provided locking functions anyway (setHorizontalLock and setVerticalLock).
(3) in qwebframe.cpp just do the lock call before setting the mode, e.g.,
d->frame->view()->setHorizontalLock(policy != Qt::ScrollBarAsNeeded);
d->frame->view()->setHorizontalScrollbarMode((ScrollbarMode)policy);
It's slightly more verbose, but I think it's nice to separate.
(In reply to comment #16) > (From update of attachment 52641 [details]) > (1) For the createView function, having two calls to set scrollbar modes is > problematic when each one results in an updateScrollers call underneath, e.g., > on Mac. You could just do: > > frameView->setHorizontalLock(horizontalLock); > frameView->setVerticalLock(verticalLock); > frameView->setScrollbarModes(horizontalScrollbarMode, verticalScrollbarMode); > > (2) Dump the lock argument from setHorizontalScrollbarMode and > setVerticalScrollbarMode. It's just inconsistent with setScrollbarModes, and > you've provided locking functions anyway (setHorizontalLock and > setVerticalLock). > > (3) in qwebframe.cpp just do the lock call before setting the mode, e.g., > > d->frame->view()->setHorizontalLock(policy != Qt::ScrollBarAsNeeded); > d->frame->view()->setHorizontalScrollbarMode((ScrollbarMode)policy); > > It's slightly more verbose, but I think it's nice to separate. ... and discussion continued on IRC: <tonikitoo> dhyatt, you suggested doing: <tonikitoo> "setHorizontalLock(hLock); setVerticalLock(vLock); setScrollbarModes(...)" <tonikitoo> but if setScrollbarModes honors the lock, it wont change to the policy you are setting, because it is already locked. <dhyatt> put them after then? <dhyatt> setScrollbarModes(....); setHorizontalLock(hLock); setVerticalLock(vLock); ? <tonikitoo> dhyatt, after can be also tricky, because a FrameView::layout() is called during setScrollbarModes, and layout() can change it <tonikitoo> I've tested these possibilities <tonikitoo> dhyatt, suppose we are calling lock after, then setScrollbarModes which calls updateScrollbars, and endup calling FrameView::layout , where it is scrollbar mode/policy unset, and then you lock, it would not work =/ <dhyatt> tonikitoo: yeah true <dhyatt> i guess if you have to lock at the same time you could leave it the way it is <dhyatt> ok, i guess the lock has to be part of the cal llike you did it then <dhyatt> setScrollbarModes should take the locks too then <dhyatt> for consistency with the setHorizontal and setVertical methods and so you can do the one call in createView that is the way I'll do it Created attachment 52736 [details]
patch_1 - v3 - addressed discussion with dhyatt on IRC
As accorded on IRC, this patch:
* adds additional lock parameters to setScrollbarModes, setVerticalScrollbarMode and setHorizontalScrollbarMode of ScrollView;
* make the latter two just call the former passing the appropriated parameters, as originally;
* adds two optional lock parameters to Frame::createView, for locking horizontal and vertical scrollbars, respectively.
* adds a layout test and a qt API unit test
* make the adjusts needed in FrameLoaderClientQt, to use the new createView parameters.
Created attachment 52737 [details] patch_1 - v3.1 - addressed discussion with dhyatt on IRC + rebased to ToT see comment #18 Attachment 52737 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/ScrollView.h:88: More than one command on the same line [whitespace/newline] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebframe.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/tests/qwebframe/tst_qwebframe.cpp"
Total errors found: 1 in 19 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 52737 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/1654226 Created attachment 52745 [details] (committed r57277) patch_1 - v3.2 - addressed discussion with dhyatt on IRC + rebased to ToT + builds in gtk see comment #18 for summary @dhyatt, could you have a look? Attachment 52745 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/ScrollView.h:88: More than one command on the same line [whitespace/newline] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebframe.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/tests/qwebframe/tst_qwebframe.cpp"
Total errors found: 1 in 20 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 52745 [details] (committed r57277) patch_1 - v3.2 - addressed discussion with dhyatt on IRC + rebased to ToT + builds in gtk r=me Comment on attachment 52745 [details] (committed r57277) patch_1 - v3.2 - addressed discussion with dhyatt on IRC + rebased to ToT + builds in gtk thank you dhyatt! Comment on attachment 52568 [details] (committed r57278) patch_2 - v1 - add stubs for Mac, Gtk, Wx and Win DRTs to implement this method thank you tronical/simon/hausmann! <cherry-pick-for-backport: 57277> <cherry-pick-for-backport: 57278> |