Bug 164789 - Implement WebPlaybackControlsManager
Summary: Implement WebPlaybackControlsManager
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-15 13:51 PST by Beth Dakin
Modified: 2016-11-16 13:10 PST (History)
9 users (show)

See Also:


Attachments
Patch (18.82 KB, patch)
2016-11-15 13:53 PST, Beth Dakin
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2016-11-15 13:51:06 PST
Implement WebPlaybackControlsManager
Comment 1 Beth Dakin 2016-11-15 13:53:23 PST
Created attachment 294876 [details]
Patch
Comment 2 WebKit Commit Bot 2016-11-15 13:55:58 PST
Attachment 294876 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mac/WebPlaybackControlsManager.mm:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/mac/WebPlaybackControlsManager.mm:108:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/mac/WebPlaybackControlsManager.mm:242:  The parameter name "playing" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 mitz 2016-11-15 20:52:52 PST
Comment on attachment 294876 [details]
Patch

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

> Source/WebCore/platform/mac/WebPlaybackControlsManager.h:29
> +#import "AVKitSPI.h"

Since this is a framework header, it should import other WebCore headers using <WebCore/ syntax.

> Source/WebCore/platform/mac/WebPlaybackControlsManager.h:42
> +@interface WebPlaybackControlsManager : NSObject<AVFunctionBarPlaybackControlsControlling> {

Missing space before the <.

> Source/WebCore/platform/mac/WebPlaybackControlsManager.h:65
> +@property NSTimeInterval seekToTime;

This could probably be nonatomic (as could may other properties here).

> Source/WebCore/platform/mac/WebPlaybackControlsManager.h:68
> +@property BOOL hasEnabledAudio;
> +@property BOOL hasEnabledVideo;

Can be nonatomic.

> Source/WebCore/platform/mac/WebPlaybackControlsManager.h:70
> +@property (nonatomic, retain, readwrite) NSArray<AVFunctionBarMediaSelectionOption *> *audioFunctionBarMediaSelectionOptions;
> +@property (nonatomic, retain, readwrite) NSArray<AVFunctionBarMediaSelectionOption *> *legibleFunctionBarMediaSelectionOptions;

We normally omit “readwrite” when possible. It’s a little unusual for a property whose value is NSArray to be “retain” rather than “copy”.

> Source/WebCore/platform/mac/WebPlaybackControlsManager.mm:98
> +    completionHandler(@[], nil);

Missing space between the brackets.

> Source/WebCore/platform/mac/WebPlaybackControlsManager.mm:104
> +    completionHandler(@[], nil);

Ditto.

> Source/WebCore/platform/mac/WebPlaybackControlsManager.mm:132
> +    _audioFunctionBarMediaSelectionOptions = audioOptions;

I’d make a copy of audioOptions here (and change the property from “retain” to “copy”.

> Source/WebCore/platform/mac/WebPlaybackControlsManager.mm:149
> +    if (audioMediaSelectionOption && self.audioFunctionBarMediaSelectionOptions)

We normally don’t use trivial accessors from within a class’s own implementation, but just use the ivar directly.

> Source/WebCore/platform/mac/WebPlaybackControlsManager.mm:162
> +    _legibleFunctionBarMediaSelectionOptions = legibleOptions;

I’d make a copy of legibleOptions here (and change the property from “retain” to “copy”.
Comment 4 Beth Dakin 2016-11-16 12:21:57 PST
(In reply to comment #3)
> Comment on attachment 294876 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294876&action=review
> 
> > Source/WebCore/platform/mac/WebPlaybackControlsManager.h:29
> > +#import "AVKitSPI.h"
> 
> Since this is a framework header, it should import other WebCore headers
> using <WebCore/ syntax.
> 
Okay!

> > Source/WebCore/platform/mac/WebPlaybackControlsManager.h:42
> > +@interface WebPlaybackControlsManager : NSObject<AVFunctionBarPlaybackControlsControlling> {
> 
> Missing space before the <.
> 

Fixed!

> > Source/WebCore/platform/mac/WebPlaybackControlsManager.h:65
> > +@property NSTimeInterval seekToTime;
> 
> This could probably be nonatomic (as could may other properties here).
> 
> > Source/WebCore/platform/mac/WebPlaybackControlsManager.h:68
> > +@property BOOL hasEnabledAudio;
> > +@property BOOL hasEnabledVideo;
> 
> Can be nonatomic.
> 

Added nonatomic.

> > Source/WebCore/platform/mac/WebPlaybackControlsManager.h:70
> > +@property (nonatomic, retain, readwrite) NSArray<AVFunctionBarMediaSelectionOption *> *audioFunctionBarMediaSelectionOptions;
> > +@property (nonatomic, retain, readwrite) NSArray<AVFunctionBarMediaSelectionOption *> *legibleFunctionBarMediaSelectionOptions;
> 
> We normally omit “readwrite” when possible. It’s a little unusual for a
> property whose value is NSArray to be “retain” rather than “copy”.
> 

We discussed this on irc and agreed that this can be removed from the header because it's a part of the protocol.

> > Source/WebCore/platform/mac/WebPlaybackControlsManager.mm:98
> > +    completionHandler(@[], nil);
> 
> Missing space between the brackets.
> 

Fixed.

> > Source/WebCore/platform/mac/WebPlaybackControlsManager.mm:104
> > +    completionHandler(@[], nil);
> 
> Ditto.
> 

Fixed.

> > Source/WebCore/platform/mac/WebPlaybackControlsManager.mm:132
> > +    _audioFunctionBarMediaSelectionOptions = audioOptions;
> 
> I’d make a copy of audioOptions here (and change the property from “retain”
> to “copy”.
> 

See above.

> > Source/WebCore/platform/mac/WebPlaybackControlsManager.mm:149
> > +    if (audioMediaSelectionOption && self.audioFunctionBarMediaSelectionOptions)
> 
> We normally don’t use trivial accessors from within a class’s own
> implementation, but just use the ivar directly.
> 

Fixed.

> > Source/WebCore/platform/mac/WebPlaybackControlsManager.mm:162
> > +    _legibleFunctionBarMediaSelectionOptions = legibleOptions;
> 
> I’d make a copy of legibleOptions here (and change the property from
> “retain” to “copy”.


See above.

Thank you!!
Comment 5 Beth Dakin 2016-11-16 12:24:21 PST
https://trac.webkit.org/changeset/208802
Comment 6 Csaba Osztrogonác 2016-11-16 12:50:31 PST
(In reply to comment #5)
> https://trac.webkit.org/changeset/208802

It broke the Sierra build, see build.webkit.org for details.
Comment 7 Beth Dakin 2016-11-16 12:59:05 PST
Build fix: https://trac.webkit.org/changeset/208805
Comment 8 Beth Dakin 2016-11-16 13:10:45 PST
Another fix: https://trac.webkit.org/changeset/208809