Bug 57418 - [Qt] [Symbian] GraphicsLayer: support plugins on symbian
Summary: [Qt] [Symbian] GraphicsLayer: support plugins on symbian
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Hardware S60 3rd edition
: P3 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on: 34415 35524
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-29 21:25 PDT by Viatcheslav Ostapenko
Modified: 2011-06-27 06:58 PDT (History)
9 users (show)

See Also:


Attachments
Implement graphics layer support for plugins on symbian. (5.44 KB, patch)
2011-03-29 21:31 PDT, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Fix style (5.44 KB, patch)
2011-03-29 22:08 PDT, Viatcheslav Ostapenko
benjamin: review-
Details | Formatted Diff | Diff
Updated patch proposal (5.35 KB, patch)
2011-03-30 09:56 PDT, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Updated patch (5.37 KB, patch)
2011-04-27 10:10 PDT, Viatcheslav Ostapenko
laszlo.gombos: review-
Details | Formatted Diff | Diff
Updated patch (7.30 KB, patch)
2011-05-26 12:10 PDT, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Viatcheslav Ostapenko 2011-03-29 21:25:54 PDT
Add graphics layer support for plugins on symbian.
Comment 1 Viatcheslav Ostapenko 2011-03-29 21:31:18 PDT
Created attachment 87462 [details]
Implement graphics layer support for plugins on symbian.
Comment 2 WebKit Review Bot 2011-03-29 21:33:50 PDT
Attachment 87462 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:113:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Viatcheslav Ostapenko 2011-03-29 22:08:38 PDT
Created attachment 87465 [details]
Fix style
Comment 4 Benjamin Poulain 2011-03-30 04:59:21 PDT
Comment on attachment 87465 [details]
Fix style

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

Is there no way to test this?
There are changes to PluginView which do not look Symbian specific, can't they be commited separately with test coverage?

> Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:93
> +// Qt's GraphicsLayer (GraphicsLayerQt) requires layers to be QGraphicsWidgets

Missing dot at the end of the sentence.
Useless comment anyway, it does not add information/explanation, you can remove it.

> Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:112
> +        clipRect.translate(-clipRect.topLeft());

That looks weird, are you trying to map and object from content coordinate to window coordinate?

> Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:118
> +        rect.adjust(-1, -1, 1, 1);

WHAAAAAAAAT?
That deserve a comment :)

> Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:484
> +            // Trigger layer computation in RenderLayerCompositor

Not period at the end of the sentence.
Useless comment anyway.
Comment 5 Viatcheslav Ostapenko 2011-03-30 08:33:47 PDT
(In reply to comment #4)
> (From update of attachment 87465 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87465&action=review
> 
> Is there no way to test this?
> There are changes to PluginView which do not look Symbian specific, can't they be commited separately with test coverage?

What is not symbian specific? Moving out m_platformLayer definition out of XP_UNIX ifdef? ;)
I have similar patch coming for windows, so all platforms should be covered.
In any case, having unused m_platformLayer member shouldn't harm.

> > Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:93
> > +// Qt's GraphicsLayer (GraphicsLayerQt) requires layers to be QGraphicsWidgets
> 
> Missing dot at the end of the sentence.
> Useless comment anyway, it does not add information/explanation, you can remove it.

Copy paste from Linux patch ;)

> > Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:112
> > +        clipRect.translate(-clipRect.topLeft());
> 
> That looks weird, are you trying to map and object from content coordinate to window coordinate?

Yes. What's weird here? It assigned from frameRect one line upper.
Would this

QRectF clipRect(m_view->frameRect().translated(-m_view->frameRect().topLeft()));

look better? ;)

> > Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:118
> > +        rect.adjust(-1, -1, 1, 1);
> 
> WHAAAAAAAAT?
> That deserve a comment :)

It is converted from QRectF to QRect. I see artifacts with animated transparent plugins when QGraphicsWebView is scaled.
Here is sample: goo.gl/uhgYk

> > Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:484
> > +            // Trigger layer computation in RenderLayerCompositor
> 
> Not period at the end of the sentence.
> Useless comment anyway.

Copy/paste again ;)
Comment 6 Viatcheslav Ostapenko 2011-03-30 09:56:47 PDT
Created attachment 87556 [details]
Updated patch proposal
Comment 7 Viatcheslav Ostapenko 2011-04-12 07:48:44 PDT
Comment on attachment 87556 [details]
Updated patch proposal

Need to update patch with fix similar to https://bugs.webkit.org/show_bug.cgi?id=34415
Comment 8 Viatcheslav Ostapenko 2011-04-27 10:10:50 PDT
Created attachment 91306 [details]
Updated patch
Comment 9 Alexis Menard (darktears) 2011-05-03 14:36:23 PDT
Comment on attachment 91306 [details]
Updated patch

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

> Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:111
> +            clipRect &= option->exposedRect;

Just by curiosity, partial updates are worth it? Do you have a test case where it performs better for the plugin?
Comment 10 Viatcheslav Ostapenko 2011-05-03 15:33:59 PDT
(In reply to comment #9)
> (From update of attachment 91306 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91306&action=review
> 
> > Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:111
> > +            clipRect &= option->exposedRect;
> 
> Just by curiosity, partial updates are worth it?

Why not? ;)

> Do you have a test case where it performs better for the plugin?

Flash plugins could be quite big - bigger than screen size. It makes difference even for simple bit blit. From previous measurements full screen bit blit without alpha on N97 was taking about 12ms. If plugin is bigger, or alpha blending involved, than it will be longer.
Comment 11 Eric Seidel 2011-05-23 15:44:55 PDT
Comment on attachment 91306 [details]
Updated patch

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

> Source/WebCore/plugins/PluginView.h:432
> +#if USE(ACCELERATED_COMPOSITING) && (defined(XP_UNIX) || OS(SYMBIAN))

Why the unix here?

> Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:472
> +        if (m_parentFrame->page()->chrome()->client()->allowsAcceleratedCompositing()
> +            && m_parentFrame->page()->settings()
> +            && m_parentFrame->page()->settings()->acceleratedCompositingEnabled()) {

I wonder if this shouldn't be a helper function.
Comment 12 Eric Seidel 2011-05-23 15:45:47 PDT
The CC'd folks should know about accellerated compositing and PlatformLayer (to tell if this patch makes sense).
Comment 13 Alexis Menard (darktears) 2011-05-23 16:01:08 PDT
Comment on attachment 91306 [details]
Updated patch

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

>> Source/WebCore/plugins/PluginView.h:432
>> +#if USE(ACCELERATED_COMPOSITING) && (defined(XP_UNIX) || OS(SYMBIAN))
> 
> Why the unix here?

+1

>>> Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:111
>>> +            clipRect &= option->exposedRect;
>> 
>> Just by curiosity, partial updates are worth it? Do you have a test case where it performs better for the plugin?
> 
> Why not? ;)

Because computing the exposed rect may be costly in QGV.
Comment 14 Viatcheslav Ostapenko 2011-05-24 07:18:44 PDT
(In reply to comment #11)
> (From update of attachment 91306 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91306&action=review
> 
> > Source/WebCore/plugins/PluginView.h:432
> > +#if USE(ACCELERATED_COMPOSITING) && (defined(XP_UNIX) || OS(SYMBIAN))
> 
> Why the unix here?

Because this patch adds AC plugin layer implementation for symbian to already existing unix implementation. Mac and windows still do not have plugin AC layer implementation for Qt. I just modified existing ifdef (added SYMBIAN).
It's a bit unclear, because I had to break previous #ifdef at line 429 (paintUsingXPixmap is not needed on symbian and other platforms).

> 
> > Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:472
> > +        if (m_parentFrame->page()->chrome()->client()->allowsAcceleratedCompositing()
> > +            && m_parentFrame->page()->settings()
> > +            && m_parentFrame->page()->settings()->acceleratedCompositingEnabled()) {
> 
> I wonder if this shouldn't be a helper function.

What exactly? Big condition or creating of plugin layer? Or both together? ;)
In any case, I'd prefer to leave it as it is to make it similar to unix implementation to make later merging of other changes simpler.
Comment 15 Eric Seidel 2011-05-24 07:27:21 PDT
(In reply to comment #14)
> (In reply to comment #11)
> > (From update of attachment 91306 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=91306&action=review
> > 
> > > Source/WebCore/plugins/PluginView.h:432
> > > +#if USE(ACCELERATED_COMPOSITING) && (defined(XP_UNIX) || OS(SYMBIAN))
> > 
> > Why the unix here?
> 
> Because this patch adds AC plugin layer implementation for symbian to already existing unix implementation. Mac and windows still do not have plugin AC layer implementation for Qt. I just modified existing ifdef (added SYMBIAN).
> It's a bit unclear, because I had to break previous #ifdef at line 429 (paintUsingXPixmap is not needed on symbian and other platforms).

Sounds like we might want to come up with a new define then.  Otherwise we're going to have to add defined(WIN) || everywhere when we add win, and then again fo mac, etc.

> > 
> > > Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:472
> > > +        if (m_parentFrame->page()->chrome()->client()->allowsAcceleratedCompositing()
> > > +            && m_parentFrame->page()->settings()
> > > +            && m_parentFrame->page()->settings()->acceleratedCompositingEnabled()) {
> > 
> > I wonder if this shouldn't be a helper function.
> 
> What exactly? Big condition or creating of plugin layer? Or both together? ;)
> In any case, I'd prefer to leave it as it is to make it similar to unix implementation to make later merging of other changes simpler.

Well, consider this example:

if (shouldUseAccelleratedComposititing(m_parentFrame))

Such is very clear at the call site what it does.  And the actual implementation of the check can use local variables and early return as well.

I'm not saying that splitting ifs off into functions is always a good choice.  But in this one, I think it might be, given all the m_parentFrame->paget()->chrome copy paste.
Comment 16 Viatcheslav Ostapenko 2011-05-24 07:31:11 PDT
(In reply to comment #13)
> >>> Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:111
> >>> +            clipRect &= option->exposedRect;
> >> 
> >> Just by curiosity, partial updates are worth it? Do you have a test case where it performs better for the plugin?
> > 
> > Why not? ;)
> 
> Because computing the exposed rect may be costly in QGV.

Costlier than blitting of the whole screen? ;)
I didn't test this implementation for performance, but in other browser on symbian there is clear difference for big plugins that do partial screen updates (like some animation running) on N8 with raster only flash plugin. Frame rate drops significantly and visually you can seed difference. With HW accelerated plugin difference is minmal (5% max for small animations on full screen plugin), but still exists.
Comment 17 Viatcheslav Ostapenko 2011-05-24 07:49:12 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #11)
> > > (From update of attachment 91306 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=91306&action=review
> > > 
> > > > Source/WebCore/plugins/PluginView.h:432
> > > > +#if USE(ACCELERATED_COMPOSITING) && (defined(XP_UNIX) || OS(SYMBIAN))
> > > 
> > > Why the unix here?
> > 
> > Because this patch adds AC plugin layer implementation for symbian to already existing unix implementation. Mac and windows still do not have plugin AC layer implementation for Qt. I just modified existing ifdef (added SYMBIAN).
> > It's a bit unclear, because I had to break previous #ifdef at line 429 (paintUsingXPixmap is not needed on symbian and other platforms).
> 
> Sounds like we might want to come up with a new define then.  Otherwise we're going to have to add defined(WIN) || everywhere when we add win, and then again fo mac, etc.

After mac and win AC plugin layers are implemented this condition can be removed because all 4 platforms will be supported.
 
> > > 
> > > > Source/WebCore/plugins/symbian/PluginViewSymbian.cpp:472
> > > > +        if (m_parentFrame->page()->chrome()->client()->allowsAcceleratedCompositing()
> > > > +            && m_parentFrame->page()->settings()
> > > > +            && m_parentFrame->page()->settings()->acceleratedCompositingEnabled()) {
> > > 
> > > I wonder if this shouldn't be a helper function.
> > 
> > What exactly? Big condition or creating of plugin layer? Or both together? ;)
> > In any case, I'd prefer to leave it as it is to make it similar to unix implementation to make later merging of other changes simpler.
> 
> Well, consider this example:
> 
> if (shouldUseAccelleratedComposititing(m_parentFrame))
> 
> Such is very clear at the call site what it does.  And the actual implementation of the check can use local variables and early return as well.
> 
> I'm not saying that splitting ifs off into functions is always a good choice.  But in this one, I think it might be, given all the m_parentFrame->paget()->chrome copy paste.

Yes, I've got it. Should I make it for linux part also? I'd like to keep it similar.
Comment 18 Laszlo Gombos 2011-05-24 15:44:44 PDT
> > > > Source/WebCore/plugins/PluginView.h:432
> > > > +#if USE(ACCELERATED_COMPOSITING) && (defined(XP_UNIX) || OS(SYMBIAN))
> > > 
> > > Why the unix here?
> > 
> > Because this patch adds AC plugin layer implementation for symbian to already existing unix implementation. Mac and windows still do not have plugin AC layer implementation for Qt. I just modified existing ifdef (added SYMBIAN).
> > It's a bit unclear, because I had to break previous #ifdef at line 429 (paintUsingXPixmap is not needed on symbian and other platforms).
> 
> Sounds like we might want to come up with a new define then.  

Perhaps a local macro ?

#if PLATFORM(QT) && USE(ACCELERATED_COMPOSITING) && ENABLE(NETSCAPE_PLUGIN_API) && (defined(XP_UNIX) || OS(SYMBIAN))
#define WTF_USE_ACCELERATED_COMPOSITING_PLUGIN_LAYER
#endif
Comment 19 Laszlo Gombos 2011-05-24 15:49:06 PDT
Comment on attachment 91306 [details]
Updated patch

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

r- to make sure guards are used consistently.

>>>>>> Source/WebCore/plugins/PluginView.h:432
>>>>>> +#if USE(ACCELERATED_COMPOSITING) && (defined(XP_UNIX) || OS(SYMBIAN))
>>>>> 
>>>>> Why the unix here?
>>>> 
>>>> +1
>>> 
>>> Because this patch adds AC plugin layer implementation for symbian to already existing unix implementation. Mac and windows still do not have plugin AC layer implementation for Qt. I just modified existing ifdef (added SYMBIAN).
>>> It's a bit unclear, because I had to break previous #ifdef at line 429 (paintUsingXPixmap is not needed on symbian and other platforms).
>> 
>> Sounds like we might want to come up with a new define then.  Otherwise we're going to have to add defined(WIN) || everywhere when we add win, and then again fo mac, etc.
> 
> After mac and win AC plugin layers are implemented this condition can be removed because all 4 platforms will be supported.

This probably breaks the build when ENABLE(NETSCAPE_PLUGIN_API) is false. A local macro use consistently would help avoid making mistakes like this and would help with readability.
Comment 20 Viatcheslav Ostapenko 2011-05-26 12:10:58 PDT
Created attachment 95016 [details]
Updated patch
Comment 21 Laszlo Gombos 2011-06-13 21:41:23 PDT
Comment on attachment 95016 [details]
Updated patch

LGTM, r+.
Comment 22 WebKit Review Bot 2011-06-14 10:13:14 PDT
Comment on attachment 95016 [details]
Updated patch

Clearing flags on attachment: 95016

Committed r88816: <http://trac.webkit.org/changeset/88816>
Comment 23 WebKit Review Bot 2011-06-14 10:13:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Ademar Reis 2011-06-27 06:58:25 PDT
Revision r88816 cherry-picked into qtwebkit-2.2 with commit 7a6f64d <http://gitorious.org/webkit/qtwebkit/commit/7a6f64d>