[Webkit-unassigned] [Bug 164789] Implement WebPlaybackControlsManager

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=164789

--- Comment #4 from Beth Dakin <bdakin at apple.com> ---
(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
> > + at interface WebPlaybackControlsManager : NSObject<AVFunctionBarPlaybackControlsControlling> {
> 
> Missing space before the <.
> 

Fixed!

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

Added nonatomic.

> > Source/WebCore/platform/mac/WebPlaybackControlsManager.h:70
> > + at property (nonatomic, retain, readwrite) NSArray<AVFunctionBarMediaSelectionOption *> *audioFunctionBarMediaSelectionOptions;
> > + at 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!!

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20161116/ee7955c5/attachment.html>