It was implemented for WebKitLegacy, but not for WebKit.
Created attachment 350431 [details] PATCH
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.
Created attachment 350454 [details] PATCH
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.
Created attachment 350706 [details] FIX Fix style. Didn't fix those in the tool generated files.
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.
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?
(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.
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.
(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.
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.
(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
Created attachment 350886 [details] PATCH Use std::wstring more.
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.
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.
(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);
Comment on attachment 350886 [details] PATCH Clearing flags on attachment: 350886 Committed r236549: <https://trac.webkit.org/changeset/236549>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44832874>