Bug 143680 – [iOS] When simultaneously exiting-and-entering fullscreen, WebVideoFullscreenManager/Proxy becomes confused about what video element it represents.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[iOS] When simultaneously exiting-and-entering fullscreen, WebVideoFullscreenManager/Proxy becomes confused about what video element it represents.

[Jer Noble]

Created attachment 250681 [details]
Patch

[Darin Adler]

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.

[Jer Noble]

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.

[Jer Noble]

Created attachment 250741 [details]
Patch

[Jer Noble]

Created attachment 250835 [details]
Patch

[Jer Noble]

Created attachment 250864 [details]
Patch

[Jer Noble]

rdar://problem/18913948

[Jer Noble]

Created attachment 250868 [details]
Patch

[Jer Noble]

Created attachment 250876 [details]
Patch

[Simon Fraser (smfr)]

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.

[Jer Noble]

(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.

[Jer Noble]

Committed r182914: <http://trac.webkit.org/changeset/182914>