Bug 157553 - If previous media session interruptions were prevented, still allow subsequent interruptions to try.
Summary: If previous media session interruptions were prevented, still allow subsequen...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-05-10 19:08 PDT by Jeremy Jones
Modified: 2017-03-30 16:59 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.16 KB, patch)
2016-05-10 19:16 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (2.14 KB, patch)
2016-05-11 11:02 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (9.81 KB, patch)
2016-06-27 13:38 PDT, Jeremy Jones
eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing. (12.67 KB, patch)
2016-07-18 16:32 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch for landing. (12.73 KB, patch)
2016-07-18 16:34 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Jones 2016-05-10 19:08:15 PDT
If previous media session interruptions were prevented, still allow subsequent interruptions to try.
Comment 1 Jeremy Jones 2016-05-10 19:09:11 PDT
rdar://problem/25740804
Comment 2 Jeremy Jones 2016-05-10 19:16:03 PDT
Created attachment 278571 [details]
Patch
Comment 3 Eric Carlson 2016-05-11 08:00:07 PDT
Comment on attachment 278571 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        If previous media session interruptions were prevented, still allow subsequent interruptions to try.
> +        https://bugs.webkit.org/show_bug.cgi?id=157553

Please include the Radar number as well.

> Source/WebCore/platform/audio/PlatformMediaSession.cpp:102
> +    

Nit: tab be gone.

> Source/WebCore/platform/audio/PlatformMediaSession.cpp:103
> +    if (++m_interruptionCount > 1 && m_interruptionType != NoInterruption)

Have you verified that there are no side effects of allowing all nested interruptions through?
Comment 4 Jeremy Jones 2016-05-11 11:01:57 PDT
(In reply to comment #3)
> Comment on attachment 278571 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278571&action=review
> 
> > Source/WebCore/ChangeLog:4
> > +        If previous media session interruptions were prevented, still allow subsequent interruptions to try.
> > +        https://bugs.webkit.org/show_bug.cgi?id=157553
> 
> Please include the Radar number as well.

Done.

> 
> > Source/WebCore/platform/audio/PlatformMediaSession.cpp:102
> > +    
> 
> Nit: tab be gone.

Done.

> 
> > Source/WebCore/platform/audio/PlatformMediaSession.cpp:103
> > +    if (++m_interruptionCount > 1 && m_interruptionType != NoInterruption)
> 
> Have you verified that there are no side effects of allowing all nested
> interruptions through?

I don't want to allow all nested interruptions. I only want to allow interruptions if the previous interruption was ignored.

We only ignore EnteringBackground for m_isPlayingToWirelessTarget or Picture in Picture. If we receive any of the other interruptions after that, I would want the interruption to succeed even though the app is in the background.
Comment 5 Jeremy Jones 2016-05-11 11:02:42 PDT
Created attachment 278636 [details]
Patch
Comment 6 Eric Carlson 2016-05-11 11:53:15 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > > Source/WebCore/platform/audio/PlatformMediaSession.cpp:103
> > > +    if (++m_interruptionCount > 1 && m_interruptionType != NoInterruption)
> > 
> > Have you verified that there are no side effects of allowing all nested
> > interruptions through?
> 
> I don't want to allow all nested interruptions. I only want to allow
> interruptions if the previous interruption was ignored.
> 
> We only ignore EnteringBackground for m_isPlayingToWirelessTarget or Picture
> in Picture. If we receive any of the other interruptions after that, I would
> want the interruption to succeed even though the app is in the background.

Isn't 'NoInterruption' only set in endInterruption? In any case, this change definitely needs a layout test to verify that nested interruptions of various types behave as expected.
Comment 7 Jeremy Jones 2016-06-27 13:38:35 PDT
Created attachment 282164 [details]
Patch
Comment 8 Eric Carlson 2016-06-27 14:21:10 PDT
Comment on attachment 282164 [details]
Patch

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

> Source/WebCore/platform/audio/PlatformMediaSession.cpp:103
> +    if (++m_interruptionCount > 1 && m_interruptionType != NoInterruption)

A comment about why this works would be helpful.

> LayoutTests/ChangeLog:17
> +        This test is for iPad only, so it must be run manually.

Is "it must be run manually" true?
Comment 9 Jeremy Jones 2016-07-18 16:02:35 PDT
(In reply to comment #8)
> Comment on attachment 282164 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282164&action=review
> 
> > Source/WebCore/platform/audio/PlatformMediaSession.cpp:103
> > +    if (++m_interruptionCount > 1 && m_interruptionType != NoInterruption)
> 
> A comment about why this works would be helpful.

    // When interruptions are overridden, m_interruptionType doesn't get set.
    // Give nested interruptions a chance when the previous interruptions were overridden.

> 
> > LayoutTests/ChangeLog:17
> > +        This test is for iPad only, so it must be run manually.
> 
> Is "it must be run manually" true?

Tests can't specify that they are iPad only.
Comment 10 Jeremy Jones 2016-07-18 16:32:41 PDT
Created attachment 283954 [details]
Patch for landing.
Comment 11 Jeremy Jones 2016-07-18 16:34:34 PDT
Created attachment 283957 [details]
Patch for landing.
Comment 12 WebKit Commit Bot 2016-07-18 17:48:35 PDT
Comment on attachment 283957 [details]
Patch for landing.

Clearing flags on attachment: 283957

Committed r203388: <http://trac.webkit.org/changeset/203388>