Bug 147797 - Media Session: generalize isFocusedContentMediaElementPlaying so it works with other playback attributes
Summary: Media Session: generalize isFocusedContentMediaElementPlaying so it works wit...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 145411
  Show dependency treegraph
 
Reported: 2015-08-07 15:56 PDT by Matt Rajca
Modified: 2015-08-10 12:19 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.01 KB, patch)
2015-08-07 15:58 PDT, Matt Rajca
no flags Details | Formatted Diff | Diff
Patch (5.90 KB, patch)
2015-08-10 11:00 PDT, Matt Rajca
no flags Details | Formatted Diff | Diff
Patch (6.25 KB, patch)
2015-08-10 11:55 PDT, Matt Rajca
no flags Details | Formatted Diff | Diff
Patch (6.24 KB, patch)
2015-08-10 12:05 PDT, Matt Rajca
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Rajca 2015-08-07 15:56:49 PDT
Media Session Focus manager's 'isFocusedContentMediaElementPlaying' method will be refactored to a more general 'valueForPlaybackAttribute' so it works with other attributes (IsNextTrackControlEnabled and IsPreviousTrackControlEnabled).
Comment 1 Matt Rajca 2015-08-07 15:58:55 PDT
Created attachment 258538 [details]
Patch
Comment 2 Matt Rajca 2015-08-10 10:30:02 PDT
Comment on attachment 258538 [details]
Patch

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

> Source/WebKit2/UIProcess/WebMediaSessionFocusManager.h:35
> +#include <WebCore/MediaRemoteControls.h>

I removed this include after sending out the patch since we don't need it here.
Comment 3 Simon Fraser (smfr) 2015-08-10 10:50:38 PDT
Comment on attachment 258538 [details]
Patch

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

> Source/WebKit2/UIProcess/WebMediaSessionFocusManager.h:66
> +    HashMap<WKMediaSessionFocusManagerPlaybackAttribute, bool> m_playbackAttributes;

Using a HashMap here is overkill, especially since you could put WKMediaSessionFocusManagerPlaybackAttribute into a bit mask.
Comment 4 Matt Rajca 2015-08-10 11:00:22 PDT
Created attachment 258625 [details]
Patch
Comment 5 Matt Rajca 2015-08-10 11:00:46 PDT
(In reply to comment #3)
> Comment on attachment 258538 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258538&action=review
> 
> > Source/WebKit2/UIProcess/WebMediaSessionFocusManager.h:66
> > +    HashMap<WKMediaSessionFocusManagerPlaybackAttribute, bool> m_playbackAttributes;
> 
> Using a HashMap here is overkill, especially since you could put
> WKMediaSessionFocusManagerPlaybackAttribute into a bit mask.

Switched over!
Comment 6 Simon Fraser (smfr) 2015-08-10 11:13:37 PDT
Comment on attachment 258625 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKMediaSessionFocusManager.h:44
>  enum {
> -    IsPlaying = 1 << 0
> +    IsPlaying                     = 1 << 0,
> +    IsPreviousTrackControlEnabled = 1 << 1,
> +    IsNextTrackControlEnabled     = 1 << 2,
>  };
>  typedef uint32_t WKMediaSessionFocusManagerPlaybackAttribute;

This should be enum WKMediaSessionFocusManagerPlaybackAttribute {  ... }
typedef uint32_t WKMediaSessionFocusManagerPlaybackAttributes;

> Source/WebKit2/UIProcess/WebMediaSessionFocusManager.h:65
> +    WKMediaSessionFocusManagerPlaybackAttribute m_playbackAttributes { 0 };

WKMediaSessionFocusManagerPlaybackAttributes
Comment 7 Matt Rajca 2015-08-10 11:55:01 PDT
Created attachment 258630 [details]
Patch
Comment 8 Matt Rajca 2015-08-10 11:57:50 PDT
(In reply to comment #6)
> Comment on attachment 258625 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258625&action=review
> 
> > Source/WebKit2/UIProcess/API/C/WKMediaSessionFocusManager.h:44
> >  enum {
> > -    IsPlaying = 1 << 0
> > +    IsPlaying                     = 1 << 0,
> > +    IsPreviousTrackControlEnabled = 1 << 1,
> > +    IsNextTrackControlEnabled     = 1 << 2,
> >  };
> >  typedef uint32_t WKMediaSessionFocusManagerPlaybackAttribute;
> 
> This should be enum WKMediaSessionFocusManagerPlaybackAttribute {  ... }
> typedef uint32_t WKMediaSessionFocusManagerPlaybackAttributes;
> 
> > Source/WebKit2/UIProcess/WebMediaSessionFocusManager.h:65
> > +    WKMediaSessionFocusManagerPlaybackAttribute m_playbackAttributes { 0 };
> 
> WKMediaSessionFocusManagerPlaybackAttributes

Fixed.
Comment 9 Simon Fraser (smfr) 2015-08-10 11:59:04 PDT
Comment on attachment 258630 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKMediaSessionFocusManager.h:65
> +WK_EXPORT bool WKMediaSessionFocusManagerValueForPlaybackAttribute(WKMediaSessionFocusManagerRef manager, WKMediaSessionFocusManagerPlaybackAttribute attribute);

No need to name the parameters here.
Comment 10 Matt Rajca 2015-08-10 12:05:42 PDT
Created attachment 258633 [details]
Patch
Comment 11 Matt Rajca 2015-08-10 12:19:37 PDT
Committed r188218: <http://trac.webkit.org/changeset/188218>