[iOS] When simultaneously exiting-and-entering fullscreen, WebVideoFullscreenManager/Proxy becomes confused about what video element it represents.
Created attachment 250681 [details] Patch
Comment on attachment 250681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250681&action=review Need to upload a new patch that is rebased. > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:46 > +class WebVideoFullscreenModelContext : public RefCounted<WebVideoFullscreenModelContext>, public WebCore::WebVideoFullscreenModel, public WebCore::WebVideoFullscreenChangeObserver { Can any of this inheritance be private rather than public? Can this class be marked final. > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:48 > + static RefPtr<WebVideoFullscreenModelContext> create(WebVideoFullscreenManagerProxy* manager, uint64_t contextId) Can the manager be null? If not, this should be a reference. This should return a Ref<>, not a RefPtr<>. > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:52 > + virtual ~WebVideoFullscreenModelContext() { } This should not be needed; it should be created automatically by the compiler without declaring it. > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:65 > + { Trailing whitespace here should be omitted. > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:231 > + if (m_contextMap.contains(contextId)) > + return m_contextMap.get(contextId); This function does a double hash table lookup. Instead of using contains, get, and set, this function should use add. > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:234 > + RefPtr<WebCore::WebVideoFullscreenInterfaceAVKit> interface = adoptRef(new WebVideoFullscreenInterfaceAVKit()); We try to do all adoptRef in functions in a class. I suggest adding a create function to this class. > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:242 > + return WTF::move(tuple); I don’t think this move is needed. Here’s my suggested different way to write this entire "ensure" function: void addResult = m_contextMap.add(contextId, ModelInterfaceTuple(); if (addResult.isNewEntry) addResult.iterator->value = createModelAndInterface(contextId); return addResult.iterator->value; Then we would write a separate create function that looks something like this: auto context = WebVideoFullscreenModelContext::create(*this, contextId); auto interface = WebVideoFullscreenInterfaceAVKit::create(context.get(), context.get()); return std::make_tuple(context, interface); > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:245 > +WebVideoFullscreenModelContext* WebVideoFullscreenManagerProxy::ensureModel(uint64_t contextId) This should return a reference, not a pointer. > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:250 > +WebCore::WebVideoFullscreenInterfaceAVKit* WebVideoFullscreenManagerProxy::ensureInterface(uint64_t contextId) This should return a reference, not a pointer.
Comment on attachment 250681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250681&action=review >> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:46 >> +class WebVideoFullscreenModelContext : public RefCounted<WebVideoFullscreenModelContext>, public WebCore::WebVideoFullscreenModel, public WebCore::WebVideoFullscreenChangeObserver { > > Can any of this inheritance be private rather than public? Can this class be marked final. This causes a cast from (e.g.) WebVideoFullscreenModelContext* to WebVideoFullscreenChangeObserver* to fail to compile. It can be marked as final, however. >> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:48 >> + static RefPtr<WebVideoFullscreenModelContext> create(WebVideoFullscreenManagerProxy* manager, uint64_t contextId) > > Can the manager be null? If not, this should be a reference. > > This should return a Ref<>, not a RefPtr<>. Ok & ok. >> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:52 >> + virtual ~WebVideoFullscreenModelContext() { } > > This should not be needed; it should be created automatically by the compiler without declaring it. Without it, there is a compiler warning about a missing virtual destructor. >> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:231 >> + return m_contextMap.get(contextId); > > This function does a double hash table lookup. Instead of using contains, get, and set, this function should use add. Ah, I avoided this (when I was using Ref<>s everywhere) because I'd have to create the objects up front. But I guess there's no need to redefine ModelInterfaceTuple in terms of Ref<>s, and leave them as RefPtr<>s. >> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:234 >> + RefPtr<WebCore::WebVideoFullscreenInterfaceAVKit> interface = adoptRef(new WebVideoFullscreenInterfaceAVKit()); > > We try to do all adoptRef in functions in a class. I suggest adding a create function to this class. Will Do. >> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:242 >> + return WTF::move(tuple); > > I don’t think this move is needed. Here’s my suggested different way to write this entire "ensure" function: > > void addResult = m_contextMap.add(contextId, ModelInterfaceTuple(); > if (addResult.isNewEntry) > addResult.iterator->value = createModelAndInterface(contextId); > return addResult.iterator->value; > > Then we would write a separate create function that looks something like this: > > auto context = WebVideoFullscreenModelContext::create(*this, contextId); > auto interface = WebVideoFullscreenInterfaceAVKit::create(context.get(), context.get()); > return std::make_tuple(context, interface); Ok. >> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:245 >> +WebVideoFullscreenModelContext* WebVideoFullscreenManagerProxy::ensureModel(uint64_t contextId) > > This should return a reference, not a pointer. Ok. >> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:250 >> +WebCore::WebVideoFullscreenInterfaceAVKit* WebVideoFullscreenManagerProxy::ensureInterface(uint64_t contextId) > > This should return a reference, not a pointer. Ok. I'll make the matching changes over on the WebFullscreenManager side.
Created attachment 250741 [details] Patch
Created attachment 250835 [details] Patch
Created attachment 250864 [details] Patch
rdar://problem/18913948
Created attachment 250868 [details] Patch
Created attachment 250876 [details] Patch
Comment on attachment 250876 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250876&action=review > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:71 > + static Ref<WebVideoFullscreenInterfaceAVKit> create() { return adoptRef(*new WebVideoFullscreenInterfaceAVKit()); } We normally don't put these on one line. > Source/WebCore/platform/ios/WebVideoFullscreenModelVideoElement.h:48 > + static RefPtr<WebVideoFullscreenModelVideoElement> create() { return adoptRef(*new WebVideoFullscreenModelVideoElement()); } Unwrap. > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:243 > + auto& model = std::get<0>(tuple); > + auto& interface = std::get<1>(tuple); auto isn't helping me here. Nor is the tuple, raeally. How do I know what <0> and <1> are? > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:298 > + return *std::get<0>(ensureModelAndInterface(contextId)); Magic 0 > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:303 > + return *std::get<1>(ensureModelAndInterface(contextId)); Magic 1 > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:169 > + auto model = std::get<0>(tuple); > + auto interface = std::get<1>(tuple); Ditto.
(In reply to comment #10) > Comment on attachment 250876 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=250876&action=review > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:71 > > + static Ref<WebVideoFullscreenInterfaceAVKit> create() { return adoptRef(*new WebVideoFullscreenInterfaceAVKit()); } > > We normally don't put these on one line. > > > Source/WebCore/platform/ios/WebVideoFullscreenModelVideoElement.h:48 > > + static RefPtr<WebVideoFullscreenModelVideoElement> create() { return adoptRef(*new WebVideoFullscreenModelVideoElement()); } > > Unwrap. > > > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:243 > > + auto& model = std::get<0>(tuple); > > + auto& interface = std::get<1>(tuple); > > auto isn't helping me here. Nor is the tuple, raeally. How do I know what > <0> and <1> are? How do you know what pair.first and pair.second are? That said, this could probably be changed to: RefPtr<WebVideoFullscreenModelContext> model; RefPtr<WebVideoFullscreenInterfaceAVKit> interface; std::tie(model, interface) = tuple; > > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:298 > > + return *std::get<0>(ensureModelAndInterface(contextId)); > > Magic 0 Think of it as equivalent to `pair.first`. In C++14, this would be std::get<WebVideoFullscreenModelContext*>(...). > > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:303 > > + return *std::get<1>(ensureModelAndInterface(contextId)); > > Magic 1 Think of it as equivalent to `pair.second`. In C++14, this would be std::get<WebVideoFullscreenInterfaceAVKit*>(...). > > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:169 > > + auto model = std::get<0>(tuple); > > + auto interface = std::get<1>(tuple); > > Ditto. Same as the first.
Committed r182914: <http://trac.webkit.org/changeset/182914>