[Win][MiniBrowser] Reimplement the toolbar by using toolbar common control https://docs.microsoft.com/en-us/windows/win32/controls/toolbar-control-reference The current implementation places button controls as the toolbar buttons. However, because these buttons are focusable, shortcut keys don't work after clicking them.
Created attachment 390617 [details] Patch
Created attachment 390618 [details] [Screenshot]
Created attachment 390623 [details] Patch
Created attachment 390624 [details] [Screenshot]
Comment on attachment 390623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390623&action=review Seems okay to me, though I think we're going to need some prettier icons, haha. :) > Tools/MiniBrowser/win/MainWindow.cpp:51 > +static constexpr int cToolbarImageSize = 24; > +static constexpr int cToolbarURLBarIndex = 4; > +static constexpr int cToolbarProgressIndicatorIndex = 6; Do we have precedent for this `cFooBar` naming convention? > Tools/MiniBrowser/win/MainWindow.h:80 > + int m_toolbarItemsWidth { }; If you're going to put braces here, shouldn't it be `{ 0 }`?
Comment on attachment 390623 [details] Patch On a more general note, would it be possible to consider doing this in a cross-platform way, where we could share the code between the different platforms? Maybe the toolbar could be a Web view?
Sounds an interesting idea, but this is not the right place to discuss. Microsoft created a sample browser using WebView2 (Chromium Edge WebView). Its UI part is implemented in HTML, CSS and JavaScript. https://github.com/MicrosoftEdge/WebView2Browser Because there is not cross-platfrom MiniBrowser, Microsoft Playwright is modifying GTK and Win MiniBrowser, and copies mac MiniBrowser. https://github.com/microsoft/playwright/tree/master/browser_patches/webkit/patches
Comment on attachment 390623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390623&action=review >> Tools/MiniBrowser/win/MainWindow.cpp:51 >> +static constexpr int cToolbarProgressIndicatorIndex = 6; > > Do we have precedent for this `cFooBar` naming convention? Oh, my brain-fart. 'kFooBar' seems the WebKit undocumented style for constants. Will fix. >> Tools/MiniBrowser/win/MainWindow.h:80 >> + int m_toolbarItemsWidth { }; > > If you're going to put braces here, shouldn't it be `{ 0 }`? C++ spec ensures it is zero-initialized. https://en.cppreference.com/w/cpp/language/zero_initialization This is also undocumented. WebKit style doesn't allow append unnecessary '.0' to floating point literals. So, I removed '0' in this case. https://webkit.org/code-style-guidelines/#floating-point-literals https://webkit.org/code-style-guidelines/#spacing-braced-init
Created attachment 390720 [details] Patch for landing
Comment on attachment 390720 [details] Patch for landing Clearing flags on attachment: 390720 Committed r256581: <https://trac.webkit.org/changeset/256581>
All reviewed patches have been landed. Closing bug.
<rdar://problem/59447830>