Re: [v4 PATCH] user32/menu: Return locked menu data when looking for menu items.

[Huw Davies]

On Wed, May 09, 2018 at 12:04:28PM +0300, Nikolay Sivov wrote:
> Signed-off-by: Nikolay Sivov
>
> v4: changed helper interface, simplified insertion point logic a bit.
>
> dlls/user32/menu.c | 556 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 348 insertions(+), 208 deletions(-)
>
> diff --git a/dlls/user32/menu.c b/dlls/user32/menu.c
> index 537c2ebe1a..98f1fce506 100644
> --- a/dlls/user32/menu.c
> +++ b/dlls/user32/menu.c
> @@ -584,26 +584,25 @@ static UINT  MENU_GetStartOfPrevColumn(
>      return i;
>  }
> -
> -
>
> -/***********************************************************************
> -
> -
>       MENU_FindItem
> -
> -
> -
> - Find a menu item. Return a pointer on the item, and modifies *hmenu
> -
> - in case the item was in a sub-menu.
> - */
>
> -static MENUITEM *MENU_FindItem( HMENU *hmenu, UINT *nPos, UINT wFlags )
> +static POPUPMENU *find_menu_item(HMENU hmenu, UINT id, UINT flags, UINT *pos)

I think this is more natural than the previous version.
> BOOL WINAPI RemoveMenu( HMENU hMenu, UINT nPos, UINT wFlags )
>  {
> - LPPOPUPMENU	menu;
> - MENUITEM *item;
> -
>
> POPUPMENU *menu;
> -
>
> UINT pos;
>
> TRACE("(menu=%p pos=%04x flags=%04x)\n",hMenu, nPos, wFlags);
> -
>
> if (!(item = MENU_FindItem( &hMenu, &nPos, wFlags ))) return FALSE;
> -
>
> if (!(menu = MENU_GetMenu(hMenu))) return FALSE;
> -
>  /* Remove item */
> - if (!(menu = find_menu_item(hMenu, nPos, wFlags, &pos)))
> -
>    return FALSE;
>
> - MENU_FreeItemData( item );
> -
>
> /* Remove item */
> -
>
> MENU_FreeItemData( &menu->items[pos] );
>
> if (--menu->nItems == 0)
> {
>
> @@ -4024,17 +4093,19 @@ BOOL WINAPI RemoveMenu( HMENU hMenu, UINT nPos, UINT wFlags )
>      }
>      else
>      {
> -
>    MENUITEM *new_items;
> - while(nPos < menu->nItems)
> -
>    MENUITEM *new_items, *item = &menu->items[pos];
> -
> - while (nPos < menu->nItems)
> {
>   *item = *(item+1);
>   item++;
>   nPos++;
> }

These should be pos not nPos.  I'd suggest changing the function's parameter name
from nPos to id.
> -
>    new_items = HeapReAlloc( GetProcessHeap(), 0, menu->items,
> -
>                             menu->nItems * sizeof(MENUITEM) );
> -
>    new_items = HeapReAlloc( GetProcessHeap(), 0, menu->items, menu->nItems * sizeof(MENUITEM) );
>    if (new_items) menu->items = new_items;
> }
> - release_menu_ptr(menu);
> - return TRUE;
>
> }
>
> @@ -4044,11 +4115,17 @@ BOOL WINAPI RemoveMenu( HMENU hMenu, UINT nPos, UINT wFlags )
>   */
>  BOOL WINAPI DeleteMenu( HMENU hMenu, UINT nPos, UINT wFlags )
>  {

Probably change nPos to id here as well.

Huw.