Run clang-tidy on CEF sources

[magreenblatt]

Is your feature request related to a problem? Please describe. clang-tidy is a clang-based C++ “linter” tool. Its purpose is to provide an extensible framework for diagnosing and fixing typical programming errors, like style violations, interface misuse, or bugs that can be deduced via static analysis.

Chromium provides clang-tidy integration that can be run locally as documented here.

Describe the solution you'd like Run clang-tidy on CEF sources.

Additional context Running clang-tidy on Windows is not perfectly documented by Chromium. The below steps work for me (at M121).

1. Start with the MasterBuildQuickStart instructions to create a local CEF/Chromium checkout.
2. Add enable_precompiled_headers=false to GN_DEFINES (details).
3. Add "C:\code\chromium_git\chromium\src\third_party\ninja" before "C:\code\depot_tools" in your system PATH (this is required for CMake to find ninja.exe instead of ninja.bat).
4. Run the cef_create_projects.bat script to create/update the out/Debug_GN_x64 directory.
5. Run the following commands:

   
   cd C:\code\chromium_git\chromium\src

:: Initialize the VS2022 build environment that will be used by Ninja "C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Auxiliary\Build\vcvars64.bat"

:: Download llvm sources and build clang-tidy andclang-apply-replacements` exes using CMake and Ninja. python3 tools/clang/scripts/build_clang_tools_extra.py --fetch out/Debug_GN_x64 clang-tidy clang-apply-replacements

6. In a new console window (to clear env), run the following commands:

cd C:\code\chromium_git\chromium\src

:: Generate the out/Debug_GN_x64/compile_commands.json file used by clang-tidy. set DEPOT_TOOLS_WIN_TOOLCHAIN=0 gn gen out/Debug_GN_x64 --export-compile-commands

7. Edit the  `out/Debug_GN_x64/compile_commands.json` file replacing "/Fo obj/" with "/Foobj/" ([details](https://github.com/chromiumembedded/cef/issues/3632#issuecomment-1901573233)).
8. Optionally edit the `out\Debug_GN_x64\tools\clang\third_party\llvm\clang-tools-extra\clang-tidy\tool\run-clang-tidy.py` script to make debugging easier ([details](https://github.com/chromiumembedded/cef/issues/3632#issuecomment-1899536513)).
9. Run the following commands:

cd C:\code\chromium_git\chromium\src

:: Build the cef target (above commands may invalidate an existing build). autoninja -C out/Debug_GN_x64 cef

cd out/Debug_GN_x64 set LLVM_SRC=C:\code\chromium_git\chromium\src\out\Debug_GN_x64\tools\clang\third_party\llvm

:: Run clang-tidy to fix files in the cef/tests/cefsimple directory. :: Run 2+ times to apply incremental fixes. :: Modify the -fix and/or -header-filter arguments to target different directories. :: -fix uses escaped backslashes to match file paths. :: -header-filter uses forward slashes to match include paths. python3 %LLVM_SRC%\clang-tools-extra\clang-tidy\tool\run-clang-tidy.py -p . -clang-tidy-binary %LLVM_SRC%\build\bin\clang-tidy.exe -clang-apply-replacements-binary %LLVM_SRC%\build\bin\clang-apply-replacements.exe -fix "cef\tests\cefsimple\." -header-filter="tests/cefsimple/."

:: Fix code style issues left after clang-tidy fixes. cd C:\code\chromium_git\chromium\src\cef tools\fix_style.bat unstaged

10. Review the code changes for any issues (examples [here](https://github.com/chromiumembedded/cef/issues/3632#issuecomment-1901650105)).
11. Review clang-tidy output for any remaining/unfixed warnings (examples [here](https://github.com/chromiumembedded/cef/issues/3632#issuecomment-1902731922) and [here](https://github.com/chromiumembedded/cef/commit/a0446a3c8ac686d970a693ebf4bac386b93b0531)).
12. Build and run ceftests on all platforms to verify that changes are correct.

[magreenblatt]

To help with debugging the -fix regex you can patch the out\Debug_GN_x64\tools\clang\third_party\llvm\clang-tools-extra\clang-tidy\tool\run-clang-tidy.py script as follows (to print matching file paths):

diff --git run-clang-tidy.py run-clang-tidy.py
index 70f8cbcdcb2f..7e1246074f8f 100755
--- run-clang-tidy.py
+++ run-clang-tidy.py
@@ -495,6 +495,7 @@ def main():
         # Fill the queue with files.
         for name in files:
             if file_name_re.search(name):
+                print(name)
                 task_queue.put(name)

         # Wait for all threads to be done.

[magreenblatt]

:: Generate the out/Debug_GN_x64/compile_commands.json file used by clang-tidy. set DEPOT_TOOLS_WIN_TOOLCHAIN=0 gn gen out/Debug_GN_x64 --export-compile-commands

Something about this step seems to invalidate existing builds, so use with caution.

[magreenblatt]

Looks like there's a bug in the generation of compile_commands.json

> C:\code\chromium_git\tools\clang\third_party\llvm\build\bin\clang-tidy.exe -header-filter=cef\\tests\\cefsimple\\.* -export-fixes C:\Users\Marshall\AppData\Local\Temp\tmplq4h6beh\tmp9244l3bx.yaml -p=. C:\code\chromium_git\chromium\src\cef\tests\cefsimple\cefsimple_win.cc
Error while processing C:\code\chromium_git\chromium\src\cef\tests\cefsimple\simple_handler.cc.
error: obj/cef/cefsimple/simple_handler.obj: 'linker' input unused [clang-diagnostic-unused-command-line-argument]
Error opening output file: no such file or directory

The correct command can be observed by running:

autoninja --verbose -C out\Debug_GN_x64 obj/cef/cefsimple/simple_handler.obj 

Comparing that output with the line from compile_commands.json, we see that replacing: /Fo obj/cef/cefsimple/simple_handler.obj with /Foobj/cef/cefsimple/simple_handler.obj fixes the issue.

Filed with Chromium as https://bugs.chromium.org/p/chromium/issues/detail?id=1520150

[magreenblatt]

Running run-clang-tidy.py multiple times for the same path may apply incremental fixes. For example:

First pass:

diff --git tests/cefclient/browser/client_handler.h tests/cefclient/browser/client_handler.h
index f1f9fe1b9..f5b660b20 100644
--- tests/cefclient/browser/client_handler.h
+++ tests/cefclient/browser/client_handler.h
@@ -439,9 +439,9 @@ class ClientHandler : public CefClient,

   // Track state information for the text context menu.
   struct TestMenuState {
-    TestMenuState() : check_item(true), radio_item(0) {}
-    bool check_item;
-    int radio_item;
+    TestMenuState()  {}
+    bool check_item = true;
+    int radio_item = 0;
   } test_menu_state_;

   // The current number of browsers using this handler.

Second pass:

diff --git tests/cefclient/browser/client_handler.h tests/cefclient/browser/client_handler.h
index f5b660b20..b1a478b12 100644
--- tests/cefclient/browser/client_handler.h
+++ tests/cefclient/browser/client_handler.h
@@ -439,7 +439,7 @@ class ClientHandler : public CefClient,

   // Track state information for the text context menu.
   struct TestMenuState {
-    TestMenuState()  {}
+    TestMenuState()  = default;
     bool check_item = true;
     int radio_item = 0;
   } test_menu_state_;

[magreenblatt]

WIP PR: https://bitbucket.org/chromiumembedded/cef/pull-requests/713

We'll only run clang-tidy on the tests, libcef_dll/wrapper and libcef folders at this time. We don't want to format generated code or code copied from Chromium headers.

[magreenblatt]

Also keep an eye out for compile errors like:

• Unused variables (left behind by code changes).
• Includes added in incorrect locations (like inside #ifdefs).
• Macros that define virtual methods (not updated automatically).

And formatting errors that will need to be fixed manually like:

diff --git tests/cefclient/browser/osr_renderer_settings.h tests/cefclient/browser/osr_renderer_settings.h
index 22e4cda52..4b5b94add 100644
--- tests/cefclient/browser/osr_renderer_settings.h
+++ tests/cefclient/browser/osr_renderer_settings.h
@@ -11,9 +11,7 @@
 namespace client {

 struct OsrRendererSettings {
-  OsrRendererSettings()
-
-      = default;
+  OsrRendererSettings() = default;

   // If true draw a border around update rectangles.
   bool show_update_rect = false;

[magreenblatt]

Error when running run-clang-tidy.py on libcef/ source files:

error: PCH file built from a different branch ((https://chromium.googlesource.com/a/external/github.com/llvm/llvm-project eb1d5065c560b3468fa0d34af3103359cd78c088)) than the compiler ((https://github.com/llvm/llvm-project.git eb1d5065c560b3468fa0d34af3103359cd78c088)) [clang-diagnostic-error]

The solution is to build with GN arg enable_precompiled_headers=false.

Filed with Chromium as https://bugs.chromium.org/p/chromium/issues/detail?id=1520273

[magreenblatt]

Unresolved clang-tidy warnings (tests, libcef_dll/wrapper and libcef folders):

../../cef/libcef_dll/wrapper/cef_resource_manager.cc:652:12: warning: 'state' used after it was moved [bugprone-use-after-move]
  652 |     DCHECK(state->current_entry_pos_ != providers_.end());
      |            ^
../../cef/libcef_dll/wrapper/cef_resource_manager.cc:661:21: note: move occurred here
  661 |         StopRequest(std::move(state));
      |                     ^

../../cef/libcef/browser/file_dialog_manager.cc:518:10: warning: 'callback' used after it was moved [bugprone-use-after-move]
  518 |   return callback;
      |          ^
../../cef/libcef/browser/file_dialog_manager.cc:507:41: note: move occurred here
  507 |           new CefFileDialogCallbackImpl(std::move(callback)));
      |                                         ^

../../cef/libcef/browser/media_access_query.cc:346:8: warning: 'query' used after it was moved [bugprone-use-after-move]
  346 |   if (!query.is_null()) {
      |        ^
../../cef/libcef/browser/media_access_query.cc:326:15: note: move occurred here
  326 |           new CefMediaAccessCallbackImpl(std::move(query)));
      |               ^

../../cef/libcef/browser/download_manager_delegate.cc:162:17: warning: 'callback' used after it was moved [bugprone-use-after-move]
  162 |       std::move(callback).Run(
      |                 ^
../../cef/libcef/browser/download_manager_delegate.cc:155:13: note: move occurred here
  155 |             base::BindOnce(
      |             ^

../../cef/libcef/browser/main_runner.cc:285:39: warning: 'shutdown_on_ui_thread' used after it was moved [bugprone-use-after-move]
  285 |     StartShutdownOnUIThread(std::move(shutdown_on_ui_thread));
      |                                       ^
../../cef/libcef/browser/main_runner.cc:267:19: note: move occurred here
  267 |                   base::BindOnce(&CefMainRunner::StartShutdownOnUIThread,
      |                   ^
../../cef/libcef/browser/main_runner.cc:285:39: note: the use happens in a later loop iteration than the move
  285 |     StartShutdownOnUIThread(std::move(shutdown_on_ui_thread));
      |                                       ^

../../cef/libcef/browser/certificate_query.cc:122:8: warning: 'callback' used after it was moved [bugprone-use-after-move]
  122 |   if (!callback.is_null() && default_disallow) {
      |        ^
../../cef/libcef/browser/certificate_query.cc:107:54: note: move occurred here
  107 |             new CefAllowCertificateErrorCallbackImpl(std::move(callback)));
      |                                                      ^

[magreenblatt]

Unresolved clang-tidy warnings (tests, libcef_dll/wrapper and libcef folders)

These all look fine. Marking this issue as complete.