Bug 189846 – [Win][WebKit] Implement authentication dialog on MiniBrowser.

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

[Basuke Suzuki]

It was implemented for WebKitLegacy, but not for WebKit.

[Basuke Suzuki]

Created attachment 350431 [details]
PATCH

[EWS Watchlist]

Attachment 350431 [details] did not pass style-queue:


ERROR: Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Basuke Suzuki]

Created attachment 350454 [details]
PATCH

[EWS Watchlist]

Attachment 350454 [details] did not pass style-queue:


ERROR: Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:84:  create_bstr_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Basuke Suzuki]

Created attachment 350706 [details]
FIX

Fix style. Didn't fix those in the tool generated files.

[EWS Watchlist]

Attachment 350706 [details] did not pass style-queue:


ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Fujii Hironori]

Comment on attachment 350706 [details]
FIX

View in context: https://bugs.webkit.org/attachment.cgi?id=350706&action=review

> Tools/MiniBrowser/win/Common.cpp:125
> +    _bstr_t& password;

The reason why I don't like this patch is I don't want to use
_bstr_t in common code and modern WebKit part becuase _bstr_t is
a string type for COM. This is not problem for WebKitLegacy
because it is a COM API.

I was using std::wstring for modern WebKit part in my original
patch (Bug 184770 Comment 1). But, I'm not confident std::wstring
is the best choice for them. How do you think?

[Basuke Suzuki]

(In reply to Fujii Hironori from comment #7)
> Comment on attachment 350706 [details]
> FIX
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350706&action=review
> 
> > Tools/MiniBrowser/win/Common.cpp:125
> > +    _bstr_t& password;
> 
> The reason why I don't like this patch is I don't want to use
> _bstr_t in common code and modern WebKit part becuase _bstr_t is
> a string type for COM. This is not problem for WebKitLegacy
> because it is a COM API.
> 
> I was using std::wstring for modern WebKit part in my original
> patch (Bug 184770 Comment 1). But, I'm not confident std::wstring
> is the best choice for them. How do you think?

I'm still very beginner programmer for Win32 platform. I've asked to my team mate what is BSTR. I've used wstring on my previous implementation for legacy, but I found that was converted to _bstr_t at the end so that I decided to use _bstr_t.

I cannot believe why MS uses _ for such a public API even if it is a wrapper. Also it's not easy to manipulate. I want to minimize the usage of _bstr_t, limiting to the very far end of passing Win32 API.

If you agree, I want to stick with std::wstring for Win app coding as much as possible.

[Stephan Szabo]

I would say that wstring makes sense for at least these kinds of uses in the shared code in minibrowser. The other option that may make sense in some cases is WKStringRef if we're going primarily from one WK api to another, rather than converting to wstring and then back.

[Basuke Suzuki]

(In reply to Stephan Szabo from comment #9)
> I would say that wstring makes sense for at least these kinds of uses in the
> shared code in minibrowser. The other option that may make sense in some
> cases is WKStringRef if we're going primarily from one WK api to another,
> rather than converting to wstring and then back.

Converting from WKStringRef to _bstr_t is not possible directly. _bstr_t  constructors requires null terminated string, not data + length. So at least some sort of simple buffer is required to get string contents from WKStringRef and to pass _bstr_t. std::wstring satisfies those requirements.

[Stephan Szabo]

The side part of that was less for this case specifically, but more for a case where we have something like:

Call WKFoo() to get a string (as WKStringRef) and store that for later on an object.

<on some later user interaction>
Call WKBar(str) with that string

Where the only/primary way the string is used is to pass back to another WK API, it may make sense to keep that as a WKStringRef rather than converting it into a wstring.

[Fujii Hironori]

(In reply to Basuke Suzuki from comment #8)
> I'm still very beginner programmer for Win32 platform. I've asked to my team
> mate what is BSTR. I've used wstring on my previous implementation for
> legacy, but I found that was converted to _bstr_t at the end so that I
> decided to use _bstr_t.
> 
> I cannot believe why MS uses _ for such a public API even if it is a
> wrapper. Also it's not easy to manipulate. I want to minimize the usage of
> _bstr_t, limiting to the very far end of passing Win32 API.
> 
> If you agree, I want to stick with std::wstring for Win app coding as much
> as possible.

_bstr_t is a useful wrapper class for BSTR. I'd like to use it in
WebKitLegacyBrowserWindow. In C++, any names prefixed with _ are
preserved for the system. It is safe for the system to introduce
a new name with prefix _ without breaking existing code. I think
this is the reason _bstr_t is _-prefixed.

There is another BSTR.
Source/WebCore/platform/win/BString.h
But, I don't like the idea using this in MiniBrowser.
I'd like MiniBrowser use only public WebKit API (WK1 and WK2).

But, this is just my opition. MiniBrowser already is using WebCore header.
#include <WebCore/COMPtr.h>
https://github.com/WebKit/webkit/blob/907de274451932ef356129302afef6208ff21ce5/Tools/MiniBrowser/win/ResourceLoadDelegate.cpp#L31

[Basuke Suzuki]

Created attachment 350886 [details]
PATCH

Use std::wstring more.

[EWS Watchlist]

Attachment 350886 [details] did not pass style-queue:


ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Fujii Hironori]

Comment on attachment 350886 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=350886&action=review

> Tools/MiniBrowser/win/Common.cpp:163
> +std::optional<Credential> askCredential(HWND hwnd, const std::wstring& realm)

askCredential is a strange name for me because Credential is a piece of information provided by server.
https://en.wikipedia.org/wiki/Credential

> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:41
> +    size_t actualLength = WKStringGetCharacters(wkString, str.data(), maxSize);

str.data() can not be modified in C++14 which we are using. This is the reason why Chris uses std::vector<char> (Bug 187167).
But, it seems OK in C++17.
https://en.cppreference.com/w/cpp/string/basic_string/data
I am OK if this doesn't emit a compiler warning.

[Fujii Hironori]

(In reply to Fujii Hironori from comment #15)
> askCredential is a strange name for me because Credential is a piece of
> information provided by server.

Never mind this my comment. WK2 API has a API WKCredentialCreate.
WKCredentialCreate(WKStringRef username, WKStringRef password, WKCredentialPersistence);

[WebKit Commit Bot]

Comment on attachment 350886 [details]
PATCH

Clearing flags on attachment: 350886

Committed r236549: <https://trac.webkit.org/changeset/236549>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44832874>