[webkit-dev] Clean way to allow delegates for PopupMenu?

[Luiz Agostini]

2010/4/16 Gustavo Sverzut Barbieri <barbieri at profusion.mobi>

> On Fri, Apr 16, 2010 at 10:30 AM, Luiz Agostini
> <luiz.agostini at openbossa.org> wrote:
> >
> > 2010/4/15 Gustavo Sverzut Barbieri <barbieri at profusion.mobi>
> >>
> >> Hello all,
> >>
> >> I'm part of the EFL port team and we're implementing the PopupMenu,
> >> however EFL is a different platform as for our port it is just a
> >> state-full canvas, not relying on any widget set/toolkit, we do not
> >> have the concept of a menu at all!
> >>
> >> Looking at all existing implementations, they all go straight to
> >> native platform menus. But we can't as we want to avoid such
> >> dependencies. What we'd like to have is a delegate, that WebCore calls
> >> the ChromeClient that is overloaded in WebKit, giving our View user a
> >> callback with all data.
> >>
> >> The good news is: we did it and it works quite well.
> >>
> >> The bad news is: we cheated and include WebKit/efl/WebCoreSupport from
> >> PopupMenuEfl.cpp, what a shame! (although some ports *cough qt*
> >> include in WebCore defines from WebKit)
> >
> > In Qt we have an abstract class for popup delegates in
> WebCore/platform/qt
> > and we do the actual popup delegate implementation in
> > WebKit/qt/WebCoreSupport. The delegates are implemented in WebKit layer
> and
> > PopupMenuQt has no knowledge about any specific delegates. The only
> reason
> > to include WebKit/qt/WebCoreSupport in PopupMenuQt is to make the call
> that
> > creates the delegate object. It could be avoided adding a method to
> Chrome
> > but at the time I was working on it it was decided that it was not
> needed.
>
> Yes, it is kinda similar to our, since we looked at your
> implementation before doing our. But the problem here is you still
> include WebKit files from WebCore.  For *me* it looks like a hack, but
> if people do not complain then I can kindly keep it as is, after all
> it is working already :-)
>
>
> >> In order to have a clean design, we'd like to know the general opinion
> >> on how to do it. From what I see Mac already defines a similar call in
> >> ChromeClient.h:
> >>
> >> #if PLATFORM(MAC)
> >> ...
> >>        virtual void willPopUpMenu(NSMenu *) { }
> >> #endif
> >>
> >> in our case, we'd need:
> >>
> >> #if PLATFORM(EFL)
> >>        virtual void showPopupMenu(const IntRect& rect, FrameView*
> >> view, int index) { }
> >>        virtual void hidePopupMenu() { }
> >> #endif
> >>
> >> so our PopupMenuEfl.cpp will just proxy to these calls.
> >>
> >> However, although Mac does that it may not be the best solution, so to
> >> avoid endless patches to be discussed at bugzilla I'd like to know
> >> your opinion on the best way so our patch is right from beginning.
> >
> >
> > I think that we could add methods to Chrome and ChromeClient to create
> the
> > delegates. For example:
> > PopupMenuDelegate* Chrome::createPopupMenuDelegate(PopupMenuClient* c) {
> > return m_client->createPopupMenuDelegate(c); }
> > virtual PopupMenuDelegate*
> > ChromeClient::createPopupMenuDelegate(PopupMenuClient*) { return 0; }
> > Each port could then provide its typedef for PopupMenuDelegate and
> > override ChromeClient::createPopupMenuDelegate.
>
> Well, as I said we just need methods to show/hide to keep it done, but
> if you want to implement another class "PopupMenuDelegate" to do it,
> then we could do such work as well. We'd like to avoid this delegate
> class as our port is C, so we'd need to do it all in C++ plus what we
> have done in C already, just to proxy it.
>

I did not suggest a class for PopupMenuDelegate: "Each port could then
provide its typedef for PopupMenuDelegate".

Remember PlatformWidget? :-)


>
>
> BR,
>
> --
> Gustavo Sverzut Barbieri
> http://profusion.mobi embedded systems
> --------------------------------------
> MSN: barbieri at gmail.com
> Skype: gsbarbieri
> Mobile: +55 (19) 9225-2202
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20100416/af0bf269/attachment.html>