Bug 148315 - [iOS] AirPlay should not stop when the screen locks
Summary: [iOS] AirPlay should not stop when the screen locks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-08-21 11:35 PDT by Eric Carlson
Modified: 2015-10-01 20:37 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (12.93 KB, patch)
2015-08-21 11:59 PDT, Eric Carlson
jer.noble: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2015-08-21 11:35:02 PDT
Don't pause when the screen locks if playing to AirPlay.
Comment 1 Eric Carlson 2015-08-21 11:37:33 PDT
<rdar://problem/22300597>
Comment 2 Eric Carlson 2015-08-21 11:59:20 PDT
Created attachment 259637 [details]
Proposed patch
Comment 3 WebKit Commit Bot 2015-08-21 12:03:57 PDT
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.
Comment 4 Jer Noble 2015-08-24 10:20:58 PDT
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.
Comment 5 Eric Carlson 2015-08-24 10:25:52 PDT
(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.
Comment 6 Jer Noble 2015-08-24 10:36:53 PDT
(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!
Comment 7 Brent Fulgham 2015-10-01 15:33:20 PDT
Committed r190434: <http://trac.webkit.org/changeset/190434>
Comment 8 Brent Fulgham 2015-10-01 16:42:08 PDT
Note: This fix broke the test 'webaudio/audiocontext-state-interrupted.html', because it was not updated to pass an argument to internals.beginMediaSessionInterruption.
Comment 9 Brent Fulgham 2015-10-01 16:43:33 PDT
Test fix committed in:

Committed r190434: <http://trac.webkit.org/changeset/190438>
Comment 10 Gyuyoung Kim 2015-10-01 18:30:54 PDT
(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);
}
Comment 11 Gyuyoung Kim 2015-10-01 18:36:38 PDT
(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
Comment 12 Gyuyoung Kim 2015-10-01 18:38:40 PDT
(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
Comment 13 Brent Fulgham 2015-10-01 20:37:57 PDT
(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! :)