Update AirPlay route handling
Created attachment 248353 [details] Proposed patch.
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.
Created attachment 248418 [details] Updated patch.
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.
Created attachment 248447 [details] Updated patch.
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.
Created attachment 248451 [details] Updated patch.
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.
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
Created attachment 248454 [details] Patch for landing.
Created attachment 248475 [details] Rebased patch for landing
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.
Committed r181423 - http://trac.webkit.org/changeset/181423
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.
(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.