[Webkit-unassigned] [Bug 164789] Implement WebPlaybackControlsManager

[bugzilla-daemon at webkit.org]

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

mitz at webkit.org <mitz at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #294876|review?                     |review+
              Flags|                            |

--- Comment #3 from mitz at webkit.org <mitz at webkit.org> ---
Comment on attachment 294876
  --> https://bugs.webkit.org/attachment.cgi?id=294876
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
> + at interface WebPlaybackControlsManager : NSObject<AVFunctionBarPlaybackControlsControlling> {

Missing space before the <.

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

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

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

-- 
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/e53cca0c/attachment.html>