Bug 193029 – [Win][Clang] Fix compilation warnings of MiniBrowser

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Fujii Hironori]

[Win][Clang] Fix compilation warnings of MiniBrowser

clang-cl reports the following warnings:

> ..\..\Tools\MiniBrowser\win/MiniBrowserReplace.h(32,13):  warning: unused function 'processCrashReport' [-Wunused-function]
> ..\..\Tools\MiniBrowser\win/MiniBrowserWebHost.h(37,11):  warning: field 'm_client' will be initialized after field 'm_hURLBarWnd' [-Wreorder]
> ..\..\Tools\MiniBrowser\win/MiniBrowserWebHost.h(81,13):  warning: private field 'm_URLBarFont' is not used [-Wunused-private-field]
> ..\..\Tools\MiniBrowser\win/MiniBrowserWebHost.h(82,13):  warning: private field 'm_oldFont' is not used [-Wunused-private-field]
> ..\..\Tools\MiniBrowser\win/PageLoadTestClient.h(149,14):  warning: private field 'm_currentURLIndex' is not used [-Wunused-private-field]
> ..\..\Tools\MiniBrowser\win\MiniBrowserWebHost.cpp(170,38):  warning: ISO C++11 does not allow conversion from string literal to 'BSTR' (aka 'wchar_t *') [-Wwritable-strings]
> ..\..\Tools\MiniBrowser\win\MiniBrowserWebHost.cpp(178,35):  warning: ISO C++11 does not allow conversion from string literal to 'BSTR' (aka 'wchar_t *') [-Wwritable-strings]
> ..\..\Tools\MiniBrowser\win\MiniBrowserWebHost.cpp(178,70):  warning: ISO C++11 does not allow conversion from string literal to 'LPWSTR' (aka 'wchar_t *') [-Wwritable-strings]
> ..\..\Tools\MiniBrowser\win\PageLoadTestClient.cpp(40,7):  warning: field 'm_repetitions' will be initialized after field 'm_waitForLoadToReallyEnd' [-Wreorder]
> ..\..\Tools\MiniBrowser\win\WebKitBrowserWindow.cpp(107,61):  warning: missing field 'decidePolicyForNavigationAction' initializer [-Wmissing-field-initializers]
> ..\..\Tools\MiniBrowser\win\WebKitLegacyBrowserWindow.cpp(649,24):  warning: ISO C++11 does not allow conversion from string literal to 'TCHAR *' (aka 'wchar_t *') [-Wwritable-strings]
> ..\..\Tools\MiniBrowser\win\WebKitLegacyBrowserWindow.cpp(71,7):  warning: field 'm_useLayeredWebView' will be initialized after field 'm_pageLoadTestClient' [-Wreorder]

[Fujii Hironori]

Created attachment 358059 [details]
Patch

[Fujii Hironori]

PageLoadTestClient::pageLoadStartedAtTime and PageLoadTestClient::pageLoadEndedAtTime have been introduced in Bug 137673.
They has been virtual since the beginning. But, nothing overrides.

[Ross Kirsling]

Comment on attachment 358059 [details]
Patch

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

> ..\..\Tools\MiniBrowser\win/MiniBrowserReplace.h(32,13):  warning: unused function 'processCrashReport' [-Wunused-function]

Did this one get addressed? I'm not noticing a related code change here.
But then it seems like this function is used in Common.cpp so I'm not certain why this warning occurs...

> Tools/ChangeLog:15
> +        * MiniBrowser/win/MiniBrowserWebHost.h: Removed unused m_oldFont
> +        and m_URLBarFont. Reoder member variables to fix -Wreorder
> +        warning.
> +        * MiniBrowser/win/PageLoadTestClient.cpp:
> +        (PageLoadTestClient::PageLoadTestClient): Reoder the member initializer list.

Nit: "Reorder" x2

> Tools/ChangeLog:23
> +        (initDocStruct): Changed the arugment type docname to const.

Nit: "argument"

> Tools/MiniBrowser/win/MiniBrowserWebHost.cpp:178
> -    hr = target->addEventListener(L"click", new SimpleEventListener (L"webkit logo click"), FALSE);
> +    hr = target->addEventListener(const_cast<wchar_t*>(L"click"), new SimpleEventListener (const_cast<wchar_t*>(L"webkit logo click")), FALSE);

It feels unfortunate to have to cast away const on all of these literals. Does BSTR imply that there's no other way?

[Fujii Hironori]

Comment on attachment 358059 [details]
Patch

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

>> Tools/MiniBrowser/win/MiniBrowserWebHost.cpp:178
>> +    hr = target->addEventListener(const_cast<wchar_t*>(L"click"), new SimpleEventListener (const_cast<wchar_t*>(L"webkit logo click")), FALSE);
> 
> It feels unfortunate to have to cast away const on all of these literals. Does BSTR imply that there's no other way?

Good catch. I should change IDL files with "const BSTR".

[Fujii Hironori]

(In reply to Ross Kirsling from comment #3)
> > ..\..\Tools\MiniBrowser\win/MiniBrowserReplace.h(32,13):  warning: unused function 'processCrashReport' [-Wunused-function]
> 
> Did this one get addressed? I'm not noticing a related code change here.
> But then it seems like this function is used in Common.cpp so I'm not
> certain why this warning occurs...

Yeah, I haven't fixed it yet because I don't know how to fix it.
MainWindow.cpp is including MiniBrowserReplace.h.

> In file included from ..\..\Tools\MiniBrowser\win\WinMain.cpp:34:
> ..\..\Tools\MiniBrowser\win/MiniBrowserReplace.h(32,13):  warning: unused function 'processCrashReport' [-Wunused-function]
> static void processCrashReport(const wchar_t* fileName) { ::MessageBox(0, fileName, L"Crash Report", MB_OK); }
>             ^

[Fujii Hironori]

(In reply to Fujii Hironori from comment #4)
> Comment on attachment 358059 [details]
> >> Tools/MiniBrowser/win/MiniBrowserWebHost.cpp:178
> >> +    hr = target->addEventListener(const_cast<wchar_t*>(L"click"), new SimpleEventListener (const_cast<wchar_t*>(L"webkit logo click")), FALSE);
> > 
> > It feels unfortunate to have to cast away const on all of these literals. Does BSTR imply that there's no other way?
> 
> Good catch. I should change IDL files with "const BSTR".

There are too many BSTR in IDL files which can be prefixed with 'const'. I will do it in another bug ticket.

[Fujii Hironori]

Created attachment 358095 [details]
Patch

[Ross Kirsling]

Comment on attachment 358095 [details]
Patch

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

r=me with a final question.

> Tools/MiniBrowser/win/WebKitLegacyBrowserWindow.cpp:619
> -static void initDocStruct(DOCINFO* di, TCHAR* docname)
> +static void initDocStruct(DOCINFO* di, const wchar_t* docname)
>  {
>      memset(di, 0, sizeof(DOCINFO));
>      di->cbSize = sizeof(DOCINFO);
> -    di->lpszDocName = docname;
> +    di->lpszDocName = const_cast<wchar_t*>(docname);

According to MSDN, it seems that lpszDocName should be LPCSTR:
https://docs.microsoft.com/en-us/windows/desktop/api/wingdi/ns-wingdi-_docinfoa

If so, isn't it okay for docname to be const?

[Fujii Hironori]

Comment on attachment 358095 [details]
Patch

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

>> Tools/MiniBrowser/win/WebKitLegacyBrowserWindow.cpp:619
>> +    di->lpszDocName = const_cast<wchar_t*>(docname);
> 
> According to MSDN, it seems that lpszDocName should be LPCSTR:
> https://docs.microsoft.com/en-us/windows/desktop/api/wingdi/ns-wingdi-_docinfoa
> 
> If so, isn't it okay for docname to be const?

Good catch! Thank you.

[Fujii Hironori]

Created attachment 358474 [details]
Patch for landing

[Fujii Hironori]

Comment on attachment 358474 [details]
Patch for landing

Clearing flags on attachment: 358474

Committed r239666: <https://trac.webkit.org/changeset/239666>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47074949>

[Fujii Hironori]

(In reply to Fujii Hironori from comment #6)
> There are too many BSTR in IDL files which can be prefixed with 'const'. I
> will do it in another bug ticket.

Filed:

  Bug 193271 – [Win][WebKitLegacy] wchar_t strings shouldn't be treated as BSTR