Bug 137939 - Remove ScrollAnimator::create factory function
Summary: Remove ScrollAnimator::create factory function
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-21 18:16 PDT by Gyuyoung Kim
Modified: 2018-08-31 07:20 PDT (History)
2 users (show)

See Also:

Attachments
Patch (6.28 KB, patch)
2014-10-21 18:16 PDT, Gyuyoung Kim 		no flags Details | Formatted Diff | Diff
Patch (7.03 KB, patch)
2014-10-22 21:41 PDT, Gyuyoung Kim 		no flags Details | Formatted Diff | Diff
Patch for ews (7.09 KB, patch)
2014-10-22 23:05 PDT, Gyuyoung Kim 		no flags Details | Formatted Diff | Diff
Patch (7.25 KB, patch)
2014-10-23 02:59 PDT, Gyuyoung Kim 		no flags Details | Formatted Diff | Diff
Patch (7.26 KB, patch)
2014-10-23 04:05 PDT, Gyuyoung Kim 		no flags Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2014-10-21 18:16:02 PDT 
SSIA
Comment 1 Gyuyoung Kim 2014-10-21 18:16:53 PDT 
Created attachment 240234 [details]
Patch
Comment 2 Darin Adler 2014-10-22 09:47:53 PDT 
Comment on attachment 240234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240234&action=review

> Source/WebCore/platform/ScrollableArea.cpp:76
> +        m_scrollAnimator = std::make_unique<ScrollAnimator>(const_cast<ScrollableArea*>(this));

This doesn’t do the same thing the old code did. Why is it OK to change behavior and make an object of a different class than before in the case of Mac, iOS, and when scrollAnimatorEnabled is false?
Comment 3 Gyuyoung Kim 2014-10-22 19:22:53 PDT 
Comment on attachment 240234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240234&action=review

>> Source/WebCore/platform/ScrollableArea.cpp:76
>> +        m_scrollAnimator = std::make_unique<ScrollAnimator>(const_cast<ScrollableArea*>(this));
> 
> This doesn’t do the same thing the old code did. Why is it OK to change behavior and make an object of a different class than before in the case of Mac, iOS, and when scrollAnimatorEnabled is false?

Oops, I really really missed it ! Exactly right. Let me fix this patch again !.
Comment 4 Gyuyoung Kim 2014-10-22 21:41:10 PDT 
Created attachment 240326 [details]
Patch
Comment 5 Gyuyoung Kim 2014-10-22 21:43:41 PDT 
Comment on attachment 240326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240326&action=review

> Source/WebCore/platform/ScrollableArea.cpp:80
> +#if PLATFORM(IOS)

I don't like to create new instance based on #if ~ #endif though, I don't know better solution except for create() factory function. Is there better approach ?
Comment 6 Gyuyoung Kim 2014-10-22 23:05:56 PDT 
Created attachment 240332 [details]
Patch for ews
Comment 7 Gyuyoung Kim 2014-10-23 02:59:19 PDT 
Created attachment 240338 [details]
Patch
Comment 8 Gyuyoung Kim 2014-10-23 04:05:07 PDT 
Created attachment 240342 [details]
Patch
Comment 9 Gyuyoung Kim 2014-10-23 17:01:34 PDT 
Comment on attachment 240342 [details]
Patch

Darin, what do you think about this change ?
Comment 10 Gyuyoung Kim 2014-10-28 17:49:39 PDT 
CC'ing Darin. I wonder what do you think about this patch.
Comment 11 Gyuyoung Kim 2014-10-28 20:37:46 PDT 
Comment on attachment 240342 [details]
Patch

Need to be modified after bug 137765