[webkit-reviews] review granted: [Bug 173718] [iOS] Respond to AudioSession interruption and resume : [Attachment 313639] Proposed patch.

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 173718: [iOS] Respond to AudioSession interruption and resume
https://bugs.webkit.org/show_bug.cgi?id=173718

Attachment 313639: Proposed patch.

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




--- Comment #3 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 313639
  --> https://bugs.webkit.org/attachment.cgi?id=313639
Proposed patch.

LGTM.
I tried to think about cases where we would run into issues with all these
states but just got some questions.
I wonder how much we would be able to test that all these state handling are
fine with unit testing.

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

> Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.h:38
> +private:

No need for space above private?

> Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.h:42
> +    virtual ~CoreAudioCaptureSourceIOS();

Probably no need for virtual here.

> Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.mm:88
> +	   return;

Can this case actually happen?
If the listener callback is null, it is probably because it was invalidated and
will not receive this notification?

> Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.mm:92
> +    else {

Should we assert that the interruptionType is
AVAudioSessionInterruptionTypeEnded? Maybe overkill?

> Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.mm:97
> +#endif

Probably no need for #if/#endif, except if wanting to do release logging, which
might be useful.

Do we want to log that an interruption started also?

> Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.mm:98
> +	   _callback->endInterruption();

If there is an error in setActive, should we still call endInterruption or do
specific handling?

> Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.mm:107
> +	   return;

Is it needed?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:90
> +    bool suspended() const { return m_suspended; }

Since we have suspend which is close, maybe we should name it isSuspended()?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:605
> +OSStatus CoreAudioSharedUnit::resume()

This seems very close to startProducingData().
Maybe we can do some nice refactoring?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:618
> +	   LOG(Media, "CoreAudioSharedUnit::start(%p) AudioOutputUnitStart
failed with error %d (%.4s)", this, (int)err, (char*)&err);

::start -> ::resume.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:668
> +

These two lines are also good for iOS, in particular persistentID which is
missing in iOS.
Maybe, to help the readability, you could introduce some helper functions like
deviceFromID and createCaptureSource that would have the #if/#endif and keep
CoreAudioCaptureSource::create free from #if/#endif.
The alternative would be to have a CoreAudioCaptureSourceIOS::create and have
this handling in the factory.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:685
> +    return CaptureSourceOrError(*source);

Use source.releaseNonNull() probably or you can stick with auto in both places.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:858
> +	   m_suspendType = unit.isProducingData() ?
SuspensionType::WhilePlaying : SuspensionType::WhilePaused;

What happens in the case of the source being played before interruption, so
m_suspendType is SuspensionType::WhilePlaying but the user paused the source
during interruption?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:885
> +	   return;

Why is the handling of m_suspendType and m_reconfigurationState different?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:888
> +    scheduleDeferredTask([this, type, &unit] {

No need to pass unit, sing you can get it through
CoreAudioSharedUnit::singleton().

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.h:73
> +    void scheduleReconfiguration();

Maybe you can make WebCoreAudioCaptureSourceIOSListener a friend?