Summary: | Cleanup Scrollbar/ScrollbarClient relationship | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||||||||||
Component: | Platform | Assignee: | Sam Weinig <sam> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, bdakin, buildbot, dglazkov, eric, gustavo, pkasting, webkit-ews, webkit.review.bot, xan.lopez | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | PC | ||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||
Attachments: |
|
Description
Sam Weinig
2011-01-19 20:26:40 PST
Created attachment 79546 [details]
WIP (Do not review)
Uploading WIP patch to see how the bots feel about it.
Attachment 79546 [details] did not build on qt: Build output: http://queues.webkit.org/results/7539233 Attachment 79546 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7516192 Attachment 79546 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7568218 Created attachment 79549 [details]
WIP-2 (Do not review)
Attachment 79549 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7610233 Attachment 79549 [details] did not build on qt: Build output: http://queues.webkit.org/results/7523238 If you're just looking for ews results you can remove the review flag as soon as numbers appear in the bubbles. you can also submit patches directly using http://queues.webkit.org/submit-to-ews (In reply to comment #8) > If you're just looking for ews results you can remove the review flag as soon as numbers appear in the bubbles. > > you can also submit patches directly using http://queues.webkit.org/submit-to-ews Cool! Created attachment 79621 [details]
Attempt 3
Attachment 79621 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7609231 Created attachment 79636 [details]
Attempt 4
Created attachment 79640 [details]
Patch
Created attachment 79641 [details]
Patch
Comment on attachment 79641 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79641&action=review Looks good. > Source/WebCore/platform/Scrollbar.cpp:310 > + if (m_orientation == HorizontalScrollbar) > + client()->scrollToXOffsetWithoutAnimation(m_dragOrigin); > + else > + client()->scrollToYOffsetWithoutAnimation(m_dragOrigin); I see this pattern in multiple places. Maybe there should be a scrollToOffsetWithoutAnimation(ScrollbarOrientation, int offset) function. > Source/WebCore/platform/efl/ScrollbarEfl.cpp:91 > + if (that->orientation() == HorizontalScrollbar) > + that->client()->scrollToXOffsetWithoutAnimation(v); > + else > + that->client()->scrollToYOffsetWithoutAnimation(v); > } See comment above about this pattern. > Source/WebCore/platform/gtk/MainFrameScrollbarGtk.cpp:115 > + if (that->orientation() == HorizontalScrollbar) > + that->client()->scrollToXOffsetWithoutAnimation(static_cast<int>(gtk_adjustment_get_value(that->m_adjustment.get()))); > + else > + that->client()->scrollToYOffsetWithoutAnimation(static_cast<int>(gtk_adjustment_get_value(that->m_adjustment.get()))); And again. Especially gross given the big gtk_adjustment_get_value expression is repeated twice. > Source/WebKit/qt/Api/qwebframe.cpp:1078 > + if (orientation == Qt::Horizontal) > + sb->client()->scrollToXOffsetWithoutAnimation(value); > + else > + sb->client()->scrollToYOffsetWithoutAnimation(value); And again. http://trac.webkit.org/changeset/76291 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, Qt Linux ARMv7 Release, Qt Windows 32-bit Release, and Qt Windows 32-bit Debug Sam, do you want to look at bug 45005, and * Dupe that against this if relevant * See if there were any useful details on that bug you haven't done here I haven't looked to see what you've done on this bug but from a high level it sounds very much like what that bug was about. *** Bug 45005 has been marked as a duplicate of this bug. *** Created attachment 79769 [details]
Part 2
Attachment 79769 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/platform/ScrollView.h:32: Alphabetical sorting problem. [build/include_order] [4]
Source/WebKit/win/WebScrollBar.h:37: Alphabetical sorting problem. [build/include_order] [4]
Source/WebCore/platform/ScrollbarThemeComposite.cpp:37: Alphabetical sorting problem. [build/include_order] [4]
Source/WebCore/platform/Scrollbar.h:46: The parameter name "orientation" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/platform/Scrollbar.h:46: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 5 in 59 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 79769 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7569263 Forgot to mention, part 1 landed in http://trac.webkit.org/changeset/76291. Comment on attachment 79769 [details] Part 2 Part 2 landed in http://trac.webkit.org/changeset/76378. Attachment 79769 [details] did not build on win: Build output: http://queues.webkit.org/results/7570267 http://trac.webkit.org/changeset/76378 might have broken Chromium Linux Release Marking fixed for now. Additional cleanups will come later. |