Bug 207682 – [Win][MiniBrowser] Reimplement the toolbar by using toolbar common control

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Fujii Hironori]

[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.

[Fujii Hironori]

Created attachment 390617 [details]
Patch

[Fujii Hironori]

Created attachment 390618 [details]
[Screenshot]

[Fujii Hironori]

Created attachment 390623 [details]
Patch

[Fujii Hironori]

Created attachment 390624 [details]
[Screenshot]

[Ross Kirsling]

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 }`?

[Per Arne Vollan]

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?

[Fujii Hironori]

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

[Fujii Hironori]

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

[Fujii Hironori]

Created attachment 390720 [details]
Patch for landing

[Fujii Hironori]

Comment on attachment 390720 [details]
Patch for landing

Clearing flags on attachment: 390720

Committed r256581: <https://trac.webkit.org/changeset/256581>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59447830>