Bug 156358 - WebPlaybackControlsManager should support mediaSelectionOptions
Summary: WebPlaybackControlsManager should support mediaSelectionOptions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-07 13:32 PDT by Beth Dakin
Modified: 2016-04-07 14:33 PDT (History)
4 users (show)

See Also:


Attachments
Patch (8.67 KB, patch)
2016-04-07 13:43 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (8.66 KB, patch)
2016-04-07 13:48 PDT, Beth Dakin
jer.noble: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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