Add Chrome Classic Menu Bar mod - #5024
Conversation
|
Thanks for the pull request! This repository uses a two-stage review: an AI review that you run yourself, followed by a human review. To get started, comment See the pull request review process for the full details. |
|
/ai-review |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. Nice idea, and the owner-drawn dark menu looks well put together. There are, however, several issues that affect the rest of the system, the process lifetime and mod reloads. 1. The global hotkeys hijack 2. Synthesized keystrokes can land in a different application. Concretely:
3. Deadlock on unload: cross-thread Copy the state out under the lock, release it, then do the window work: std::vector<std::pair<HWND, WindowState>> windows;
{
AcquireSRWLockExclusive(&g_stateLock);
windows.assign(std::make_move_iterator(g_windows.begin()),
std::make_move_iterator(g_windows.end()));
g_windows.clear();
ReleaseSRWLockExclusive(&g_stateLock);
}
for (auto& [hwnd, state] : windows) { /* SetMenu / RemoveWindowSubclassFromAnyThread / delete */ }
4. Worker-thread teardown isn't reliable, and the DLL can be unloaded while mod code is running.
5. The hotkey window class survives the mod, leaving a dangling 6. The mod loads into every Chrome subprocess. if (wcsstr(GetCommandLineW(), L"--type=")) {
return FALSE;
}7. The two pollers force a frame recalculation on every Chrome window 4-6 times per second, forever. 8. 9. The same window can be attached twice. 10. 11. Drawing is hard-coded to 96 DPI while measuring uses the real DPI. 12. There's no settings block; the only option lives inside the menu and isn't persisted. 13. Overlap with the existing native-frame mods. Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
/ai-review |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. Nice idea, and the frame integration is credited properly. The main problems are around process scope, continuous polling, and unload safety — several of them can hang or crash Chrome. 1. The mod runs in every
if (wcsstr(GetCommandLineW(), L"--type=") != nullptr) {
Wh_Log(L"Auxiliary process detected, skipping");
return FALSE;
}Returning 2. Two polling loops, plus a
At minimum, make 3. Deadlock on unload: cross-thread sends while holding
Copy the state out, release the lock, then do the window work: std::unordered_map<HWND, WindowState> windows;
{
AcquireSRWLockExclusive(&g_stateLock);
windows.swap(g_windows);
ReleaseSRWLockExclusive(&g_stateLock);
}
for (auto& [hwnd, state] : windows) {
if (IsWindow(hwnd)) {
SetMenu(hwnd, nullptr);
...
WindhawkUtils::RemoveWindowSubclassFromAnyThread(hwnd, ChromeSubclassProc);
}
...
}
4. Worker threads can outlive the mod DLL Three separate paths let a thread keep running code in an unmapped DLL after unload:
See https://github.com/ramensoftware/windhawk/wiki/Global-objects-and-process-shutdown for the teardown rules around worker threads and globals (#1-worker-thread-stdthread). 5. The hotkey window class is registered with the wrong wc.hInstance = GetModuleHandleW(nullptr); // this is chrome.exe, not the mod
RegisterClassExW(&wc); // result ignored
...
if (!g_hotkeyWindow) { return 0; } // class never unregistered on this pathA window class isn't removed when the mod DLL unloads, and 6.
None of these are Chrome shortcuts to begin with, and the menu items work without them. I'd drop the hotkey window entirely; if you want to keep them, make them opt-in settings, off by default. 7. Race in The Hold the exclusive lock across the check and the insert, or insert a placeholder entry under the exclusive lock first so the second thread sees it and bails. 8.
Since 9. The synthesized keystrokes go wherever focus happens to be
Prefer handing the URL to the browser instead of typing it: wchar_t exePath[MAX_PATH];
GetModuleFileNameW(nullptr, exePath, ARRAYSIZE(exePath));
std::wstring cmd = L"\"" + std::wstring(exePath) + L"\" \"" + url + L"\"";
STARTUPINFOW si{sizeof(si)};
PROCESS_INFORMATION pi{};
if (CreateProcessW(nullptr, cmd.data(), nullptr, nullptr, FALSE, 0, nullptr,
nullptr, &si, &pi)) { ... }Chrome forwards the URL to the running instance and opens it in a tab, with no focus dependency and no sleeps. If you keep 10. No settings block at all The mod has no 11. Overlaps and conflicts with Titlebar For Everyone The README credits it, and the frame code really is the same approach: strip I understand the menu bar needs the native frame to be visible at all, so the duplication isn't gratuitous — but please make the frame handling a setting that can be turned off, so users who already run Titlebar For Everyone can let that mod own the frame, and say in the README how the two interact. 12. Add a screenshot to the README The whole point of the mod is a visible menu bar, and there's no image. Please add a screenshot (or GIF) showing both the light and dark menu — allowed hosts are Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
Address AI review safety and performance issues
|
/ai-review |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. Nice idea, and the lifecycle handling (subclass removal, per-window state, no locks held across 1. The menu is attached to any captioned
At minimum compare the class name exactly and exclude owned windows, as chrome-pip-alt-tab-window-hider does: if (wcscmp(className, L"Chrome_WidgetWin_1") != 0) return false;
if (GetWindow(hwnd, GW_OWNER) != nullptr) return false;That still won't separate the Task Manager / profile picker from a real browser window, so please check those cases explicitly and add whatever further filter works (e.g. requiring a 2. "Manage Chrome native frame" doesn't actually stop the mod from owning the frame. The README tells users to disable this setting when Titlebar For Everyone is enabled, but the subclass routes the frame messages to case WM_NCCALCSIZE:
case WM_NCHITTEST:
case WM_NCLBUTTONDOWN:
return DefWindowProcW(hwnd, msg, wParam, lParam);That is exactly what "owning the frame" means here, so with the setting off the two mods still fight over the non-client area. Gate these on Two related points in the same area:
3. case WM_MEASUREITEM:
MeasureOwnerItem(hwnd, ...);
return TRUE;
case WM_DRAWITEM:
DrawOwnerItem(...);
return TRUE;Both handlers You already keep every allocation in 4. DPI handling is inconsistent between measure and draw.
5. Overlap with existing mods — consider dropping the owner-draw theming and the frame management. Both non-menu-bar halves of this mod duplicate mods that are already in the catalog:
If the menu were built with plain 6. Frame changes aren't reverted when the mod is disabled.
WindhawkUtils::RemoveWindowSubclassFromAnyThread(hWnd, SubclassProc);
SetWindowPos(hWnd, NULL, 0, 0, 0, 0,
SWP_FRAMECHANGED | SWP_NOMOVE | SWP_NOSIZE | SWP_NOZORDER |
SWP_NOOWNERZORDER | SWP_NOACTIVATE);7. Bookmark Manager / Extensions / Settings / About all go through 8. Add a screenshot to the README. The mod's whole point is a visible UI change; a screenshot (light and dark) makes it much easier to evaluate and to find on windhawk.net. Only Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
Thanks for the suggestion! I haven't tested other Chromium-based browsers yet, but I'll definitely look into it. |
|
On Edge, "About Google Chrome" opens a homepage. |
|
Show bookmarks bar item has no checkbox on it. |
|
Thanks for testing and greatly appreciated although I haven't officially tested or added Edge support yet, so those menu items are currently Chrome-specific. I'll keep this in mind when I work on official Edge compatibility. |
|
On Edge, does not work in newly-opened windows, only in those which were already open when the mod started. |
Updated the Chrome Classic Menu Bar mod to support Microsoft Edge and improved descriptions. Enhanced functionality for menu attachment and hotkey handling.
|
/ai-review |
|
This pull request has already had 3 AI reviews in the last 24 hours, which is the limit, so no review was posted this time. Comment |
|
Now works in new windows and the settings page opens. A problem: new windows are always opened in dark theme, and the setting for it was removed. The menu checkbox only affects the current window. |

Changelog
If this pull request updates an existing mod, describe the changes below:
Changelog
Initial release.
Features:
Mod authorship
If this pull request introduces a new mod, please complete the section below.
This mod was created by:
Please select the options that best apply. Your selection does not affect the acceptance criteria, but it helps reviewers understand the context of the code and provide relevant feedback.