Bug 72106 – Consolidate the logic that creates run loop observers for flushing layer tree changes to CoreAnimation

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

Consolidate the logic that creates run loop observers for flushing layer tree changes to CoreAnimation

[Andy Estes]

Created attachment 114661 [details]
Patch

[Anders Carlsson]

Comment on attachment 114661 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/LayerFlushScheduler.h:42
> +    typedef void (*CallbackFunction)(void* context);
> +
> +    static PassOwnPtr<LayerFlushScheduler> create(CallbackFunction callback, void* context)

It would be nicer if this took a Client class instead of a callback and a context. You'd have to add an extra class to WebKit1, but maybe that's fine.

> Source/WebCore/platform/graphics/ca/LayerFlushScheduler.h:52
> +    void invalidate();
> +    void invalidateIfScheduled();

I don't think we need both invalidate and invalidateIfScheduled. Let's just have invalidate() become invalidateIfScheduled().

> Source/WebCore/platform/graphics/ca/LayerFlushScheduler.h:53
> +    void setEnabled(bool);

Instead of having a setEnabled function, it would be nicer to have suspend/resume calls that would increment/decrement a counter.

[Darin Adler]

Comment on attachment 114661 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/mac/LayerFlushSchedulerMac.cpp:32
> +#define CoreAnimationRunLoopOrder 2000000

We use const for this sort of thing, not #define.

> Source/WebCore/platform/graphics/ca/mac/LayerFlushSchedulerMac.cpp:34
> +using namespace WebCore;

File should be *in* the WebCore namespace, not *using* the WebCore namespace.

> Source/WebCore/platform/graphics/ca/mac/LayerFlushSchedulerMac.cpp:37
> +    : m_enabled(true)

I think it’s better to name this m_isEnabled.

> Source/WebCore/platform/graphics/ca/mac/LayerFlushSchedulerMac.cpp:49
> +void LayerFlushScheduler::runLoopObserverCallback(CFRunLoopObserverRef, CFRunLoopActivity, void* info)

Why info vs. context?

> Source/WebCore/platform/graphics/ca/mac/LayerFlushSchedulerMac.cpp:71
> +    // Run before the Core Animation commit observer.
> +    const CFIndex runLoopOrder = CoreAnimationRunLoopOrder - 1;

I think this would be better at the top of the file.

> Source/WebCore/platform/graphics/ca/mac/LayerFlushSchedulerMac.cpp:80
> +    ASSERT(m_runLoopObserver);

Why assert this instead of doing an if? I think this should be idempotent and callable multiple times and you should not bother having a separate invalidateIfScheduled function.

> Source/WebCore/platform/graphics/ca/mac/LayerFlushSchedulerMac.cpp:103
> +    if (m_enabled) {
> +        schedule();
> +        return;
> +    }
> +
> +    invalidateIfScheduled();

Seems like this would read better as if/else.

[Andy Estes]

(In reply to comment #3)
> (From update of attachment 114661 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114661&action=review
> 
> > Source/WebCore/platform/graphics/ca/mac/LayerFlushSchedulerMac.cpp:49
> > +void LayerFlushScheduler::runLoopObserverCallback(CFRunLoopObserverRef, CFRunLoopActivity, void* info)
> 
> Why info vs. context?

info is what CF calls the third parameter in its CFRunLoopObserverCallBack declaration in CFRunLoop.h. context is the term I use in this class, so it's probably more consistent to go with that.

> 
> > Source/WebCore/platform/graphics/ca/mac/LayerFlushSchedulerMac.cpp:80
> > +    ASSERT(m_runLoopObserver);
> 
> Why assert this instead of doing an if? I think this should be idempotent and callable multiple times and you should not bother having a separate invalidateIfScheduled function.

LayerTreeHostCAMac asserted that a run loop observer was in fact registered before invalidating it in didPerformScheduledLayerFlush(), so I wanted to provide a way to retain this assertion without exposing m_runLoopObserver, hence invalidate() and invalidateIfScheduled(). I agree this makes for a confusing interface. I can make invalidate() do what invalidateIfScheduled() does, drop invalidateIfScheduled(), and perhaps even add a debug-only method for asserting that a run loop observer is scheduled.

[Andy Estes]

(In reply to comment #2)
> (From update of attachment 114661 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114661&action=review
> 
> > Source/WebCore/platform/graphics/ca/LayerFlushScheduler.h:53
> > +    void setEnabled(bool);
> 
> Instead of having a setEnabled function, it would be nicer to have suspend/resume calls that would increment/decrement a counter.

I'm not sure I follow why this is better given the current use of setEnabled() (and setLayerTreeStateIsFrozen(), by extension) in WebKit2. For instance, I don't think calls to setLayerTreeStateIsFrozen() in WebFrameLoaderClient are guaranteed to be balanced, so we'd have to do a check to make sure we don't over-resume. This seems more complicated than the current approach.

Maybe you have a use case in mind that I don't know about, or maybe I'm missing something.

[Andy Estes]

Created attachment 115105 [details]
Patch

Despite my previous question, I decided to implement suspend()/resume() to see what it would look like. This patch addresses all review feedback.

[Andy Estes]

Comment on attachment 115105 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/LayerFlushScheduler.cpp:40
> +    ASSERT(m_suspendCount);

This should be m_suspendCount > 0

[Anders Carlsson]

Comment on attachment 115105 [details]
Patch

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

>> Source/WebCore/platform/graphics/ca/LayerFlushScheduler.cpp:40
>> +    ASSERT(m_suspendCount);
> 
> This should be m_suspendCount > 0

I don't think this assertion buys us anything unless someone calls suspend more than 2^31 times :)

> Source/WebCore/platform/graphics/ca/LayerFlushScheduler.cpp:46
> +    --m_suspendCount;
> +    ASSERT(m_suspendCount > -1);

I think it's cleaner to put the ASSERT before the decrement here - then you can just assert that m_suspendCount is not 0.

> Source/WebCore/platform/graphics/ca/LayerFlushScheduler.h:51
> +    int m_suspendCount;

I'd make this an unsigned integer instead.

[Andy Estes]

Committed r100384: <http://trac.webkit.org/changeset/100384>