Bug 74769 – [chromium] Add CCTimer class for the compositor

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tien-Ren Chen]

[chromium] Add CCTimer class for the compositor

[Tien-Ren Chen]

Created attachment 119702 [details]
Patch

[WebKit Review Bot]

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.

[James Robinson]

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.

[Tien-Ren Chen]

Created attachment 119956 [details]
Patch

[James Robinson]

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

[Tien-Ren Chen]

Created attachment 119963 [details]
Patch

[James Robinson]

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

[Nat Duca]

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?

[James Robinson]

That would work too, although we should still tag in that case.

[Nat Duca]

Fine by me.

[Tien-Ren Chen]

(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

[Tien-Ren Chen]

(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.

[Tien-Ren Chen]

Created attachment 120234 [details]
Patch

[WebKit Review Bot]

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.

[Tien-Ren Chen]

Created attachment 120237 [details]
Patch

[WebKit Review Bot]

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.

[Nat Duca]

Comment on attachment 120237 [details]
Patch

I think the style bot is hosed.

[Tien-Ren Chen]

Created attachment 120244 [details]
Patch

[WebKit Review Bot]

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.

[James Robinson]

Comment on attachment 120244 [details]
Patch

R=me

[Nat Duca]

Committed r104199: <http://trac.webkit.org/changeset/104199>