Bug 207682

Summary: [Win][MiniBrowser] Reimplement the toolbar by using toolbar common control
Product: WebKit Reporter: Fujii Hironori <Hironori.Fujii>
Component: Tools / TestsAssignee: Fujii Hironori <Hironori.Fujii>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, don.olmstead, pfeldman, pvollan, ross.kirsling, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
[Screenshot]
none
Patch
none
[Screenshot]
none
Patch for landing none

Description Fujii Hironori 2020-02-12 22:12:09 PST
[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.
Comment 1 Fujii Hironori 2020-02-12 22:21:05 PST
Created attachment 390617 [details]
Patch
Comment 2 Fujii Hironori 2020-02-12 22:21:40 PST
Created attachment 390618 [details]
[Screenshot]
Comment 3 Fujii Hironori 2020-02-13 00:04:23 PST
Created attachment 390623 [details]
Patch
Comment 4 Fujii Hironori 2020-02-13 00:04:46 PST
Created attachment 390624 [details]
[Screenshot]
Comment 5 Ross Kirsling 2020-02-13 11:37:31 PST
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 6 Per Arne Vollan 2020-02-13 12:41:20 PST
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?
Comment 7 Fujii Hironori 2020-02-13 17:40:00 PST
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 8 Fujii Hironori 2020-02-13 18:25:59 PST
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
Comment 9 Fujii Hironori 2020-02-13 19:02:46 PST
Created attachment 390720 [details]
Patch for landing
Comment 10 Fujii Hironori 2020-02-13 19:15:21 PST
Comment on attachment 390720 [details]
Patch for landing

Clearing flags on attachment: 390720

Committed r256581: <https://trac.webkit.org/changeset/256581>
Comment 11 Fujii Hironori 2020-02-13 19:15:24 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2020-02-13 19:16:12 PST
<rdar://problem/59447830>