| Summary: | Chromium Mac: Fork ScrollAnimatorMac to ScrollAnimatorChromiumMac for overlay scrollbar support | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sailesh Agrawal <sail> | ||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | bdakin, commit-queue, dglazkov, jamesr, mihaip, morrita, tony, webkit.review.bot | ||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
| Hardware: | Other | ||||||||||||||||||||||||
| OS: | OS X 10.5 | ||||||||||||||||||||||||
| Bug Depends on: | 61347 | ||||||||||||||||||||||||
| Bug Blocks: | 59728 | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Sailesh Agrawal
2011-04-28 15:56:28 PDT
Created attachment 91578 [details]
Patch
Please mark the patch review? when you want it reviewed. Created attachment 91589 [details]
Fixed Objective-C symbol clash error.
Comment on attachment 91589 [details] Fixed Objective-C symbol clash error. View in context: https://bugs.webkit.org/attachment.cgi?id=91589&action=review The changes to ScrollbarOverlayUtilitiesMac.h/mm appear to be changes from WebKit style braces to non WebKit style braces, except for one typo. I'd suggest not changing those files at all. Why is the include guard on ScrollAnimatorMac changed? Do we want to use ScrollAnimator on chromium mac even if ENABLE(SMOOTH_SCROLLING) is off? That seems wrong, why not just turn SMOOTH_SCROLLING on if that's the behavior we want? Is there code in ScrollAnimatorMac.mm currently that is not related to the SMOOTH_SCROLLING enable that should be refactored? It seems you have some unintended changes in ScrollAnimatorMac.mm (like reodering the elasticDelta/reboundDelta functions for no obvious reason). > Source/WebCore/ChangeLog:10 > + No new tests. (OOPS!) you can't do this, it will fail a presubmit check. replace this with some text explaining why this patch does not have layout tests. Created attachment 91598 [details]
Fixed patch to not include changes from another branch.
Comment on attachment 91589 [details] Fixed Objective-C symbol clash error. View in context: https://bugs.webkit.org/attachment.cgi?id=91589&action=review >> Source/WebCore/ChangeLog:10 >> + No new tests. (OOPS!) > > you can't do this, it will fail a presubmit check. replace this with some text explaining why this patch does not have layout tests. Fixed. > The changes to ScrollbarOverlayUtilitiesMac.h/mm appear to be changes from WebKit style braces to non WebKit style braces, except for one typo. I'd suggest not changing those files at all. Ooops, sorry. Forgot to rebase my branch. Fixed. > Why is the include guard on ScrollAnimatorMac changed? Do we want to use ScrollAnimator on chromium mac even if ENABLE(SMOOTH_SCROLLING) is off? That seems wrong, why not just turn SMOOTH_SCROLLING on if that's the behavior we want? Is there code in ScrollAnimatorMac.mm currently that is not related to the SMOOTH_SCROLLING enable that should be refactored? So I think enabling SMOOTH_SCROLLING should be separate from enabling overlay scrollbar. As far as I can tell there's no reason we have to have smooth scrolling to use ScrollAnimatorMac.mm > It seems you have some unintended changes in ScrollAnimatorMac.mm (like reodering the elasticDelta/reboundDelta functions for no obvious reason). Fixed. Shouldn't have been in the patch. Attachment 91598 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8520542 Created attachment 91728 [details]
Fixed some bugs
Comment on attachment 91728 [details]
Fixed some bugs
Oops, this patch has some problems. Removing.
Created attachment 91733 [details]
Fixed change log
Comment on attachment 91733 [details] Fixed change log View in context: https://bugs.webkit.org/attachment.cgi?id=91733&action=review > Source/WebCore/platform/mac/ScrollAnimatorMac.h:29 > +#if ENABLE(SMOOTH_SCROLLING) || (PLATFORM(CHROMIUM) && OS(DARWIN)) Why doesn't Chromium on Mac just enable SMOOTH_SCROLLING if you want to take this code path. Adding additional defines defeats the purpose of have fine grained enabled macros. > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:54 > +static NSPoint IntPointToNSPoint(WebCore::IntPoint point) { > + return NSMakePoint(point.x(), point.y()); > +} > + > +static WebCore::IntPoint NSPointToIntPoint(NSPoint point) { > + return WebCore::IntPoint(point.x, point.y); > +} { needs to be on the next line. Function can't start with a capital. > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:221 > + return IntPointToNSPoint(_animator->scrollableArea()->currentMousePosition()); Why is this necessary, IntPoint has a conversion to NSPoint. > Why doesn't Chromium on Mac just enable SMOOTH_SCROLLING if you want to take this code path. Adding additional defines defeats the purpose of have fine grained enabled macros. I'm not sure where we are when it comes to implementing smooth scrolling. I'll ask around but it would be nice to decouple these two features. http:://crbug.com/61140 tracks the smooth scrolling work. I was talking to jamesr and he recommended refactoring ScrollAnimatorMac.mm so that the overlay code was in a separate file. What do you think about that idea? > Why is this necessary, IntPoint has a conversion to NSPoint. Will fix. (In reply to comment #13) > I was talking to jamesr and he recommended refactoring ScrollAnimatorMac.mm so that the overlay code was in a separate file. What do you think about that idea? > Right now there are a lot of delegates implemented in ScrollAnimatorMac.mm, and I think it would be reasonable to move those into a different file. However, I definitely do not think that all of the overlay-related code should be moved to a different file. It's very much relevant to the animator, and I think the animator should continue to hold member variables that point to the delegates whether the delegates are implemented elsewhere or not. Without having a ton of knowledge about this code, I think it should be possible to turn overlays on without enabling the scroll animator (or at least the bits associated with smooth scrolling). I think it's reasonable that the smooth scrolling logic here depend on overlay scrollbars being enabled. > However, I definitely do not think that all of the overlay-related code should be moved to a different file. It's very much relevant to the animator, and I think the animator should continue to hold member variables that point to the delegates whether the delegates are implemented elsewhere or not.
Hi Beth and Sam. It sounds like the best thing would be to temporarily fork ScrollAnimatorMac to ScrollAnimatorChromiumMac. I'll file a bug on myself to merge it back when smooth scrolling is enabled for Chromium (Peter is working on this right now).
Does this seem reasonable?
Created attachment 93526 [details]
Patch
Hi all, this latest patch addresses review comments and goes back to forking ScrollAnimatorMac. I think this is much cleaner than using #ifdefs everywhere. This feature needs to make it into the next version fo Google Chrome so I'd appreciate any help getting change moving forward. I'll file a bug to track the work to merge it back to ScrollAnimatorMac once smooth scrolling is enabled for Google Chrome Mac. Thanks! How about to make ScrollAnimatorChromiumMac a subclass of ScrollAnimatorMac and factor out chromium specific parts into a method and override it? Forked code is really hard to maintain and I hope we didn't have more.... (In reply to comment #19) > How about to make ScrollAnimatorChromiumMac a subclass of ScrollAnimatorMac > and factor out chromium specific parts into a method and override it? > Forked code is really hard to maintain and I hope we didn't have more.... Currently ScrollAnimatorMac is guarded by an #if ENABLE(SMOOTH_SCROLLING). To subclass it I would have to change that to: #if ENABLE(SMOOTH_SCROLLING) || (PLATFORM(CHROMIUM) && OS(DARWIN)) As per comment 12 we want to avoid that. At this point the lack of smooth scrolling is the only real difference between the Mac and Chrome version. Once we enable smooth scrolling I wouldn't even need a subclass. Ah I see. My comment was pointless... I'm sorry for the interruption. Sailesh and I talked about this on IRC. We agreed that this is okay for a temporary solution, but a bug should be filed to un-fork asap. Filed bug 61144 to track the work to merge ScrollAnimatorChromiumMac.mm back to ScrollAnimatorMac. Beth and Sam, could I commit this change? Thanks! (In reply to comment #23) > Filed bug 61144 to track the work to merge ScrollAnimatorChromiumMac.mm back to ScrollAnimatorMac. > > Beth and Sam, could I commit this change? > > Thanks! Yes, you can. You should get a Chromium person to r+ since in the end this patch will only affect Chromium. But yes, you can commit it. Created attachment 94473 [details]
Patch
Removed some unnecessary changes to ScrollAnimatorChromiumMac.mm. At this point the only changes to the forked code is renaming ScrollAnimatorMac to ScrollAnimatorChromiumMac and renaming ScrollbarThemeMac to ScrollbarThemeChromiumMac. Added this to the ChangeLog. James could you take another look? Thanks! Comment on attachment 94473 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94473&action=review Seems OK > Source/WebCore/platform/chromium/ScrollAnimatorChromiumMac.h:55 > + ScrollAnimatorChromiumMac(ScrollableArea*); should be explicit > Source/WebCore/platform/graphics/IntPoint.h:40 > +#if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && OS(DARWIN)) should this just be #if OS(DARWIN)? > Source/WebCore/platform/graphics/IntPoint.h:122 > +#if (PLATFORM(MAC) || (PLATFORM(CHROMIUM) && OS(DARWIN))) && !defined(NSGEOMETRY_TYPES_SAME_AS_CGGEOMETRY_TYPES) should this just be #if OS(DARWIN) && !defined(NSGEOMETRY_TYPES_SAME_AS_CGGEOMETRY_TYPES) ? Created attachment 94485 [details]
Patch
Comment on attachment 94473 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94473&action=review >> Source/WebCore/platform/chromium/ScrollAnimatorChromiumMac.h:55 >> + ScrollAnimatorChromiumMac(ScrollableArea*); > > should be explicit Done. >> Source/WebCore/platform/graphics/IntPoint.h:40 >> +#if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && OS(DARWIN)) > > should this just be #if OS(DARWIN)? Done. >> Source/WebCore/platform/graphics/IntPoint.h:122 >> +#if (PLATFORM(MAC) || (PLATFORM(CHROMIUM) && OS(DARWIN))) && !defined(NSGEOMETRY_TYPES_SAME_AS_CGGEOMETRY_TYPES) > > should this just be #if OS(DARWIN) && !defined(NSGEOMETRY_TYPES_SAME_AS_CGGEOMETRY_TYPES) ? Done. Comment on attachment 94485 [details] Patch Clearing flags on attachment: 94485 Committed r87118: <http://trac.webkit.org/changeset/87118> All reviewed patches have been landed. Closing bug. Re-opening bug since my change got backed out. Uploading a new patch in a second. Created attachment 95034 [details]
Patch
This is a new patch that passes layout tests. See: http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds/194 http://build.chromium.org/p/tryserver.chromium/builders/mac_layout/builds/633 For comparison here's the output from the try bot with an empty change list: http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds/191 http://build.chromium.org/p/tryserver.chromium/builders/mac_layout/builds/631 The differences between this patch and the one that got reverted are: - disable smooth scrolling by adding and #ifdef in ScrollAnimatorChromiumMac::scroll() - adding IntPointMac.mm. This is used by some ScrollAnimatorChromiuMac code that's currently #ifdef'ed out. - removed exceptions from test_expectations.txt that were added when my change was reverted I'm running browser tests and ui_tests locally and will update this bug with the results. Thanks. > I'm running browser tests and ui_tests locally and will update this bug with the results. Thanks.
browser_tests passed. There were some errors in the ui_tests but I don't think they're related to my change.
Comment on attachment 95034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95034&action=review > Source/WebCore/platform/chromium/ScrollAnimatorChromiumMac.mm:926 > + m_haveScrolledSincePageLoad = true; Nit: Is this necessary? smoothScrollWithEvent is only called by handleWheelEvent, and you already set m_haveScrolledSincePageLoad to true there. Comment on attachment 95034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95034&action=review >> Source/WebCore/platform/chromium/ScrollAnimatorChromiumMac.mm:926 >> + m_haveScrolledSincePageLoad = true; > > Nit: Is this necessary? smoothScrollWithEvent is only called by handleWheelEvent, and you already set m_haveScrolledSincePageLoad to true there. This is all copy paste from the platform/mac/ScrollAnimatorMac.mm code. My goal is to keep the two files as similar as possible so that I can re-merge them once we have smooth scrolling on chromium mac. Comment on attachment 95034 [details] Patch Rejecting attachment 95034 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2 Last 500 characters of output: (offset -4 lines). patching file Source/WebCore/platform/ScrollAnimator.cpp patching file Source/WebCore/platform/chromium/ScrollAnimatorChromiumMac.h patching file Source/WebCore/platform/chromium/ScrollAnimatorChromiumMac.mm patching file Source/WebCore/platform/graphics/IntPoint.h Hunk #1 succeeded at 38 (offset 1 line). Hunk #2 succeeded at 126 (offset 6 lines). Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Mihai Parparita', u'--..." exit_code: 1 Full output: http://queues.webkit.org/results/8811198 Created attachment 96292 [details]
Patch
Comment on attachment 96292 [details] Patch Clearing flags on attachment: 96292 Committed r88286: <http://trac.webkit.org/changeset/88286> All reviewed patches have been landed. Closing bug. |