Re: Added XEmbed embedder support

[Dimi Paun]

On Mon, 2005-06-20 at 00:05 +0200, Jacek Caban wrote:
> Hello.
>
> This patch adds support for XEmbed embedder
> It implements most of it (remaining todos
> are listed in comments of xembed.c).

Cool stuff, I'm really excited about this work. 
Not to mention that this might prove useful outside
of the MSHTML context.

I will not comment on the fundamentals of
the approach, that's more Alexandre's alley, but
just a few style nits.
> +struct xembed_list
> +{
> - struct xembed_list *next;
> - HWND hwnd;
>
> +};

What about using the standard list from wine/list.h?
> /* DIB Section sync state */
> @@ -590,6 +601,8 @@ enum x11drv_atoms
>      XATOM_XdndSelection,
>      XATOM_XdndTarget,
>      XATOM_XdndTypeList,
> - XATOM__XEMBED,
> - XATOM__XEMBED_INFO,
> XATOM_WCF_DIB,
> XATOM_image_gif,
> XATOM_text_html,
>
> @@ -649,6 +662,10 @@ struct x11drv_win_data
>      struct dce *dce;            /* DCE for CS_OWNDC or CS_CLASSDC windows */
>      HBITMAP     hWMIconBitmap;
>      HBITMAP     hWMIconMask;
> - union {
> -
>    struct xembed_list *list;
> -
>    ULONG_PTR cnt;
> - } xembed;
>
> };

Do we really need the union? We're not that hard pressed for
memory. What about just

@@ -649,6 +662,10 @@ struct x11drv_win_data
     struct dce *dce;            /* DCE for CS_OWNDC or CS_CLASSDC windows */
     HBITMAP     hWMIconBitmap;
     HBITMAP     hWMIconMask;
+    struct xembed_list *list;
+    ULONG_PTR cnt;
 };

Also, instead of having explicit tests like these:
> - if(data->xembed.cnt)

Maybe we can have a 
  BOOL win_has_embedded_children(struct x11drv_win_data *data)

function to make it more explicit what we're testing for.

Added XEmbed embedder support

[Jacek Caban]

Dimi Paun wrote:
> On Mon, 2005-06-20 at 00:05 +0200, Jacek Caban wrote:
> > Hello.
> >
> > This patch adds support for XEmbed embedder
> > It implements most of it (remaining todos
> > are listed in comments of xembed.c).
>
> Cool stuff, I'm really excited about this work. 
> Not to mention that this might prove useful outside
> of the MSHTML context.
>
> I will not comment on the fundamentals of
> the approach, that's more Alexandre's alley, but
> just a few style nits.
> > +struct xembed_list
> > +{
> > - struct xembed_list *next;
> > - HWND hwnd;
> >
> > +};
>
> What about using the standard list from wine/list.h?

AFAIK using standard list lists or not is up to developer.
Personally I don't like Wine's implementation... I don't have
any argument why - I just feel better not using it :-)
> > /* DIB Section sync state */
> > @@ -590,6 +601,8 @@ enum x11drv_atoms
> >     XATOM_XdndSelection,
> >     XATOM_XdndTarget,
> >     XATOM_XdndTypeList,
> > - XATOM__XEMBED,
> > - XATOM__XEMBED_INFO,
> > XATOM_WCF_DIB,
> > XATOM_image_gif,
> > XATOM_text_html,
> >
> > @@ -649,6 +662,10 @@ struct x11drv_win_data
> >     struct dce *dce;            /* DCE for CS_OWNDC or CS_CLASSDC windows */
> >     HBITMAP     hWMIconBitmap;
> >     HBITMAP     hWMIconMask;
> > - union {
> > -
> >    struct xembed_list *list;
> > -
> >    ULONG_PTR cnt;
> > - } xembed;
> >
> > };
>
> Do we really need the union? We're not that hard pressed for
> memory. What about just
>
> @@ -649,6 +662,10 @@ struct x11drv_win_data
>     struct dce *dce;            /* DCE for CS_OWNDC or CS_CLASSDC windows */
>     HBITMAP     hWMIconBitmap;
>     HBITMAP     hWMIconMask;
> - struct xembed_list *list;
> - ULONG_PTR cnt;
>
> };

I can't see why union is worse... But instead of union it can
always be a list, if we don't care about memory. I didn't do it
this way because it was better for my earlier versions, but now
I may change it.
> Also, instead of having explicit tests like these:
> > - if(data->xembed.cnt)
>
> Maybe we can have a 
>  BOOL win_has_embedded_children(struct x11drv_win_data *data)
>
> function to make it more explicit what we're testing for.

OK, I thought it's clean enough, but I can change it.

Jacek