Bug 128125 - Refine MediaSession interruptions
Summary: Refine MediaSession interruptions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords:
Depends on: 128184
Blocks:
  Show dependency treegraph
 
Reported: 2014-02-03 14:11 PST by Eric Carlson
Modified: 2014-02-12 12:25 PST (History)
6 users (show)

See Also:


Attachments
Proposed patch (41.69 KB, patch)
2014-02-03 14:49 PST, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2014-02-03 14:11:11 PST
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.
Comment 1 Eric Carlson 2014-02-03 14:49:04 PST
Created attachment 223024 [details]
Proposed patch
Comment 2 WebKit Commit Bot 2014-02-03 14:51:23 PST
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.
Comment 3 Jer Noble 2014-02-03 15:23:58 PST
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.
Comment 4 Eric Carlson 2014-02-04 06:55:10 PST
(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.
Comment 5 Eric Carlson 2014-02-04 06:55:38 PST
https://trac.webkit.org/r163376
Comment 6 WebKit Commit Bot 2014-02-04 07:45:24 PST
Re-opened since this is blocked by bug 128184
Comment 7 Eric Carlson 2014-02-04 11:05:34 PST
Recommitted in r163390: https://trac.webkit.org/r163390
Comment 8 Eric Carlson 2014-02-04 11:18:12 PST
Plus https://trac.webkit.org/r163393 to fix Release builds.
Comment 9 Darin Adler 2014-02-08 08:31:38 PST
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).
Comment 10 Eric Carlson 2014-02-10 07:52:35 PST
(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).
Comment 11 Eric Carlson 2014-02-12 12:25:44 PST
Made the suggested cleanups in https://trac.webkit.org/r163971.