Bug 142541 – [Mac] Update AirPlay handling

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

Update AirPlay route handling

[Eric Carlson]

Created attachment 248353 [details]
Proposed patch.

[WebKit Commit Bot]

Attachment 248353 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/avfoundation/AVPlaybackTargetMac.mm:60:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/AVPlaybackTargetMac.mm:60:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 248418 [details]
Updated patch.

[WebKit Commit Bot]

Attachment 248418 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/avfoundation/AVPlaybackTargetMac.mm:60:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/AVPlaybackTargetMac.mm:60:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 248447 [details]
Updated patch.

[WebKit Commit Bot]

Attachment 248447 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/avfoundation/MediaPlaybackTargetMac.mm:60:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/MediaPlaybackTargetMac.mm:60:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/mac/WebMediaPlaybackTargetPickerProxyMac.mm:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 248451 [details]
Updated patch.

[WebKit Commit Bot]

Attachment 248451 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/avfoundation/MediaPlaybackTargetMac.mm:60:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/MediaPlaybackTargetMac.mm:60:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 248451 [details]
Updated patch.

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

> Source/WebCore/dom/Document.cpp:6497
> +    for (auto targetClient : m_playbackTargetClients) {

I would write this as 'auto* client : m_playbackTargetClients'

> Source/WebCore/dom/Document.cpp:6513
> +    for (auto client : m_playbackTargetClients)

I would write this as 'auto* client : m_playbackTargetClients'

> Source/WebCore/dom/Document.cpp:6521
> +    for (auto client : m_playbackTargetClients) {

I would write this as 'auto* client : m_playbackTargetClients'

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2710
> +    bool wirelessTarget = false;
> +
> +    wirelessTarget = m_avPlayer.get().externalPlaybackActive;

No need to use three lines for this.

> Source/WebKit2/UIProcess/mac/WebMediaPlaybackTargetPickerProxyMac.mm:162
> +- (void)observeValueForKeyPath:keyPath ofObject:(id)object change:(NSDictionary *)change context:(void *)context

I think you are missing the type of keyPath.  It should probably be (id)keyPath

[Eric Carlson]

Created attachment 248454 [details]
Patch for landing.

[Eric Carlson]

Created attachment 248475 [details]
Rebased patch for landing

[WebKit Commit Bot]

Attachment 248475 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/avfoundation/MediaPlaybackTargetMac.mm:60:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/MediaPlaybackTargetMac.mm:60:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Committed r181423 - http://trac.webkit.org/changeset/181423

[Joseph Pecoraro]

Comment on attachment 248451 [details]
Updated patch.

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

Cool! I realize this landed, I just have some questions / comments that don't really need to be addressed unless you desire to.

> Source/WebCore/page/ChromeClient.h:449
> +    virtual void showPlaybackTargetPicker(const WebCore::IntPoint&, bool) { }

Nit: It is unclear what this bool flag is for, it would be nice to have a name in the header.

> Source/WebCore/page/Page.cpp:1703
> +

Nit: Empty line could be removed.

> Source/WebCore/page/Page.cpp:1729
> +    for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext()) {
> +        Document* document = frame->document();
> +        if (frame->document() == m_documentRequestingPlaybackTargetPicker) {
> +            documentThatRequestedPicker = document;
> +            continue;
> +        }
> +        frame->document()->didChoosePlaybackTarget(target);
> +    }
> +
> +    if (documentThatRequestedPicker)
> +        documentThatRequestedPicker->didChoosePlaybackTarget(target);

Any particular reason this delays to call the documentThatRequested last? Is there a behavior here that is significant that should be documented? ChangeLog doesn't say.

> Source/WebCore/page/Page.cpp:1755
> +
> +

Nit: extra newline can be removed.

> Source/WebKit2/Shared/mac/WebCoreArgumentCodersMac.mm:42
> +#import <WebCore/SoftLinking.h>
> +#import <objc/runtime.h>

Are these needed? I don't see any objc_* or SOFT_LINK macros.

> Source/WebKit2/UIProcess/WebMediaPlaybackTargetPickerProxy.cpp:40
> +    m_client = 0;

nullptr

> Source/WebKit2/UIProcess/mac/WebMediaPlaybackTargetPickerProxyMac.h:38
> +namespace WebCore {
> +class MediaPlaybackTarget;
> +class FloatRect;
> +}

These should already be covered by WebMediaPlaybackTargetPickerProxy.h.

> Source/WebKit2/UIProcess/mac/WebMediaPlaybackTargetPickerProxyMac.h:45
> +class WebMediaPlaybackTargetPickerProxyMac : public WebMediaPlaybackTargetPickerProxy {

Nit: You can mark the class as "final"

> Source/WebKit2/UIProcess/mac/WebMediaPlaybackTargetPickerProxyMac.mm:55
> +- (id)initWithCallback:(WebMediaPlaybackTargetPickerProxyMac*)callback;

Nit: "instancetype" instead of "id".

> Source/WebKit2/UIProcess/mac/WebMediaPlaybackTargetPickerProxyMac.mm:57
> +- (void)observeValueForKeyPath:keyPath ofObject:(id)object change:(NSDictionary *)change context:(void *)context;

Nit: You should provide a type of "keyPath" instead of leaving it off, which I think means (id).

> Source/WebKit2/UIProcess/mac/WebMediaPlaybackTargetPickerProxyMac.mm:110
> +        [m_devicePickerMenuController.get() addObserver:m_devicePickerMenuControllerDelegate.get() forKeyPath:@"externalOutputDeviceAvailable" options:NSKeyValueObservingOptionNew context:nullptr];
> +        [m_devicePickerMenuController.get() addObserver:m_devicePickerMenuControllerDelegate.get() forKeyPath:@"externalOutputDevicePicked" options:NSKeyValueObservingOptionNew context:nullptr];

These strings look like magic. Where do they come from? Can they be made constants to avoid accidental typos?

> Source/WebKit2/UIProcess/mac/WebMediaPlaybackTargetPickerProxyMac.mm:166
> +    UNUSED_PARAM(change);
> +    UNUSED_PARAM(context);
> +    UNUSED_PARAM(object);

I don't think we need UNUSED_PARAM in ObjC but it can't hurt to leave them in.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:1107
> +    //     IntPoint scrollPositionAtNewScale = mainFrameView()->rootViewToContents(-centerInViewCoordinates);

Stale comment? Remove.

[Eric Carlson]

(In reply to comment #14)
> Comment on attachment 248451 [details]
>
Thanks for the detailed post-review! I fixed all of the issues you pointed out and landed some changes that were in the last patch but which didn't make the initial commit because of a bungled merge in r181442.