Implementing hasLiveStreamingContent will enable the live streaming UI.
<rdar://problem/16358957>
Created attachment 227474 [details] Patch
Simon, One question, in the IPC interface I convert a TimeRange to a Vector instead of creating an encoder/decoder for TimeRange. Would it be preferable to create the encoder/decoder?
Comment on attachment 227474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227474&action=review > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:56 > + virtual void setSeekableRangesVector(Vector<std::pair<double, double>>); Pass a reference unless you're using std::move. > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:70 > + Blank space. > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:71 > + for (auto range : ranges) const auto& > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:72 > + timeRanges->add(range.first, range.second); You should add some basic validity checking here (check that range end is > range start, and any other invariants). Basically you should assume that data coming over IPC is unsafe. > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.h:73 > + virtual void setSeekableRanges(WebCore::TimeRanges&) override; const WebCore::TimeRanges&
(In reply to comment #4) > (From update of attachment 227474 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=227474&action=review > > > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:56 > > + virtual void setSeekableRangesVector(Vector<std::pair<double, double>>); > > Pass a reference unless you're using std::move. Passing Vector<> by reference is unprecedented in IPC interface, so I'll change the one call site to use std::move. > > > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:70 > > + > > Blank space. Removed. > > > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:71 > > + for (auto range : ranges) > > const auto& Done. > > > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:72 > > + timeRanges->add(range.first, range.second); > > You should add some basic validity checking here (check that range end is > range start, and any other invariants). Basically you should assume that data coming over IPC is unsafe. Done > > > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.h:73 > > + virtual void setSeekableRanges (WebCore::TimeRanges&) override; > > const WebCore::TimeRanges& Done.
Created attachment 227512 [details] Patch
Comment on attachment 227512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227512&action=review This looks OK to me modulo my minor comments, but I am now a WK2 reviewer. > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:113 > + m_videoFullscreenInterface->setSeekableRanges(*m_mediaElement->seekable()); This is OK for now, but please add a FIXME and file a bug about coming up with a more efficient method of updating ranges than polling 3 times per second. > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:79 > + setSeekableRanges(*timeRanges); Could you pass the Vector<> directly instead of creating a temporary TimeRanges?
(In reply to comment #7) > (From update of attachment 227512 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=227512&action=review > > This looks OK to me modulo my minor comments, but I am now a WK2 reviewer. > By which I meant I am *not* a WK2 reviewer, of course.
Comment on attachment 227512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227512&action=review > Source/WebCore/platform/ios/WebVideoFullscreenInterface.h:33 > +#import <WebCore/TimeRanges.h> Don't include the header unless you need the implementation. Instead, forward-declare the class. Same with PlatformLayer.h if that's fixable. We have huge build time problems because of over-eager #includes. > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:365 > +void WebVideoFullscreenInterfaceAVKit::setSeekableRanges(const WebCore::TimeRanges& timeRanges) Need the WebCore::? > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:112 > m_videoFullscreenInterface->setCurrentTime(m_mediaElement->currentTime(), [[NSProcessInfo processInfo] systemUptime]); What is the [[NSProcessInfo processInfo] systemUptime] thing for? Weird. > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:56 > + virtual void setSeekableRangesVector(Vector<std::pair<double, double>>); Vector&?
(In reply to comment #7) > (From update of attachment 227512 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=227512&action=review > > This looks OK to me modulo my minor comments, but I am now a WK2 reviewer. > > > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:113 > > + m_videoFullscreenInterface->setSeekableRanges(*m_mediaElement->seekable()); > > This is OK for now, but please add a FIXME and file a bug about coming up with a more efficient method of updating ranges than polling 3 times per second. I added a change to use the progressEvent. > > > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:79 > > + setSeekableRanges(*timeRanges); > > Could you pass the Vector<> directly instead of creating a temporary TimeRanges? The is one of a couple solutions. 1) Create an encoder/decoder for TimeRanges to they can cross IPC boundary. Thus removing the vector. 2) Pass Vector<> directly, which will unnecessarily convert TimeRanges to a Vector<> in WK1. I wrote https://bugs.webkit.org/show_bug.cgi?id=130784 to follow up on this.
(In reply to comment #9) > (From update of attachment 227512 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=227512&action=review > > > Source/WebCore/platform/ios/WebVideoFullscreenInterface.h:33 > > +#import <WebCore/TimeRanges.h> > > Don't include the header unless you need the implementation. Instead, forward-declare the class. Same with PlatformLayer.h if that's fixable. > > We have huge build time problems because of over-eager #includes. Removed include of TimeRanges.h and added it to the appropriate .m files. Removed include of PlatformLayer.h > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:365 > > +void WebVideoFullscreenInterfaceAVKit::setSeekableRanges(const WebCore::TimeRanges& timeRanges) > > Need the WebCore::? Removed. > > > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:112 > > m_videoFullscreenInterface->setCurrentTime(m_mediaElement->currentTime(), [[NSProcessInfo processInfo] systemUptime]); > > What is the [[NSProcessInfo processInfo] systemUptime] thing for? Weird. It means, currentTime was X when systemUptime was Y. You can use this and the current rate to extrapolate the current time more accurately and continuously. Then you just periodically update this to minimize drift. > > > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:56 > > + virtual void setSeekableRangesVector(Vector<std::pair<double, double>>); > > Vector&? Aha! The IPC interface can be declared without the reference, but this implementation can still use a reference because the call syntax is the same. Done.
(In reply to comment #10) > (In reply to comment #7) > > (From update of attachment 227512 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=227512&action=review > > > > This looks OK to me modulo my minor comments, but I am now a WK2 reviewer. > > > > > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:113 > > > + m_videoFullscreenInterface->setSeekableRanges(*m_mediaElement->seekable()); > > > > This is OK for now, but please add a FIXME and file a bug about coming up with a more efficient method of updating ranges than polling 3 times per second. > > I added a change to use the progressEvent. progressEvent isn't the correct event. I've added a FIXME, and filed https://bugs.webkit.org/show_bug.cgi?id=130788
Created attachment 227871 [details] Patch
Comment on attachment 227871 [details] Patch Rejecting attachment 227871 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 227871, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.appspot.com/results/6248663194009600
(In reply to comment #12) > progressEvent isn't the correct event. I've added a FIXME, and filed https://bugs.webkit.org/show_bug.cgi?id=130788 Thanks.
Created attachment 227875 [details] Patch
Comment on attachment 227875 [details] Patch Clearing flags on attachment: 227875 Committed r166312: <http://trac.webkit.org/changeset/166312>