Bug 128125 – Refine MediaSession interruptions

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

[Eric Carlson]

Don't assume a media session "begin interruption" will always be paired with an "end interruption". Move code for handling interruptions from HTMLMediaElement to HTMLMediaSession.

[Eric Carlson]

Created attachment 223024 [details]
Proposed patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/audio/MediaSession.h:57:  The parameter name "state" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/audio/MediaSession.cpp:39:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/html/HTMLMediaSession.cpp:50:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 3 in 22 files


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

[Jer Noble]

Comment on attachment 223024 [details]
Proposed patch

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

Nice stuff! r=me with just a couple of nits:

> Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:124
> +    [center addObserver:self selector:@selector(applicationWillEnterForeground:) name:UIApplicationWillEnterForegroundNotification object:nil];
> +    [center addObserver:self selector:@selector(applicationWillResignActive:) name:UIApplicationWillResignActiveNotification object:nil];
> +

Nit: this isn't going to work in WK2 mode.  We'll need to pipe a notification down through the process boundary in that case.  We should probably add a FIXME to come up with an alternate mechanism.

> Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:140
> +- (void)interruption:(NSNotification *)notification
> +{

Did you mean to get rid of the "if (!_callback)" check here? The prettydiff is a bit confusing.

[Eric Carlson]

(In reply to comment #3)
> (From update of attachment 223024 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223024&action=review
> 
> Nice stuff! r=me with just a couple of nits:
> 
> > Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:124
> > +    [center addObserver:self selector:@selector(applicationWillEnterForeground:) name:UIApplicationWillEnterForegroundNotification object:nil];
> > +    [center addObserver:self selector:@selector(applicationWillResignActive:) name:UIApplicationWillResignActiveNotification object:nil];
> > +
> 
> Nit: this isn't going to work in WK2 mode.  We'll need to pipe a notification down through the process boundary in that case.  We should probably add a FIXME to come up with an alternate mechanism.
> 

Good point, done.

> > Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:140
> > +- (void)interruption:(NSNotification *)notification
> > +{
> 
> Did you mean to get rid of the "if (!_callback)" check here? The prettydiff is a bit confusing.
> 

Fixed.

[Eric Carlson]

https://trac.webkit.org/r163376

[WebKit Commit Bot]

Re-opened since this is blocked by bug 128184

[Eric Carlson]

Recommitted in r163390: https://trac.webkit.org/r163390

[Eric Carlson]

Plus https://trac.webkit.org/r163393 to fix Release builds.

[Darin Adler]

Comment on attachment 223024 [details]
Proposed patch

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

> Source/WebCore/html/HTMLMediaSession.cpp:48
> +#define CASE(_restriction) case HTMLMediaSession::_restriction: return #_restriction; break;

No need for break after return. No need for underline prefix in macro argument name.

> Source/WebCore/html/HTMLMediaSession.cpp:50
> +    switch (restriction)
> +    {

WebKit coding style puts the { on the same line as the switch.

> Source/WebCore/html/HTMLMediaSession.cpp:59
> +    CASE(NoRestrictions);
> +    CASE(RequireUserGestureForLoad);
> +    CASE(RequireUserGestureForRateChange);
> +    CASE(RequireUserGestureForFullscreen);
> +    CASE(RequirePageConsentToLoadMedia);
> +    CASE(RequirePageConsentToResumeMedia);
> +#if ENABLE(IOS_AIRPLAY)
> +    CASE(RequireUserGestureToShowPlaybackTargetPicker);
> +#endif

Extra semicolons on all these lines. No reason to include the semicolon in both the macro and outside the macro.

> Source/WebCore/platform/audio/MediaSession.cpp:44
> +#define CASE(_state) case MediaSession::_state: return #_state; break;
> +    switch (state)
> +    {
> +    CASE(Idle);
> +    CASE(Playing);
> +    CASE(Paused);
> +    CASE(Interrupted);
> +    }

ll the same comments apply here as above.

> Source/WebCore/platform/audio/MediaSession.h:57
> +    void setState(State state);

Should remove the argument name here.

> Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:69
>  - (void)interruption:(NSNotification*)notification;
> +- (void)applicationWillEnterForeground:(NSNotification*)notification;
> +- (void)applicationWillResignActive:(NSNotification*)notification;

WebKit formatting puts a space before the "*" for Objective-C classes (although I personally don’t like that style rule).

[Eric Carlson]

(In reply to comment #9)
> (From update of attachment 223024 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223024&action=review
> 
> > Source/WebCore/html/HTMLMediaSession.cpp:48
> > +#define CASE(_restriction) case HTMLMediaSession::_restriction: return #_restriction; break;
> 
> No need for break after return. No need for underline prefix in macro argument name.
> 
> > Source/WebCore/html/HTMLMediaSession.cpp:50
> > +    switch (restriction)
> > +    {
> 
> WebKit coding style puts the { on the same line as the switch.
> 

I fixed before I committed the changes.

> > Source/WebCore/html/HTMLMediaSession.cpp:59
> > +    CASE(NoRestrictions);
> > +    CASE(RequireUserGestureForLoad);
> > +    CASE(RequireUserGestureForRateChange);
> > +    CASE(RequireUserGestureForFullscreen);
> > +    CASE(RequirePageConsentToLoadMedia);
> > +    CASE(RequirePageConsentToResumeMedia);
> > +#if ENABLE(IOS_AIRPLAY)
> > +    CASE(RequireUserGestureToShowPlaybackTargetPicker);
> > +#endif
> 
> Extra semicolons on all these lines. No reason to include the semicolon in both the macro and outside the macro.
> 

Good point, I will fix this.

> > Source/WebCore/platform/audio/MediaSession.cpp:44
> > +#define CASE(_state) case MediaSession::_state: return #_state; break;
> > +    switch (state)
> > +    {
> > +    CASE(Idle);
> > +    CASE(Playing);
> > +    CASE(Paused);
> > +    CASE(Interrupted);
> > +    }
> 
> ll the same comments apply here as above.
> 

Ditto.

> > Source/WebCore/platform/audio/MediaSession.h:57
> > +    void setState(State state);
> 
> Should remove the argument name here.
> 

Fixed before I committed the changes.

> > Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:69
> >  - (void)interruption:(NSNotification*)notification;
> > +- (void)applicationWillEnterForeground:(NSNotification*)notification;
> > +- (void)applicationWillResignActive:(NSNotification*)notification;
> 
> WebKit formatting puts a space before the "*" for Objective-C classes (although I personally don’t like that style rule).

Oops, I will fix this as well (although I also don't care for this seemingly arbitrary style difference).

[Eric Carlson]

Made the suggested cleanups in https://trac.webkit.org/r163971.