Bug 164789

Created attachment 294876 [details] Patch

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

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

(In reply to comment #5) > https://trac.webkit.org/changeset/208802 It broke the Sierra build, see build.webkit.org for details.

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

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