[webkit-reviews] review granted: [Bug 181623] AVSampleBufferDisplayLayer should be flushed when application activates : [Attachment 331261] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 181623: AVSampleBufferDisplayLayer should be flushed when application
activates
https://bugs.webkit.org/show_bug.cgi?id=181623

Attachment 331261: Patch

https://bugs.webkit.org/attachment.cgi?id=331261&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 331261
  --> https://bugs.webkit.org/attachment.cgi?id=331261
Patch

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

Can we find a way to test this?

> Source/WebCore/dom/Document.h:1410
> +    void applicationWillResignActive();
> +    void applicationDidEnterBackground(bool);
> +    void applicationWillEnterForeground(bool);
> +    void applicationDidBecomeActive();

Perhaps instead of these we can just expose a single function:

    forEachApplicationStateChangeListener(const
Function<void(ApplicationStateChangeListener&)>&);

I think this would cut down the total amount of code. The functions in Page can
do more of the work.

> Source/WebCore/dom/Document.h:1895
> +    HashSet<ApplicationStateChangeListener*>
m_applicationStateChangeListener;

Should be m_applicationStateChangeListeners, plural.

> Source/WebCore/page/ApplicationStateChangeListener.h:39
> +    virtual void applicationWillResignActive(Document&) { };
> +    virtual void applicationDidBecomeActive(Document&) { };
> +
> +    enum class SuspendState { Suspended, SuspendedUnderLock };
> +    virtual void applicationDidEnterBackground(Document&, SuspendState) { };
> +    virtual void applicationWillEnterForeground(Document&, SuspendState) {
};

I don’t think these functions need to receive a Document& argument. I know that
in Objective-C delegation patterns we often pass the object, but this is
internal to WebKit and this might not be helpful. It’s very easy for DOM nodes
to get their document.

The did/will enter foreground/background functions are not used yet. Can we
wait to add them until we need them instead of adding them now?

What about the SuspendState? Will clients really need these? And is it better
to use argument to communicate this rather than four separate functions?

If we do need to communicate the suspend state and decide we want to use an
argument instead of additional separate functions, I suggest we just use a
boolean; our rule of thumb is that we use enumerations when callers are going
to be passing constants that would otherwise be mysterious. But there are no
callers like that.

No need for any of these semicolons after the "{ }".