[Webkit-unassigned] [Bug 61878] Smooth scrolling for Chromium

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=61878





--- Comment #14 from Vangelis Kokkevis <vangelis at chromium.org>  2011-06-02 19:15:06 PST ---
(From update of attachment 95843)
View in context: https://bugs.webkit.org/attachment.cgi?id=95843&action=review

The Settings design confuses me too.  What functionality are you going for? Wouldn't things be a lot simpler if ScrollAnimatorSettingsChromium was a class nested in ScrollAnimatorChrome?  Are you trying to expose the scroll animator settings so that they can be modified from outside this code (i.e. via a UI or flags)?

> Source/WebCore/page/FrameView.cpp:2248
> +        return m_page->settings()->scrollAnimatorSettings();

I'm missing the point here.  Why go through the FrameView to get settings that are global to the Page?

>> Source/WebCore/platform/ScrollAnimatorSettings.h:44
>> +};
> 
> This seems like way overkill.  Is this supposed to be a client?  This isn't how we do settings.

I agree with Adam.  Even if you do end up with a separate ScrolAnimatorSettings class, I think the enable/disable still fits as a boolean in WebCore::Settings .

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:382
> +    const ScrollAnimatorSettingsChromium::Settings *input_settings;

input_settings -> inputSettings

> Source/WebCore/platform/chromium/ScrollAnimatorSettingsChromium.cpp:56
> +    for (int i = 0; i < 5; ++i)

Better to augment the Type enum with a last value entry and replace the 5 here by the entry's name.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.