RFR: JDK-8311113: Remove invalid pointer cast and clean up setLabel() in awt_MenuItem.cpp
Alexey Ivanov
aivanov at openjdk.org
Thu Aug 31 12:20:02 UTC 2023
On Thu, 31 Aug 2023 00:49:01 GMT, Sergey Bylokhov <serb at openjdk.org> wrote:
>>> Hmm, so where does JAWS get that info if we no longer set it ? It isn't scraping pixels and doing OCR.
>>
>> I'll have to check this further.
>
>>I'll have to check this further
>
> It will get it via javaaccessbridge we provided(kind of native interface) and this native lib uses our a11y java code for each component.
@mrserb @prrace If the string label was stored in the menu structure by Windows, we could retrieve it, right? Let's do it.
I applied the following **patch** to the original `awt_MenuItem.cpp`:
diff --git a/src/java.desktop/windows/native/libawt/windows/awt_MenuItem.cpp b/src/java.desktop/windows/native/libawt/windows/awt_MenuItem.cpp
index 69a7f4606e2..ccdc82c63f5 100644
--- a/src/java.desktop/windows/native/libawt/windows/awt_MenuItem.cpp
+++ b/src/java.desktop/windows/native/libawt/windows/awt_MenuItem.cpp
@@ -34,6 +34,7 @@
#include <tchar.h>
#include <imm.h>
#include <ime.h>
+#include <stdio.h>
// End -- Win32 SDK include files
//add for multifont menuitem
@@ -685,6 +686,51 @@ void AwtMenuItem::DoCommand()
}
}
+static void PrintMenuItemInfo(HMENU hMenu, UINT id)
+{
+ MENUITEMINFO mii;
+
+ memset(&mii, 0, sizeof(MENUITEMINFO));
+ mii.cbSize = sizeof(MENUITEMINFO);
+ mii.fMask = MIIM_CHECKMARKS | MIIM_DATA | MIIM_ID
+ | MIIM_STATE | MIIM_SUBMENU | MIIM_TYPE;
+
+ if (!::GetMenuItemInfo(hMenu, id, FALSE, &mii)) {
+ wprintf(L"GetMenuItemInfo returned an error for %u, %p\n", id, hMenu);
+ return;
+ }
+
+ wprintf(L"Menu info for %u, %p\n", id, hMenu);
+ wprintf(L" dwItemData = %p\n", (void *) mii.dwItemData);
+ wprintf(L" cch = %u\n", mii.cch);
+ wprintf(L" dwTypeData = %p\n", mii.dwTypeData);
+ if (mii.cch > 0) {
+ mii.dwTypeData = new TCHAR[mii.cch + 1];
+ if (!::GetMenuItemInfo(hMenu, id, FALSE, &mii)) {
+ wprintf(L" failed to get label\n");
+ } else {
+ wprintf(L" label = %s\n", mii.dwTypeData);
+ }
+ delete[] mii.dwTypeData;
+ }
+
+ // fType
+ wprintf(L" fType = %x = ", mii.fType);
+ if (mii.fType & MFT_OWNERDRAW) {
+ wprintf(L"MFT_OWNERDRAW ");
+ }
+ if (mii.fType & MFT_BITMAP) {
+ wprintf(L"MFT_BITMAP ");
+ }
+ if ((mii.fType & (MFT_OWNERDRAW | MFT_BITMAP)) == 0) {
+ wprintf(L"MFT_STRING ");
+ }
+ if (mii.fType & MFT_SEPARATOR) {
+ wprintf(L"MFT_SEPARATOR ");
+ }
+ wprintf(L"\n");
+}
+
void AwtMenuItem::SetLabel(LPCTSTR sb)
{
AwtMenu* menu = GetMenuContainer();
@@ -700,6 +746,13 @@ void AwtMenuItem::SetLabel(LPCTSTR sb)
HMENU hMenu = menu->GetHMenu();
MENUITEMINFO mii, mii1;
+ wprintf(L"AwtMenuItem::SetLabel(this = %p, id = %u, hMenu = %p\n",
+ this, GetID(), hMenu);
+ wprintf(L"> Before\n");
+ PrintMenuItemInfo(hMenu, GetID());
+ wprintf(L"< Before\n");
+
+
// get full information about menu item
memset(&mii, 0, sizeof(MENUITEMINFO));
mii.cbSize = sizeof(MENUITEMINFO);
@@ -725,6 +778,10 @@ void AwtMenuItem::SetLabel(LPCTSTR sb)
::RemoveMenu(hMenu, idx, MF_BYPOSITION);
::InsertMenuItem(hMenu, idx, TRUE, &mii);
+ wprintf(L"> After\n");
+ PrintMenuItemInfo(hMenu, GetID());
+ wprintf(L"< After\n");
+
RedrawMenuBar();
}
By running Harshitha's test, I get the following **output**:
AwtMenuItem::SetLabel(this = 000001F97CA87F50, id = 131645, hMenu = 0000000000030239
> Before
Menu info for 131645, 0000000000030239
dwItemData = 000001F97CA87F50
cch = 0
dwTypeData = 0000000000000000
fType = 100 = MFT_OWNERDRAW
< Before
> After
Menu info for 131645, 0000000000030239
dwItemData = 000001F97CA87F50
cch = 0
dwTypeData = 0000000000000000
fType = 100 = MFT_OWNERDRAW
< After
AwtMenuItem::SetLabel(this = 000001F97CA87EE0, id = 0, hMenu = 000000000002023D
> Before
Menu info for 0, 000000000002023D
dwItemData = 000001F97CA87F50
cch = 0
dwTypeData = 0000000000000000
fType = 100 = MFT_OWNERDRAW
< Before
> After
Menu info for 0, 000000000002023D
dwItemData = 000001F97CA87F50
cch = 0
dwTypeData = 0000000000000000
fType = 100 = MFT_OWNERDRAW
< After
AwtMenuItem::SetLabel(this = 000001F97CA88490, id = 2, hMenu = 000000000002023D
> Before
Menu info for 2, 000000000002023D
dwItemData = 000001F97CA87F50
cch = 0
dwTypeData = 0000000000000000
fType = 100 = MFT_OWNERDRAW
< Before
> After
Menu info for 2, 000000000002023D
dwItemData = 000001F97CA87F50
cch = 0
dwTypeData = 0000000000000000
fType = 100 = MFT_OWNERDRAW
< After
AwtMenuItem::SetLabel(this = 000001F97CA88730, id = 3, hMenu = 000000000002019F
> Before
Menu info for 3, 000000000002019F
dwItemData = 000001F97D5413B0
cch = 0
dwTypeData = 0000000000000000
fType = 100 = MFT_OWNERDRAW
< Before
> After
Menu info for 3, 000000000002019F
dwItemData = 000001F97D5413B0
cch = 0
dwTypeData = 0000000000000000
fType = 100 = MFT_OWNERDRAW
< After
AwtMenuItem::SetLabel(this = 000001F97CA89300, id = 5, hMenu = 000000000002019F
> Before
Menu info for 5, 000000000002019F
dwItemData = 000001F97D5413B0
cch = 0
dwTypeData = 0000000000000000
fType = 100 = MFT_OWNERDRAW
< Before
> After
Menu info for 5, 000000000002019F
dwItemData = 000001F97D5413B0
cch = 0
dwTypeData = 0000000000000000
fType = 100 = MFT_OWNERDRAW
< After
As you can see, in all the cases, the `cch` member of the `MENUITEMINFO` is zero — the string label cannot be retrieved. The *before* cases are expected to have no string data because `AppendMenu` doesn't allow setting it, I [explained it above](https://github.com/openjdk/jdk/pull/15276#issuecomment-1699846911). Yet the *after* case can't still retrieve the string label even though we went through the hoops to set it.
This proves *what's done in `_SetLabel` is **dead code***: it's not used by us, it's not used by the OS. Removing more than 100 lines which essentially do nothing is a good thing in my opinion.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/15276#issuecomment-1700927944
More information about the client-libs-dev
mailing list