[webkit-reviews] review denied: [Bug 57418] [Qt] [Symbian] GraphicsLayer: support plugins on symbian : [Attachment 91306] Updated patch

[bugzilla-daemon at webkit.org]

Laszlo Gombos <laszlo.1.gombos at nokia.com> has denied Viatcheslav Ostapenko
<ostapenko.viatcheslav at nokia.com>'s request for review:
Bug 57418: [Qt] [Symbian] GraphicsLayer: support plugins on symbian
https://bugs.webkit.org/show_bug.cgi?id=57418

Attachment 91306: Updated patch
https://bugs.webkit.org/attachment.cgi?id=91306&action=review

------- Additional Comments from Laszlo Gombos <laszlo.1.gombos at nokia.com>
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.