Bug 181623 – AVSampleBufferDisplayLayer should be flushed when application activates

• Component: WebRTC
• Status: RESOLVED FIXED

[Eric Carlson]

AVSampleBufferDisplayLayer sometimes gets into an error state when the web process deactivates, and it must be flushed when application activates again.

[Radar WebKit Bug Importer]

<rdar://problem/36487738>

[Eric Carlson]

Created attachment 331261 [details]
Patch

[Darin Adler]

Comment on attachment 331261 [details]
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 "{ }".

[youenn fablet]

Comment on attachment 331261 [details]
Patch

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

> Source/WebCore/dom/Document.h:1411
> +

All these state changes are probably the same for all documents of a page.
How about moving these methods to the page and having media elements register to the page directly?

[Darin Adler]

Comment on attachment 331261 [details]
Patch

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

>> Source/WebCore/dom/Document.h:1411
>> +
> 
> All these state changes are probably the same for all documents of a page.
> How about moving these methods to the page and having media elements register to the page directly?

The great thing about a document is that it always outlasts all the elements in the document. That’s why it’s a great place to register things. The page lifecycle is not such a good fit.

I think it’s OK to register on the document, but I do agree that we should keep the minimum on the document; that’s why I suggested exposing just three functions (add/remove/forEach) on Document.

[Eric Carlson]

Created attachment 331391 [details]
Patch

[Eric Carlson]

Comment on attachment 331261 [details]
Patch

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

>> Source/WebCore/dom/Document.h:1410
>> +    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.

Good idea, changed.

>> Source/WebCore/dom/Document.h:1895
>> +    HashSet<ApplicationStateChangeListener*> m_applicationStateChangeListener;
> 
> Should be m_applicationStateChangeListeners, plural.

Fixed.

>> Source/WebCore/page/ApplicationStateChangeListener.h:39
>> +    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 "{ }".

Removed all parameters and semicolons.

[WebKit Commit Bot]

Comment on attachment 331391 [details]
Patch

Clearing flags on attachment: 331391

Committed r226990: <https://trac.webkit.org/changeset/226990>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.