Bug 164789 – Implement WebPlaybackControlsManager

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Beth Dakin]

Implement WebPlaybackControlsManager

[Beth Dakin]

Created attachment 294876 [details]
Patch

[WebKit Commit Bot]

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 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”.

[Beth Dakin]

(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!!

[Beth Dakin]

https://trac.webkit.org/changeset/208802

[Csaba Osztrogonác]

(In reply to comment #5)
> https://trac.webkit.org/changeset/208802

It broke the Sierra build, see build.webkit.org for details.

[Beth Dakin]

Build fix: https://trac.webkit.org/changeset/208805

[Beth Dakin]

Another fix: https://trac.webkit.org/changeset/208809