| Summary: | [Win][Clang] Fix compilation warnings of MiniBrowser | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||||
| Component: | Tools / Tests | Assignee: | Fujii Hironori <Hironori.Fujii> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | achristensen, bfulgham, don.olmstead, lforschler, pvollan, ross.kirsling, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 171618 | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 358059 [details]
Patch
PageLoadTestClient::pageLoadStartedAtTime and PageLoadTestClient::pageLoadEndedAtTime have been introduced in Bug 137673. They has been virtual since the beginning. But, nothing overrides. 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? 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". (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); } > ^ (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. Created attachment 358095 [details]
Patch
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? 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. Created attachment 358474 [details]
Patch for landing
Comment on attachment 358474 [details] Patch for landing Clearing flags on attachment: 358474 Committed r239666: <https://trac.webkit.org/changeset/239666> All reviewed patches have been landed. Closing bug. (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 |
[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]