Bug 83360 - [BlackBerry] Upstreaming BlackBerry-specific changes to PluginView
Summary: [BlackBerry] Upstreaming BlackBerry-specific changes to PluginView
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Charles Wei
URL:
Keywords:
Depends on: 83756 83767 83772
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-06 03:47 PDT by Charles Wei
Modified: 2014-01-28 08:15 PST (History)
4 users (show)

See Also:


Attachments
Patch (12.73 KB, patch)
2012-04-06 04:13 PDT, Charles Wei
no flags Details | Formatted Diff | Diff
Patch (18.98 KB, patch)
2012-04-11 04:42 PDT, Charles Wei
tonikitoo: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charles Wei 2012-04-06 03:47:53 PDT
Need to upstream BlackBerry-specific events in PluginView.
Comment 1 Charles Wei 2012-04-06 04:13:51 PDT
Created attachment 136001 [details]
Patch
Comment 2 Rob Buis 2012-04-06 05:57:02 PDT
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?
Comment 3 Antonio Gomes 2012-04-09 20:56:58 PDT
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?
Comment 4 Charles Wei 2012-04-11 03:41:04 PDT
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.
Comment 5 Charles Wei 2012-04-11 04:42:51 PDT
Created attachment 136659 [details]
Patch
Comment 6 Rob Buis 2012-04-11 07:22:43 PDT
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?
Comment 7 Rob Buis 2012-04-11 07:31:54 PDT
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.
Comment 8 Antonio Gomes 2012-04-11 07:48:15 PDT
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)
Comment 9 Rob Buis 2012-04-11 07:49:52 PDT
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.
Comment 10 Rob Buis 2012-04-11 07:50:23 PDT
(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.
Comment 11 Antonio Gomes 2012-04-11 07:53:55 PDT
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