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

[Huw Davies]

On Mon, Apr 30, 2018 at 03:11:25PM +0300, Nikolay Sivov wrote:
> +struct menu_item_desc
> +{
> - MENUITEM *item;
> - POPUPMENU *menu;
> - UINT pos;
>
> +};
>
> /***********************************************************************
> -
>       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 BOOL MENU_FindItem(HMENU hmenu, UINT id, UINT flags, struct menu_item_desc *desc)

I'd have probably used the chance to rename this as find_item() ;-)

I also wondered about having this just return the menu and pos (in
which case it's find_item_pos()) and then either letting the callers
index directly into to the returned menu's items array, or adding a
get_item() that would do that.  I suppose though there are enough
callers that want the item, that this combined approach is a good
compromise.
> {
> - UINT fallback_pos = ~0u, i;
> POPUPMENU *menu;
> -
>
> MENUITEM *fallback = NULL;
> -
>
> UINT fallback_pos = 0;
> -
>
> UINT i;
> -
>
> if ((*hmenu == (HMENU)0xffff) || (!(menu = MENU_GetMenu(*hmenu)))) return NULL;
> -
>
> if (wFlags & MF_BYPOSITION)
> - memset(desc, 0, sizeof(*desc));
> -
> - menu = grab_menu_ptr(hmenu);
> - if (!menu)
> -
>    return FALSE;
> -
> - if (flags & MF_BYPOSITION)
> {
> - if (*nPos >= menu->nItems) return NULL;
> - return &menu->items[*nPos];
> -
>    if (id >= menu->nItems)
> -
>    {
> -
>        release_menu_ptr(menu);
> -
>        return FALSE;
> -
>    }
> -
> -
>    desc->menu = menu;
> -
>    desc->item = &menu->items[id];
> -
>    desc->pos = id;
> -
>    return TRUE;
> }
>    else
>    {
>
> @@ -612,32 +625,37 @@ static MENUITEM *MENU_FindItem( HMENU *hmenu, UINT *nPos, UINT wFlags )
>    {
>        if (item->fType & MF_POPUP)
>        {
> -
> HMENU hsubmenu = item->hSubMenu;
> -
> MENUITEM *subitem = MENU_FindItem( &hsubmenu, nPos, wFlags );
> -
> if (subitem)
> -
> {
> -
>     *hmenu = hsubmenu;
> -
>     return subitem;
> -
> }
> -
> else if (item->wID == *nPos)
> -
>            if (MENU_FindItem(item->hSubMenu, id, flags, desc))
> -
>            {
> -
>                release_menu_ptr(menu);
> -
>                return TRUE;
> -
>            }
> -
>            else if (item->wID == id)
> {
>       /* fallback to this item if nothing else found */
>       fallback_pos = i;
> -
>     fallback = item;
> }
>   }
> -
>    else if (item->wID == *nPos)
> -
>    else if (item->wID == id)
>  {
> -
> *nPos = i;
> -
> return item;
> -
>            desc->menu = menu;
> -
>            desc->item = item;
> -
>            desc->pos = i;
> -
>            return TRUE;
>  }
> }
>    }
> - if (fallback)
> -
>    *nPos = fallback_pos;
> - if (fallback_pos != ~0u)
> - {
> -
>    desc->menu = menu;
> -
>    desc->item = &menu->items[fallback_pos];
> -
>    desc->pos = fallback_pos;
> - }
> - else
> -
>    release_menu_ptr(menu);
>
> - return fallback;
> - return fallback_pos != ~0u;
>
> }
>
> /***********************************************************************
> @@ -2135,28 +2153,34 @@ static void MENU_MoveSelection( HWND hwndOwner, HMENU hmenu, INT offset )
> -
> - Insert (allocate) a new item into a menu.
>
> */
> -static MENUITEM *MENU_InsertItem( HMENU hMenu, UINT pos, UINT flags )
> +static BOOL MENU_InsertItem(HMENU hMenu, UINT id, UINT flags, struct menu_item_desc *desc)
>  {
> - UINT pos = id;
> MENUITEM *newItems;
> POPUPMENU *menu;
> - if (!(menu = MENU_GetMenu(hMenu)))
> -
>    return NULL;
> -
>
> memset(desc, 0, sizeof(*desc));
>
> /* Find where to insert new item */
> - if (flags & MF_BYPOSITION) {
> -
>    if (pos > menu->nItems)
> -
>    menu = grab_menu_ptr(hMenu);
> -
>    if (id > menu->nItems)
>        pos = menu->nItems;
> } else {
> -
>    if (!MENU_FindItem( &hMenu, &pos, flags ))
> -
>    struct menu_item_desc item_desc;
> -
> -
>    if (!MENU_FindItem(hMenu, id, flags, &item_desc))
> -
>    {
> -
>        menu = grab_menu_ptr(hMenu);
>        pos = menu->nItems;
> -
>    else {
> -
>        if (!(menu = MENU_GetMenu( hMenu )))
> -
>            return NULL;
>    }
> -
>    else
> -
>        menu = item_desc.menu;

Shouldn't you use the returned item_desc.pos here?

Also, to simplify things, couldn't the MF_BYPOSITION case just call
MENU_FindItem() too?

Huw.