[chromium] Add CCTimer class for the compositor
Created attachment 119702 [details] Patch
Attachment 119702 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit/chromium/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Source/WebKit/chromium/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Total errors found: 4 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 119702 [details] Patch Can you fix the tab issue? This appears to be copy-pasting a fair amount of boilerplate, not all of which is really relevant for our use cases. For example I don't think we have any need for repeating timers or for passing a CCTimer* to the timer fired callback. Let's start with the API that we need and the implementation we need to support that interface alone.
Created attachment 119956 [details] Patch
Comment on attachment 119956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119956&action=review > Source/WebCore/platform/graphics/chromium/cc/CCTimer.h:78 > +class CCTimer : public CCTimerBase { > +public: > + CCTimer(CCThread* thread, CCTimerClient* client = 0) : CCTimerBase(thread), m_client(client) { } why do we need the base class here? we only have one subclass
Created attachment 119963 [details] Patch
Comment on attachment 119963 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119963&action=review > Source/WebCore/platform/graphics/chromium/cc/CCTimer.h:35 > +#include <cfloat> what is cfloat for? does it need to be included in this header? > Source/WebCore/platform/graphics/chromium/cc/CCTimer.h:48 > + CCTimer(CCThread* thread, CCTimerClient* client) : m_client(client), m_thread(thread), m_task(0) { } please expand the initializer list to one statement per line to keep this header smaller and more readable could you move the implementations of the c'tor/d'tor into the .cpp? I don't expect this to be performance-critical code > Source/WebCore/platform/graphics/chromium/cc/CCTimer.h:51 > + void startOneShot(double interval); is this seconds, milliseconds, microseconds, something else? we try to tag parameter names that represent periods of time with a suffix to indicate the unit CCThread and CCDelayBasedTimeSource both have time values of milliseconds with a "Ms" suffix, can we stick with that? can you document in the header what this function does if there's already a timer pending (looks like it cancels the previous task, which i think is great behavior but it should be documented) > Source/WebCore/platform/graphics/chromium/cc/CCTimer.h:52 > + void stop() { deschedule(); } why are stop() and deschedule() different functions if they are just aliases of each other? Can you fold them into one function? > Source/WebCore/platform/graphics/chromium/cc/CCTimer.h:59 > + Task(CCTimer* timer) : CCThread::Task(0), m_timer(timer) { } explicit on this constructor, please can you expand the member initializations out to separate lines? we very rarely combine multiple statements to a single line in webkit
Independent of this patch, I think we should eradicate all millisecond-unit measures out of CC. That way we dont have to deal with this confusion. Thoughts?
That would work too, although we should still tag in that case.
Fine by me.
(In reply to comment #7) > (From update of attachment 119963 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=119963&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCTimer.h:35 > > +#include <cfloat> > > what is cfloat for? does it need to be included in this header? Removed. It is leftover from the shrink. > > Source/WebCore/platform/graphics/chromium/cc/CCTimer.h:48 > > + CCTimer(CCThread* thread, CCTimerClient* client) : m_client(client), m_thread(thread), m_task(0) { } > > please expand the initializer list to one statement per line > to keep this header smaller and more readable could you move the implementations of the c'tor/d'tor into the .cpp? I don't expect this to be performance-critical code Done > > Source/WebCore/platform/graphics/chromium/cc/CCTimer.h:51 > > + void startOneShot(double interval); > > is this seconds, milliseconds, microseconds, something else? we try to tag parameter names that represent periods of time with a suffix to indicate the unit > CCThread and CCDelayBasedTimeSource both have time values of milliseconds with a "Ms" suffix, can we stick with that? Done However I totally agree with Nat that the use of millisecond is a source of confusion. Why not just use seconds? > can you document in the header what this function does if there's already a timer pending (looks like it cancels the previous task, which i think is great behavior but it should be documented) Done > > Source/WebCore/platform/graphics/chromium/cc/CCTimer.h:52 > > + void stop() { deschedule(); } > > why are stop() and deschedule() different functions if they are just aliases of each other? Can you fold them into one function? Done But I'm a little bit reluctant to this. I designed them into 2 function because they have different semantics. deschedule() means to break the link between the task and the timer, where stop() is a public function that request the timer to cancel. deschedule() is a mean to cancel, but cancel does not necessarily need to be implemented this way. Anyway, I agree with you that keep things simple is good, so I'm fine with either way. > > Source/WebCore/platform/graphics/chromium/cc/CCTimer.h:59 > > + Task(CCTimer* timer) : CCThread::Task(0), m_timer(timer) { } > > explicit on this constructor, please > > can you expand the member initializations out to separate lines? we very rarely combine multiple statements to a single line in webkit Done
(In reply to comment #9) > That would work too, although we should still tag in that case. So other code will also migrate to use seconds eventually? Personally I prefer seconds much better than milliseconds, but I agree with you that things should be unified.
Created attachment 120234 [details] Patch
Attachment 120234 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource From git://git.webkit.org/WebKit 97efa88..723f8ee master -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 103468 = 97efa88a66ff7f597257cf86d0f8315cd016e1e3 r103470 = 723f8ee6c151d1039a19237711dc00f3284f27f8 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Applying: Inform the scrolling coordinator when scrollbar layers come and go Using index info to reconstruct a base tree... <stdin>:474806: trailing whitespace. [Chromium] DatabaseTrackerChromium: iterating DatabaseSet races with Database disposal on worker thread <stdin>:474827: trailing whitespace. Nothing to test, just removing redundant code. Correct behavior tested by <stdin>:475346: trailing whitespace. warning: 3 lines add whitespace errors. Falling back to patching base and 3-way merge... warning: too many files (created: 167519 deleted: 3), skipping inexact rename detection Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/chromium/test_expectations.txt CONFLICT (content): Merge conflict in LayoutTests/platform/chromium/test_expectations.txt Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Auto-merging Source/WebCore/page/ScrollingCoordinator.h CONFLICT (content): Merge conflict in Source/WebCore/page/ScrollingCoordinator.h Auto-merging Source/WebCore/page/mac/ScrollingCoordinatorMac.mm CONFLICT (content): Merge conflict in Source/WebCore/page/mac/ScrollingCoordinatorMac.mm Auto-merging Source/WebCore/platform/ScrollView.cpp Auto-merging Source/WebCore/rendering/RenderLayerCompositor.cpp CONFLICT (content): Merge conflict in Source/WebCore/rendering/RenderLayerCompositor.cpp Auto-merging Source/WebKit2/ChangeLog CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog Auto-merging Tools/ChangeLog CONFLICT (content): Merge conflict in Tools/ChangeLog Auto-merging Tools/Scripts/build-webkit Auto-merging Tools/Scripts/webkitdirs.pm CONFLICT (content): Merge conflict in Tools/Scripts/webkitdirs.pm Failed to merge in the changes. Patch failed at 0001 Inform the scrolling coordinator when scrollbar layers come and go When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 158. If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 120237 [details] Patch
Attachment 120237 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource From git://git.webkit.org/WebKit 710a358..eea810d master -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 103471 = 710a358c829343885bc1490e41fde775bbcdfd98 r103472 = 1044a8135bc559ae0c921f4d5fb016e6f39433e1 r103473 = eea810d79cfd732c04ef10cfc8da0e3793db3102 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Applying: Inform the scrolling coordinator when scrollbar layers come and go Using index info to reconstruct a base tree... <stdin>:474806: trailing whitespace. [Chromium] DatabaseTrackerChromium: iterating DatabaseSet races with Database disposal on worker thread <stdin>:474827: trailing whitespace. Nothing to test, just removing redundant code. Correct behavior tested by <stdin>:475346: trailing whitespace. warning: 3 lines add whitespace errors. Falling back to patching base and 3-way merge... warning: too many files (created: 167519 deleted: 3), skipping inexact rename detection Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/chromium/test_expectations.txt CONFLICT (content): Merge conflict in LayoutTests/platform/chromium/test_expectations.txt Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Auto-merging Source/WebCore/page/ScrollingCoordinator.h CONFLICT (content): Merge conflict in Source/WebCore/page/ScrollingCoordinator.h Auto-merging Source/WebCore/page/mac/ScrollingCoordinatorMac.mm CONFLICT (content): Merge conflict in Source/WebCore/page/mac/ScrollingCoordinatorMac.mm Auto-merging Source/WebCore/platform/ScrollView.cpp Auto-merging Source/WebCore/rendering/RenderLayerCompositor.cpp CONFLICT (content): Merge conflict in Source/WebCore/rendering/RenderLayerCompositor.cpp Auto-merging Source/WebKit2/ChangeLog CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog Auto-merging Tools/ChangeLog CONFLICT (content): Merge conflict in Tools/ChangeLog Auto-merging Tools/Scripts/build-webkit Auto-merging Tools/Scripts/webkitdirs.pm CONFLICT (content): Merge conflict in Tools/Scripts/webkitdirs.pm Failed to merge in the changes. Patch failed at 0001 Inform the scrolling coordinator when scrollbar layers come and go When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 158. If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 120237 [details] Patch I think the style bot is hosed.
Created attachment 120244 [details] Patch
Attachment 120244 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource From git://git.webkit.org/WebKit 7877ad0..e4a13f1 master -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 103474 = 7877ad0b253dfa76fd0208597dbae7ce5072fea0 r103475 = aa75dfaa601e7c5fcc0780716e0903da229dd1a5 r103476 = e4a13f19647c218b1fc1ce0ecc69c5ed16a4d7f4 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Applying: Inform the scrolling coordinator when scrollbar layers come and go Using index info to reconstruct a base tree... <stdin>:474806: trailing whitespace. [Chromium] DatabaseTrackerChromium: iterating DatabaseSet races with Database disposal on worker thread <stdin>:474827: trailing whitespace. Nothing to test, just removing redundant code. Correct behavior tested by <stdin>:475346: trailing whitespace. warning: 3 lines add whitespace errors. Falling back to patching base and 3-way merge... warning: too many files (created: 167528 deleted: 3), skipping inexact rename detection Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/chromium/test_expectations.txt CONFLICT (content): Merge conflict in LayoutTests/platform/chromium/test_expectations.txt Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Auto-merging Source/WebCore/page/ScrollingCoordinator.h CONFLICT (content): Merge conflict in Source/WebCore/page/ScrollingCoordinator.h Auto-merging Source/WebCore/page/mac/ScrollingCoordinatorMac.mm CONFLICT (content): Merge conflict in Source/WebCore/page/mac/ScrollingCoordinatorMac.mm Auto-merging Source/WebCore/platform/ScrollView.cpp Auto-merging Source/WebCore/rendering/RenderLayerCompositor.cpp CONFLICT (content): Merge conflict in Source/WebCore/rendering/RenderLayerCompositor.cpp Auto-merging Source/WebKit2/ChangeLog CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog Auto-merging Tools/ChangeLog CONFLICT (content): Merge conflict in Tools/ChangeLog Auto-merging Tools/Scripts/build-webkit Auto-merging Tools/Scripts/webkitdirs.pm CONFLICT (content): Merge conflict in Tools/Scripts/webkitdirs.pm Failed to merge in the changes. Patch failed at 0001 Inform the scrolling coordinator when scrollbar layers come and go When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 158. If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 120244 [details] Patch R=me
Committed r104199: <http://trac.webkit.org/changeset/104199>