[Webkit-unassigned] [Bug 57418] [Qt] [Symbian] GraphicsLayer: support plugins on symbian

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=57418





--- Comment #15 from Eric Seidel <eric at webkit.org>  2011-05-24 07:27:21 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.