On 07/09/12 02:13, Erich E. Hoover wrote:
- case HH_CLOSE_ALL: {
struct list *ptr;HHInfo *info;LIST_FOR_EACH(ptr, &window_list){info = LIST_ENTRY(ptr, HHInfo, entry);TRACE("Destroying window %s.\n", debugstr_w(info->pszType));if (info->WinType.hwndHelp)DestroyWindow(info->WinType.hwndHelp);list_remove(&info->entry);}
How is it supposed to work? AFAICS you're destroying windows, removing info object from the list and forget about it. If your intention is to destroy these objects, then ReleaseHelpViewer seems more appropriate. Also moving list_remove call to ReleaseHelpViewer seems preferable, so that it's removed from the list regardless of the reason for being destroyed. Same for list_add_tail, shouldn't it be done in CreateHelpViewer?
Jacek
On Mon, Jul 9, 2012 at 3:31 AM, Jacek Caban jacek@codeweavers.com wrote:
... How is it supposed to work? AFAICS you're destroying windows, removing info object from the list and forget about it. If your intention is to destroy these objects, then ReleaseHelpViewer seems more appropriate. Also moving list_remove call to ReleaseHelpViewer seems preferable, so that it's removed from the list regardless of the reason for being destroyed. Same for list_add_tail, shouldn't it be done in CreateHelpViewer?
That makes sense, I'll correct this and resubmit. Is there anything else you would like me to change?
Erich
On 07/09/12 17:49, Erich E. Hoover wrote:
On Mon, Jul 9, 2012 at 3:31 AM, Jacek Caban jacek@codeweavers.com wrote:
... How is it supposed to work? AFAICS you're destroying windows, removing info object from the list and forget about it. If your intention is to destroy these objects, then ReleaseHelpViewer seems more appropriate. Also moving list_remove call to ReleaseHelpViewer seems preferable, so that it's removed from the list regardless of the reason for being destroyed. Same for list_add_tail, shouldn't it be done in CreateHelpViewer?
That makes sense, I'll correct this and resubmit. Is there anything else you would like me to change?
Other patches will have to be slightly changed due to above, to their full review is not possible ATM. From what I see in patch 5, you also call list_add_tail outside constructor. I'd have also minor suggestions to use an inline function instead of NAVIGATION_VISIBLE macro and consider using LIST_FOR_EACH_ENTRY instead of LIST_FOR_EACH.
Jacek
On Tue, Jul 10, 2012 at 3:52 AM, Jacek Caban jacek@codeweavers.com wrote:
... From what I see in patch 5, you also call list_add_tail outside constructor. ...
I don't see much of a choice, you need to be able to "set" wintype data for windows that don't exist yet. I've attached a proposed update including the other suggestions you've made, let me know what you think.
Erich
On 07/10/12 19:12, Erich E. Hoover wrote:
On Tue, Jul 10, 2012 at 3:52 AM, Jacek Caban jacek@codeweavers.com wrote:
... From what I see in patch 5, you also call list_add_tail outside constructor. ...
I don't see much of a choice, you need to be able to "set" wintype data for windows that don't exist yet. I've attached a proposed update including the other suggestions you've made, let me know what you think.
It still may be moved to common place, perhaps by splitting allocation and initialization to separated functions, but I'm fine with this solution for now. I have just minor comments:
info = CreateHelpViewer(fullname); if(!info) + { + heap_free((WCHAR*)default_index); + heap_free((WCHAR*)window); return NULL; + }
This is already existing problem I noticed while reviewing the patch that those calls should not be needed (resolve_filename should not return const strings). It's worth fixing before spreading those casts in heap_free calls (they are present in other patch as well).
+ HHInfo *info, *next; + + LIST_FOR_EACH_ENTRY_SAFE(info, next, &window_list, HHInfo, entry) + { + TRACE("Destroying window %s.\n", debugstr_w(info->WinType.pszType)); + if (info) + ReleaseHelpViewer(info);
if(info) is not needed here.
Thanks, Jacek
-
Erich E. Hoover -
Jacek Caban