Bug 137939

Summary: Remove ScrollAnimator::create factory function
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: WebCore Misc.Assignee: Gyuyoung Kim <gyuyoung.kim>
Status: NEW    
Severity: Normal CC: darin, fred.wang
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for ews
none
Patch
none
Patch none

Gyuyoung Kim
Reported 2014-10-21 18:16:02 PDT
SSIA
Attachments
Patch (6.28 KB, patch)
2014-10-21 18:16 PDT, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch (7.03 KB, patch)
2014-10-22 21:41 PDT, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch for ews (7.09 KB, patch)
2014-10-22 23:05 PDT, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch (7.25 KB, patch)
2014-10-23 02:59 PDT, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Patch (7.26 KB, patch)
2014-10-23 04:05 PDT, Gyuyoung Kim 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Gyuyoung Kim
Comment 1 2014-10-21 18:16:53 PDT
Created attachment 240234 [details] Patch
Darin Adler
Comment 2 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?
Gyuyoung Kim
Comment 3 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 !.
Gyuyoung Kim
Comment 4 2014-10-22 21:41:10 PDT
Created attachment 240326 [details] Patch
Gyuyoung Kim
Comment 5 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 ?
Gyuyoung Kim
Comment 6 2014-10-22 23:05:56 PDT
Created attachment 240332 [details] Patch for ews
Gyuyoung Kim
Comment 7 2014-10-23 02:59:19 PDT
Created attachment 240338 [details] Patch
Gyuyoung Kim
Comment 8 2014-10-23 04:05:07 PDT
Created attachment 240342 [details] Patch
Gyuyoung Kim
Comment 9 2014-10-23 17:01:34 PDT
Comment on attachment 240342 [details] Patch Darin, what do you think about this change ?
Gyuyoung Kim
Comment 10 2014-10-28 17:49:39 PDT
CC'ing Darin. I wonder what do you think about this patch.
Gyuyoung Kim
Comment 11 2014-10-28 20:37:46 PDT
Comment on attachment 240342 [details] Patch Need to be modified after bug 137765
Note You need to log in before you can comment on or make changes to this bug.