Summary: | Mac: Adopt the new version of AVOutputDeviceMenuController's showMenuForRect method | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ada Chan <adachan> | ||||||||||
Component: | Media | Assignee: | Ada Chan <adachan> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | Keywords: | InRadar | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Ada Chan
2016-02-29 10:35:49 PST
Created attachment 272503 [details]
Patch
Comment on attachment 272503 [details]
Patch
Need to update AVKitSPI.h. Will send an updated patch soon.
Created attachment 272513 [details]
Patch
Comment on attachment 272513 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272513&action=review > Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:277 > + for (size_t i = 0; i < m_clientState.size(); ++i) { > + auto& state = m_clientState[i]; > + Should use a modern for loop: for (auto& state : m_clientState) { > Source/WebCore/dom/Document.cpp:6890 > + auto it = m_idToClientMap.find(clientId); > + if (it == m_idToClientMap.end()) > + return; > + > + it->value->customPlaybackActionSelected(); When a map’s values are pointers, you can do this in a way that’s less complex with get: if (auto* client = m_idToClientMap.get(clientId)) client->customPlaybackActionSelected(); I have no idea why the functions above don’t do it this simpler way. > Source/WebCore/html/HTMLMediaElement.cpp:5217 > + return String(); There’s now a more terse way of writing the null string: return { }; > Source/WebCore/html/MediaElementSession.cpp:53 > +#if PLATFORM(MAC) > +#include "HTMLMediaElementEnums.h" > +#endif This seems peculiar. The code below is in #if ENABLE(WIRELESS_PLAYBACK_TARGET) && !PLATFORM(IOS) so it doesn't seem right to put the #include in #if PLATFORM(MAC). Maybe make the include unconditional? Or match the conditional around the code below exactly? > Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.h:73 > + WEBCORE_EXPORT void setExternalPlayback(bool enabled, ExternalPlaybackTargetType, WTF::String localizedDeviceName) override; A little bit strange to have the argument be WTF::String rather than const WTF::String&, but that’s in the base class too, not just here. > Source/WebKit/mac/WebView/WebView.mm:8743 > + [self _devicePicker]->showPlaybackTargetPicker(clientId, rectInScreenCoordinates, hasVideo, String()); Can use { } here instead of String(). Thanks for the feedback! (In reply to comment #5) > Comment on attachment 272513 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=272513&action=review > > > Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:277 > > + for (size_t i = 0; i < m_clientState.size(); ++i) { > > + auto& state = m_clientState[i]; > > + > > Should use a modern for loop: > > for (auto& state : m_clientState) { Fixed. > > > Source/WebCore/dom/Document.cpp:6890 > > + auto it = m_idToClientMap.find(clientId); > > + if (it == m_idToClientMap.end()) > > + return; > > + > > + it->value->customPlaybackActionSelected(); > > When a map’s values are pointers, you can do this in a way that’s less > complex with get: > > if (auto* client = m_idToClientMap.get(clientId)) > client->customPlaybackActionSelected(); > > I have no idea why the functions above don’t do it this simpler way. Fixed. > > > Source/WebCore/html/HTMLMediaElement.cpp:5217 > > + return String(); > > There’s now a more terse way of writing the null string: > > return { }; Fixed. > > > Source/WebCore/html/MediaElementSession.cpp:53 > > +#if PLATFORM(MAC) > > +#include "HTMLMediaElementEnums.h" > > +#endif > > This seems peculiar. The code below is in #if > ENABLE(WIRELESS_PLAYBACK_TARGET) && !PLATFORM(IOS) so it doesn't seem right > to put the #include in #if PLATFORM(MAC). Maybe make the include > unconditional? Or match the conditional around the code below exactly? I made the include unconditional. > > > Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.h:73 > > + WEBCORE_EXPORT void setExternalPlayback(bool enabled, ExternalPlaybackTargetType, WTF::String localizedDeviceName) override; > > A little bit strange to have the argument be WTF::String rather than const > WTF::String&, but that’s in the base class too, not just here. > > > Source/WebKit/mac/WebView/WebView.mm:8743 > > + [self _devicePicker]->showPlaybackTargetPicker(clientId, rectInScreenCoordinates, hasVideo, String()); > > Can use { } here instead of String(). Fixed. I had to add a forward declaration for the SPI in MediaPlaybackTargetPickerMac.mm to get things to compile for systems without that SPI. Will post an updated patch soon. Created attachment 272581 [details]
Patch
Removing the forward declaration since it's no longer a requirement to support building without the SPI in the headers. Created attachment 272597 [details]
Patch
Committed: http://trac.webkit.org/changeset/197429 |