Created attachment 50045 [details] Add EFL port files to WebCore/plugins/efl (patch 2/2)

Comment on attachment 50045 [details] Add EFL port files to WebCore/plugins/efl (patch 2/2) Patch mostly looks fine. I have minir comments inside... > +X11EmbedContainer::~X11EmbedContainer() > +{ > + setVisible(false); > + > + if (client()) { > + XUnmapWindow(x11Info()->display(), client()); > + XReparentWindow(x11Info()->display(), client(), rootWindow(), 0, 0); > + } > + > + delete m_focusProxy; > + delete m_clientWindow; It would make sense to make m_focusProxy and m_clientWindow OwnPtr here. > +class X11Info { > +public: > + Display* display() { return (Display*) ecore_x_display_get(); } > + Visual* visual() { return (Visual*) m_att.visual; } > + unsigned int depth() { return m_att.depth; } > + Colormap colormap() { return m_att.colormap; } > + unsigned int x11Time() { return ecore_x_current_time_get(); } > + > +private: > + X11Info(WinId owner) > + { > + m_owner = owner; > + ecore_x_window_attributes_get(owner, &m_att); > + m_att.colormap = DefaultColormap(display(), DefaultScreen(display())); > + } > + WinId m_owner; > + Ecore_X_Window_Attributes m_att; > + friend class X11Window; > +}; Is there any reason to not have X11Info more static? Currently you will end up having one instance per plugin, I think having one instance per Screen should be fine?

(In reply to comment #2) > > +X11EmbedContainer::~X11EmbedContainer() > > +{ (...) > > It would make sense to make m_focusProxy and m_clientWindow OwnPtr here. > Will do. > > +class X11Info { (...) > > > Is there any reason to not have X11Info more static? Currently you will end up > having > one instance per plugin, I think having one instance per Screen should be fine? > I wouldn't bother with this right now, since WebCore/plugins/efl is scheduled to be rewritten.

(In reply to comment #3) > > I wouldn't bother with this right now, since WebCore/plugins/efl is scheduled > to be rewritten. If that is the case please us the PluginViewNone.cpp, this way no one needs to spend time on dead and deprecated code.

Comment on attachment 50045 [details] Add EFL port files to WebCore/plugins/efl (patch 2/2) According to the comments the plugin code will be rewritten and I think there is little point reviewing and landing a version that will not be used. If this judgement seem inappropriate please just set the review flag again.

Hi zecke, The current system works, albeit the code is not as perfect as one would want. Going with "none" is loosing functionality. The next system will be our attempt to use Qt and Gtk efforts for windowless flash... the golden goal, but we're holding this since you do know it will takes lots of time to get this working and with great performance. :-)

(In reply to comment #6) > The current system works, albeit the code is not as perfect as one would want. > Going with "none" is loosing functionality. > > The next system will be our attempt to use Qt and Gtk efforts for windowless > flash... the golden goal, but we're holding this since you do know it will > takes lots of time to get this working and with great performance. :-) I think what zecke is saying is that we have two options: land a fixed-up version of the current code, or land the rewritten version. EFL has currently no plugin code in the tree, so you won't be loosing anything for now. So, if the golden goal is going to take a while, fixing the issues with the current code to get it landed seems to be worth it =).

Please close this bug as we'll go with Plugin*None as requested by Zecke as soon as #37478 is done.

Closing this bug as we'll use the Plugin*None implementation until we have a better Plugin implementation.