Bug 83360 – [BlackBerry] Upstreaming BlackBerry-specific changes to PluginView

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKit BlackBerry

[Charles Wei]

Need to upstream BlackBerry-specific events in PluginView.

[Charles Wei]

Created attachment 136001 [details]
Patch

[Rob Buis]

Comment on attachment 136001 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136001&action=review

Still some stuff to clear up.

> Source/WebCore/plugins/PluginView.cpp:186
> +#endif

How about like this:
if (!m_plugin
#if !PLATFORM(BLACKBERRY)
       || m_isWindowed // On BB windowed plugins want to get events.
#endif
    )
    return;

> Source/WebCore/plugins/PluginView.cpp:202
> +    else if (event->type() == eventNames().DOMFocusOutEvent)

Any difference between DOMFocusOutEvent and focusoutEvent below?

[Antonio Gomes]

Comment on attachment 136001 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136001&action=review

>> Source/WebCore/plugins/PluginView.cpp:186
>> +#endif
> 
> How about like this:
> if (!m_plugin
> #if !PLATFORM(BLACKBERRY)
>        || m_isWindowed // On BB windowed plugins want to get events.
> #endif
>     )
>     return;

I would even go with 

if (!m_plugin)
    return;

#if platform(blackberry)
if (m_isWindowed)
    return;
#endif

> Source/WebCore/plugins/PluginView.cpp:300
> +#if PLATFORM(BLACKBERRY)
> +        if (!SecurityPolicy::shouldHideReferrer(m_url, m_parentFrame->document()->baseURL()))
> +            frameLoadRequest.resourceRequest().setHTTPReferrer(m_parentFrame->loader()->outgoingReferrer());
> +#endif

does not seem event handling related (?)

> Source/WebCore/plugins/PluginView.cpp:462
> +    // Don't let a plugin start any loads if it no longer has a page associated with it.
> +    if (!m_parentFrame->page())
> +        return;

it does not seem blackberry specific. Was it a crash issue? Is it possible to come with a layout test?

[Charles Wei]

Comment on attachment 136001 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136001&action=review

>>> Source/WebCore/plugins/PluginView.cpp:186
>>> +#endif
>> 
>> How about like this:
>> if (!m_plugin
>> #if !PLATFORM(BLACKBERRY)
>>        || m_isWindowed // On BB windowed plugins want to get events.
>> #endif
>>     )
>>     return;
> 
> I would even go with 
> 
> if (!m_plugin)
>     return;
> 
> #if platform(blackberry)
> if (m_isWindowed)
>     return;
> #endif

that's logically wrong, Antonio :-)
it should be 

#if !PLATFORM(BLACKBERRY)
if (m_isWindowed)       //On BlackBerry, windowed plugins want to get events also.
    return;
#endif

>> Source/WebCore/plugins/PluginView.cpp:202
>> +    else if (event->type() == eventNames().DOMFocusOutEvent)
> 
> Any difference between DOMFocusOutEvent and focusoutEvent below?

DOMFocusOutEvent are DOM2 events, and focusoutEvent are DOM3.  In webkit, whenever there's a DOMFocusOutEvent sent out, there will be a focusOutEvent sent out also.  WebKit and DOM event spec will obsolete DOMFocusOutEvent, DOMFocusInEvent eventually.   So we will need to remove them here also.
Thanks for your pick this out.

>> Source/WebCore/plugins/PluginView.cpp:300
>> +#endif
> 
> does not seem event handling related (?)

Not event handling related,  it's loading related. Maybe the title of this bug is not appropriate. It should read "Upstream BlackBerry-specific change to PluginView".  Will do that.

[Charles Wei]

Created attachment 136659 [details]
Patch

[Rob Buis]

Comment on attachment 136659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136659&action=review

> Source/WebCore/plugins/PluginView.h:301
> +    void handleFocusOutEvent();

These two could be removed.

> Source/WebCore/plugins/PluginView.h:404
>          void handleFocusOutEvent();

If we re-enable this?

[Rob Buis]

Comment on attachment 136659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136659&action=review

> Source/WebCore/plugins/PluginView.h:328
> +    Element* getElement() { return m_element; }

I don't think this is typical, element() would be.

[Antonio Gomes]

Comment on attachment 136659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136659&action=review

Lets upstream it by parts: first the event handling stuff, then networking, etc.

> Source/WebCore/ChangeLog:8
> +        No new testsr, initial checkin of BlackBerry porting of PluginView,

testsr*

> Source/WebCore/plugins/PluginView.cpp:460
> +    // Don't let a plugin start any loads if it no longer has a page associated with it.
> +    if (!m_parentFrame->page())
> +        return;
> +

this is hard to accept without a test case of failure or a crash. no tests...

> Source/WebCore/plugins/PluginView.h:88
> +#if (PLATFORM(QT) && USE(ACCELERATED_COMPOSITING) && ENABLE(NETSCAPE_PLUGIN_API) && (defined(XP_UNIX) )) || PLATFORM(BLACKBERRY)

I think it should be like:

#if (PLATFORM(QT) || PLATFORM(BLACKBERRY) && USE(ACCELERATED_COMPOSITING) && ENABLE(NETSCAPE_PLUGIN_API) && defined(XP_UNIX)

[Rob Buis]

Looking at this again and after discussion with Antonio, I think this is easier to review if it is split into multiple patches. We see here changes having to do with event handling, networking, graphics, a crash fix, all lumped together.

[Rob Buis]

(In reply to comment #8)
> > Source/WebCore/plugins/PluginView.cpp:460
> > +    // Don't let a plugin start any loads if it no longer has a page associated with it.
> > +    if (!m_parentFrame->page())
> > +        return;
> > +
> 
> this is hard to accept without a test case of failure or a crash. no tests...

An ASSERT may be more acceptible, until we prove that there is a problem.

[Antonio Gomes]

Comment on attachment 136659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136659&action=review

> Source/WebCore/plugins/PluginView.cpp:73
> +#include "TouchEvent.h"

should it get guarded against enabled(touch)?

does any other platform support touch events in flash?

> Source/WebCore/plugins/PluginView.cpp:185
> +#if !PLATFORM(BLACKBERRY) // On BlackBerry, even windowed plugins want to get events.
> +    if (m_isWindowed)
>          return;
> +#endif

maybe add a static local fucntion:

bool bypassEventsToWindowedPlugin()
{
if platform(bb)
   return false;
endif

return true;
}

> Source/WebCore/plugins/PluginView.cpp:196
> +#if PLATFORM(BLACKBERRY) && ENABLE(NETSCAPE_PLUGIN_API)

is not #if platform(bb) enough here?

> Source/WebCore/plugins/PluginView.cpp:200
> +    else if (event->interfaceName() == eventNames().interfaceForTouchEvent)
> +        handleTouchEvent(static_cast<TouchEvent*>(event));

do we need to guard enable(touch)?

> Source/WebCore/plugins/PluginView.cpp:489
> +#if PLATFORM(BLACKBERRY)
> +            ResourceRequest resourceRequest = request->frameLoadRequest().resourceRequest();
> +            resourceRequest.setIsRequestedByPlugin(true);
> +            m_parentFrame->loader()->load(resourceRequest, targetFrameName, false);
> +#else
>              m_parentFrame->loader()->load(request->frameLoadRequest().resourceRequest(), targetFrameName, false);
> +#endif

maybe this should get re-worked. see my comments in bug 83447