Bug 148315 – [iOS] AirPlay should not stop when the screen locks

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Don't pause when the screen locks if playing to AirPlay.

[Eric Carlson]

<rdar://problem/22300597>

[Eric Carlson]

Created attachment 259637 [details]
Proposed patch

[WebKit Commit Bot]

Attachment 259637 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/audio/PlatformMediaSession.cpp:44:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/audio/PlatformMediaSession.cpp:58:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Comment on attachment 259637 [details]
Proposed patch

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

r=me, with nit.

> Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:267
> +    Vector<PlatformMediaSession*> sessions = this->sessions();
> +    for (auto* session : sessions) {

nit: Why the local variable?  Seems like this could just be:

for (auto* session : sessions()) { ... }

Looking into PlatformMediaSessionManager, I don't understand why sessions() doesn't return a Vector<>&.  But it also seems a waste to have to create a new Vector every time this method is called.

[Eric Carlson]

(In reply to comment #4)
> Comment on attachment 259637 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259637&action=review
> 
> r=me, with nit.
> 
> > Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:267
> > +    Vector<PlatformMediaSession*> sessions = this->sessions();
> > +    for (auto* session : sessions) {
> 
> nit: Why the local variable?  Seems like this could just be:
> 
> for (auto* session : sessions()) { ... }
> 
> Looking into PlatformMediaSessionManager, I don't understand why sessions()
> doesn't return a Vector<>&.  But it also seems a waste to have to create a
> new Vector every time this method is called.

I think because calling session.whatever() can cause m_sessions to be modified.

[Jer Noble]

(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 259637 [details]
> > Proposed patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=259637&action=review
> > 
> > r=me, with nit.
> > 
> > > Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:267
> > > +    Vector<PlatformMediaSession*> sessions = this->sessions();
> > > +    for (auto* session : sessions) {
> > 
> > nit: Why the local variable?  Seems like this could just be:
> > 
> > for (auto* session : sessions()) { ... }
> > 
> > Looking into PlatformMediaSessionManager, I don't understand why sessions()
> > doesn't return a Vector<>&.  But it also seems a waste to have to create a
> > new Vector every time this method is called.
> 
> I think because calling session.whatever() can cause m_sessions to be
> modified.

Ah yes, I see that pattern elsewhere in PlatformMediaSessionManager.cpp. Carry on!

[Brent Fulgham]

Committed r190434: <http://trac.webkit.org/changeset/190434>

[Brent Fulgham]

Note: This fix broke the test 'webaudio/audiocontext-state-interrupted.html', because it was not updated to pass an argument to internals.beginMediaSessionInterruption.

[Brent Fulgham]

Test fix committed in:

Committed r190434: <http://trac.webkit.org/changeset/190438>

[Gyuyoung Kim]

(In reply to comment #7)
> Committed r190434: <http://trac.webkit.org/changeset/190434>

Is there any reason we don't use *interruption* local variable in beginInterruption() ?


void Internals::beginMediaSessionInterruption(const String& interruptionString, ExceptionCode& ec)
{
    PlatformMediaSession::InterruptionType interruption = PlatformMediaSession::SystemInterruption;

    if (equalIgnoringCase(interruptionString, "System"))
        interruption = PlatformMediaSession::SystemInterruption;
    else if (equalIgnoringCase(interruptionString, "SystemSleep"))
        interruption = PlatformMediaSession::SystemSleep;
    else if (equalIgnoringCase(interruptionString, "EnteringBackground"))
        interruption = PlatformMediaSession::EnteringBackground;
    else {
        ec = INVALID_ACCESS_ERR;
        return;
    }

    PlatformMediaSessionManager::sharedManager().beginInterruption(PlatformMediaSession::SystemInterruption);
}

[Gyuyoung Kim]

(In reply to comment #10)
> (In reply to comment #7)
> > Committed r190434: <http://trac.webkit.org/changeset/190434>
> 
> Is there any reason we don't use *interruption* local variable in
> beginInterruption() ?
> 
> 
> void Internals::beginMediaSessionInterruption(const String&
> interruptionString, ExceptionCode& ec)
> {
>     PlatformMediaSession::InterruptionType interruption =
> PlatformMediaSession::SystemInterruption;
> 
>     if (equalIgnoringCase(interruptionString, "System"))
>         interruption = PlatformMediaSession::SystemInterruption;
>     else if (equalIgnoringCase(interruptionString, "SystemSleep"))
>         interruption = PlatformMediaSession::SystemSleep;
>     else if (equalIgnoringCase(interruptionString, "EnteringBackground"))
>         interruption = PlatformMediaSession::EnteringBackground;
>     else {
>         ec = INVALID_ACCESS_ERR;
>         return;
>     }
> 
>    
> PlatformMediaSessionManager::sharedManager().
> beginInterruption(PlatformMediaSession::SystemInterruption);
> }


I upload a fix to Bug 148315. If there is any reason we don't use *interruption* variable, please let me know.

https://bugs.webkit.org/show_bug.cgi?id=148315

[Gyuyoung Kim]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #7)
> > > Committed r190434: <http://trac.webkit.org/changeset/190434>
> > 
> > Is there any reason we don't use *interruption* local variable in
> > beginInterruption() ?
> > 
> > 
> > void Internals::beginMediaSessionInterruption(const String&
> > interruptionString, ExceptionCode& ec)
> > {
> >     PlatformMediaSession::InterruptionType interruption =
> > PlatformMediaSession::SystemInterruption;
> > 
> >     if (equalIgnoringCase(interruptionString, "System"))
> >         interruption = PlatformMediaSession::SystemInterruption;
> >     else if (equalIgnoringCase(interruptionString, "SystemSleep"))
> >         interruption = PlatformMediaSession::SystemSleep;
> >     else if (equalIgnoringCase(interruptionString, "EnteringBackground"))
> >         interruption = PlatformMediaSession::EnteringBackground;
> >     else {
> >         ec = INVALID_ACCESS_ERR;
> >         return;
> >     }
> > 
> >    
> > PlatformMediaSessionManager::sharedManager().
> > beginInterruption(PlatformMediaSession::SystemInterruption);
> > }
> 
> 
> I upload a fix to Bug 148315. If there is any reason we don't use
> *interruption* variable, please let me know.
> 
> https://bugs.webkit.org/show_bug.cgi?id=148315

Oops, r190447 already fixed it. - http://trac.webkit.org/changeset/190447

[Brent Fulgham]

(In reply to comment #10)
> (In reply to comment #7)
> > Committed r190434: <http://trac.webkit.org/changeset/190434>
> 
> Is there any reason we don't use *interruption* local variable in
> beginInterruption() ?

It's just an oversight. The initial test case always used System, so it was missed earlier today. I had a note to myself to fix it tomorrow, but you've beat me to it! :)