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


Reply via email to