Bug 156358

Summary: WebPlaybackControlsManager should support mediaSelectionOptions
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, commit-queue, eric.carlson, jer.noble
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch jer.noble: review+

Description Beth Dakin 2016-04-07 13:32:01 PDT
WebPlaybackControlsManager should support mediaSelectionOptions

rdar://problem/25048743
Comment 1 Beth Dakin 2016-04-07 13:43:03 PDT
Created attachment 275925 [details]
Patch
Comment 2 WebKit Commit Bot 2016-04-07 13:44:24 PDT
Attachment 275925 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:101:  Should not have spaces around = in property synthesis.  [whitespace/property] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:102:  Should not have spaces around = in property synthesis.  [whitespace/property] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:103:  Should not have spaces around = in property synthesis.  [whitespace/property] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:104:  Should not have spaces around = in property synthesis.  [whitespace/property] [4]
Total errors found: 4 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Beth Dakin 2016-04-07 13:48:04 PDT
Created attachment 275926 [details]
Patch
Comment 4 Jer Noble 2016-04-07 14:02:40 PDT
Comment on attachment 275926 [details]
Patch

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

r=me, with possible nits.

> Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:269
> +        [webOption setLocalizedDisplayName:name];

I could be wrong, but I feel like the new preferred style guides for this kind of thing would read:

webOption.get().localizedDisplayName = name;

> Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:280
> +    [controlsManager setAudioMediaSelectionOptions:webOptions.get()];

...and:

controlsManager.get().audioMediaSelectionOptions = webOptions.get();

> Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:282
> +        [controlsManager setCurrentAudioMediaSelectionOption:[webOptions objectAtIndex:static_cast<NSUInteger>(selectedIndex)]];

...and:

controlsManager.get().currentAudioMediaSelectionOption = [webOptions objectAtIndex:static_cast<NSUInteger>(selectedIndex)]];

etc.

But I may not be correct about that use of style.  Aside from than that, good patch!
Comment 5 Beth Dakin 2016-04-07 14:33:09 PDT
Thanks Jer! http://trac.webkit.org/changeset/199184

My understanding has been that most people prefer not to use dot syntax with RetainPtrs, but it really is something we should address in the style guide. I chose not to address the comment for now.

http://trac.webkit.org/changeset/199184