Add a verification step for typemap config [chromium/src : main]

[Daniel Cheng (Gerrit)]

Attention needed from Fred Shih

Daniel Cheng added 7 comments

Patchset-level comments

File-level comment, Patchset 23:

Fred Shih . resolved

Hi Daniel -- sorry for the large change. Turns out there was a lot of dead code. If this looks ok to you, I will bring in the other owners. Thanks!

Daniel Cheng

General approach seems good. I'm comfortable using OO+1 here as well. Let me know what you prefer.

File mojo/public/tools/bindings/mojom_bindings_generator.py

Line 187, Patchset 24 (Latest): if language == '_metadata':

Daniel Cheng . unresolved

What is `_metadata` used for? Is there somewhere we could add a comment? Perhaps in the docstring for `LoadTypemaps()`?

Line 195, Patchset 24 (Latest): ):

Daniel Cheng . resolved

... heh this is really awkward. Could use `lang` instead of `language` I guess...

Line 206, Patchset 24 (Latest): f"Unused typemaps found for {lang}:\n{sorted(typemaps)},\n\n" \

Daniel Cheng . unresolved

Nit: no \ needed here; we can rely on implicit line joining between parens

Line 267, Patchset 24 (Latest): def Check(kind, enclosing_kind=None):

Daniel Cheng . unresolved

I think a brief comment here would be helpful

File services/network/public/mojom/BUILD.gn

Line 1965, Patchset 24 (Parent): mojom = "network::mojom::ReportingApiReportStatus"

Daniel Cheng . unresolved

o.O

File third_party/blink/renderer/modules/serial/serial.cc

Line 175, Patchset 24 (Parent): ::bluetooth::mojom::blink::UUID::New(

Daniel Cheng . unresolved

oh.

Open in Gerrit

Related details

Attention is currently required from:

• Fred Shih

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review
• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I75b40055cf9cc045b5e7890365550d846fb71145

Gerrit-Change-Number: 6692233

Gerrit-PatchSet: 24

Gerrit-Owner: Fred Shih <ff...@chromium.org>

Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>

Gerrit-Reviewer: Fred Shih <ff...@chromium.org>

Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>

Gerrit-CC: Akihiro Ota <akihi...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>

Gerrit-CC: Nikhil Nayunigari <nikh...@google.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Fred Shih <ff...@chromium.org>

Gerrit-Comment-Date: Tue, 08 Jul 2025 04:23:40 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Fred Shih <ff...@chromium.org>

[Fred Shih (Gerrit)]

Attention needed from Daniel Cheng

Fred Shih added 7 comments

Patchset-level comments

File-level comment, Patchset 23:

Fred Shih . resolved

Hi Daniel -- sorry for the large change. Turns out there was a lot of dead code. If this looks ok to you, I will bring in the other owners. Thanks!

Daniel Cheng

General approach seems good. I'm comfortable using OO+1 here as well. Let me know what you prefer.

Fred Shih

Let's go with OO+1 here. thanks.

File mojo/public/tools/bindings/mojom_bindings_generator.py

Line 187, Patchset 24: if language == '_metadata':

Daniel Cheng . resolved

What is `_metadata` used for? Is there somewhere we could add a comment? Perhaps in the docstring for `LoadTypemaps()`?

Fred Shih

Done

Line 195, Patchset 24: ):

Daniel Cheng . resolved

... heh this is really awkward. Could use `lang` instead of `language` I guess...

Fred Shih

done

Line 206, Patchset 24: f"Unused typemaps found for {lang}:\n{sorted(typemaps)},\n\n" \

Daniel Cheng . resolved

Nit: no \ needed here; we can rely on implicit line joining between parens

Fred Shih

Done

Line 267, Patchset 24: def Check(kind, enclosing_kind=None):

Daniel Cheng . resolved

I think a brief comment here would be helpful

Fred Shih

Done

File services/network/public/mojom/BUILD.gn

Line 1965, Patchset 24 (Parent): mojom = "network::mojom::ReportingApiReportStatus"

Daniel Cheng . resolved

o.O

Fred Shih

My guess is that this has never worked..

File third_party/blink/renderer/modules/serial/serial.cc

Line 175, Patchset 24 (Parent): ::bluetooth::mojom::blink::UUID::New(

Daniel Cheng . resolved

oh.

Fred Shih

Yea, this one is particularly strange. I think because the typemap was declared on another build rule, the actual type in the generated struct can be different depending on your dep path...

Open in Gerrit

Related details

Attention is currently required from:

• Daniel Cheng

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I75b40055cf9cc045b5e7890365550d846fb71145

Gerrit-Change-Number: 6692233

Gerrit-PatchSet: 26

Gerrit-Owner: Fred Shih <ff...@chromium.org>

Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>

Gerrit-Reviewer: Fred Shih <ff...@chromium.org>

Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>

Gerrit-CC: Akihiro Ota <akihi...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>

Gerrit-CC: Nikhil Nayunigari <nikh...@google.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Daniel Cheng <dch...@chromium.org>

Gerrit-Comment-Date: Tue, 08 Jul 2025 21:41:53 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Daniel Cheng <dch...@chromium.org>

Comment-In-Reply-To: Fred Shih <ff...@chromium.org>

[Daniel Cheng (Gerrit)]

Attention needed from Fred Shih

Daniel Cheng voted and added 3 comments

Votes added by Daniel Cheng

Code-Review	+1
Owners-Override	+1

3 comments

Patchset-level comments

File-level comment, Patchset 26 (Latest):

Daniel Cheng . resolved

LGTM w/nits

File mojo/public/tools/bindings/mojom_bindings_generator.py

Line 273, Patchset 26 (Latest): """Remove all the types that were encountered in code generation from the

Daniel Cheng . unresolved

Given the docstring... I guess I would have expected this to be named something more like "RemoveUsedTypemaps" or "ConsumeUsedTypemaps"

Line 278, Patchset 26 (Latest): def Check(kind, enclosing_kind=None):

Daniel Cheng . unresolved

And something like "RemoveUsedKind" or "ConsumeUsedKind" here.

Open in Gerrit

Related details

Attention is currently required from:

• Fred Shih

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review
• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I75b40055cf9cc045b5e7890365550d846fb71145

Gerrit-Change-Number: 6692233

Gerrit-PatchSet: 26

Gerrit-Owner: Fred Shih <ff...@chromium.org>

Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>

Gerrit-Reviewer: Fred Shih <ff...@chromium.org>

Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>

Gerrit-CC: Akihiro Ota <akihi...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>

Gerrit-CC: Nikhil Nayunigari <nikh...@google.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Fred Shih <ff...@chromium.org>

Gerrit-Comment-Date: Wed, 09 Jul 2025 01:49:45 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Fred Shih (Gerrit)]

Fred Shih added 3 comments

Patchset-level comments

File-level comment, Patchset 27 (Latest):

Fred Shih . resolved

Thanks for the review!

File mojo/public/tools/bindings/mojom_bindings_generator.py

Line 273, Patchset 26: """Remove all the types that were encountered in code generation from the

Daniel Cheng . resolved

Given the docstring... I guess I would have expected this to be named something more like "RemoveUsedTypemaps" or "ConsumeUsedTypemaps"

Fred Shih

Done

Line 278, Patchset 26: def Check(kind, enclosing_kind=None):

Daniel Cheng . resolved

And something like "RemoveUsedKind" or "ConsumeUsedKind" here.

Fred Shih

Done

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I75b40055cf9cc045b5e7890365550d846fb71145

Gerrit-Change-Number: 6692233

Gerrit-PatchSet: 27

Gerrit-Owner: Fred Shih <ff...@chromium.org>

Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>

Gerrit-Reviewer: Fred Shih <ff...@chromium.org>

Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>

Gerrit-CC: Akihiro Ota <akihi...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>

Gerrit-CC: Nikhil Nayunigari <nikh...@google.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Comment-Date: Wed, 09 Jul 2025 21:33:01 +0000

[Fred Shih (Gerrit)]

Fred Shih voted Commit-Queue+2

Commit-Queue

+2

Gerrit-Comment-Date: Wed, 09 Jul 2025 21:44:02 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes

[Chromium LUCI CQ (Gerrit)]

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

26 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: mojo/public/tools/bindings/mojom_bindings_generator.py
Insertions: 7, Deletions: 7.

@@ -263,19 +263,19 @@
           filtered_args = [arg for arg in remaining_args
                            if arg.startswith(prefix)]
         generator.GenerateFiles(filtered_args)
-        self._CheckTypemapsUsed(language, module)
+        self._RemoveUsedTypemaps(language, module)
 
     # Save result.
     self._processed_files[rel_filename.path] = module
     return module
 
-  def _CheckTypemapsUsed(self, language, module):
+  def _RemoveUsedTypemaps(self, language, module):

     """Remove all the types that were encountered in code generation from the

     declared typemap list.
     """
 
     # Fully qualify the kind and remove it from the declared typemaps.
-    def Check(kind, enclosing_kind=None):
+    def RemoveUsedKind(kind, enclosing_kind=None):
       namespace = module.mojom_namespace
       if enclosing_kind:
         namespace += f".{enclosing_kind.name}"
@@ -285,16 +285,16 @@
         self._typemap_usage_verification[language].remove(name)
 
     for kind in module.unions + module.enums:
-      Check(kind)
+      RemoveUsedKind(kind)
 
     for struct_kind in module.structs:
-      Check(struct_kind)
+      RemoveUsedKind(struct_kind)
       for enum in struct_kind.enums:
-        Check(enum, struct_kind)
+        RemoveUsedKind(enum, struct_kind)
 
     for iface_kind in module.interfaces:
       for enum in iface_kind.enums:
-        Check(enum, iface_kind)
+        RemoveUsedKind(enum, iface_kind)
 
 def _Generate(args, remaining_args):
   if args.variant == "none":
```

Change information

Commit message:

Add a verification step for typemap config

If there is a typo in the typemap config, this will raise a warning
telling the developer that a typemap has not been referenced.

Most of the change is mechanical (i.e.: removing dead typemaps that have
no effect).

This change will force typemaps to be declared in the mojom rule that
contains the mojom module where the type is declared, which is probably
better because having another target declare the typemaps can lead to
some very... interesting.. side effects. One of these side effects is
that the type of the generated code will look different, depending on
your deps.

Bug: 428039213

Change-Id: I75b40055cf9cc045b5e7890365550d846fb71145

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6692233

Owners-Override: Daniel Cheng <dch...@chromium.org>

Reviewed-by: Daniel Cheng <dch...@chromium.org>

Commit-Queue: Fred Shih <ff...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1484659}

Files:

• M ash/BUILD.gn
• M ash/webui/diagnostics_ui/backend/BUILD.gn
• M ash/webui/personalization_app/mojom/BUILD.gn
• M ash/webui/shimless_rma/mojom/BUILD.gn
• M chrome/browser/ash/net/BUILD.gn
• M chrome/browser/ash/net/network_diagnostics/BUILD.gn
• M chrome/browser/ash/net/rollback_network_config/BUILD.gn
• M chrome/browser/ui/ash/network/BUILD.gn
• M chrome/common/search/BUILD.gn
• M chrome/services/printing/public/mojom/BUILD.gn
• M chrome/services/sharing/nearby/platform/BUILD.gn
• M chrome/test/data/webui/BUILD.gn
• M chromeos/ash/services/cros_healthd/public/mojom/BUILD.gn
• D chromeos/ash/services/cros_healthd/public/mojom/cros_healthd_probe_mojom_traits.cc
• D chromeos/ash/services/cros_healthd/public/mojom/cros_healthd_probe_mojom_traits.h
• M chromeos/ash/services/network_config/cros_network_config.cc
• M chromeos/components/cdm_factory_daemon/mojom/BUILD.gn
• M chromeos/crosapi/mojom/BUILD.gn
• M chromeos/services/network_config/public/mojom/BUILD.gn
• D chromeos/services/network_config/public/mojom/cros_network_config_mojom_traits.cc
• D chromeos/services/network_config/public/mojom/cros_network_config_mojom_traits.h
• M components/autofill/core/common/mojom/BUILD.gn
• M components/media_router/common/mojom/BUILD.gn
• M content/common/BUILD.gn
• M device/bluetooth/public/mojom/BUILD.gn
• M gpu/ipc/common/BUILD.gn
• M media/mojo/mojom/BUILD.gn
• M mojo/public/interfaces/bindings/tests/BUILD.gn
• M mojo/public/mojom/base/BUILD.gn
• M mojo/public/tools/bindings/generate_type_mappings.py
• M mojo/public/tools/bindings/mojom_bindings_generator.py
• M remoting/host/mojom/BUILD.gn
• M services/audio/public/mojom/BUILD.gn
• M services/device/public/mojom/BUILD.gn
• M services/network/public/mojom/BUILD.gn
• M services/tracing/public/mojom/BUILD.gn
• M services/viz/privileged/mojom/compositing/BUILD.gn
• M services/viz/privileged/mojom/gl/BUILD.gn
• M services/viz/public/mojom/BUILD.gn
• M third_party/blink/public/mojom/BUILD.gn
• M third_party/blink/renderer/modules/serial/serial.cc
• M third_party/blink/renderer/modules/serial/serial_port.cc
• M third_party/blink/renderer/modules/serial/serial_unittest.cc
• M ui/accessibility/mojom/BUILD.gn
• M ui/gfx/mojom/BUILD.gn

Change size: XL

Delta: 45 files changed, 446 insertions(+), 697 deletions(-)

Branch: refs/heads/main

Submit Requirements:

• Code-Review: +1 by Daniel Cheng

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: merged

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I75b40055cf9cc045b5e7890365550d846fb71145

Gerrit-Change-Number: 6692233

Gerrit-PatchSet: 28

Gerrit-Owner: Fred Shih <ff...@chromium.org>

Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>

Gerrit-Reviewer: Fred Shih <ff...@chromium.org>

Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>

Gerrit-CC: Akihiro Ota <akihi...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

[Fumitoshi Ukai (Gerrit)]

Fumitoshi Ukai added 1 comment

Patchset-level comments

File-level comment, Patchset 28 (Latest):

Fumitoshi Ukai . resolved

https://issuetracker.google.com/430747719 missing deps for obj/third_party/blink/public/mojom/mojom_platform_blink/array_buffer_contents.mojom-blink.o
?

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I75b40055cf9cc045b5e7890365550d846fb71145

Gerrit-Change-Number: 6692233

Gerrit-PatchSet: 28

Gerrit-Owner: Fred Shih <ff...@chromium.org>

Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>

Gerrit-Reviewer: Fred Shih <ff...@chromium.org>

Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>

Gerrit-CC: Akihiro Ota <akihi...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Fumitoshi Ukai <uk...@google.com>

Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>

Gerrit-CC: Nikhil Nayunigari <nikh...@google.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Comment-Date: Thu, 10 Jul 2025 04:44:38 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Fred Shih (Gerrit)]

Fred Shih has created a revert of this change

Open original change

Related details

Attention set is empty

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: revert