Skip to content

Document frozen embedder API structs - #186842

Merged
auto-submit[bot] merged 9 commits into
flutter:masterfrom
mvincentong:docs-embedder-sealed-structs
Aug 5, 2026
Merged

Document frozen embedder API structs#186842
auto-submit[bot] merged 9 commits into
flutter:masterfrom
mvincentong:docs-embedder-sealed-structs

Conversation

@mvincentong

@mvincentong mvincentong commented May 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Issue

Part of #38470. The issue asks for embedder API structs that must not be extended in place to be documented. Review feedback asked what basis was used to decide which structs were frozen.

Fix

Narrowed the PR to document only the structs that are already protected by the EmbedderFrozen golden test in engine/src/flutter/shell/platform/embedder/tests/embedder_frozen_unittests.cc.

This removes the earlier unsupported ABI-sealed labels from renderer, backing-store, task, Dart object, and AOT structs that are not covered by that test-backed source of truth.

Tests

  • git diff --check
  • Not run: clang-format --dry-run --Werror engine/src/flutter/shell/platform/embedder/embedder.h, because clang-format is not installed in this checkout.

Risk

Docs-only change to engine/src/flutter/shell/platform/embedder/embedder.h. No generated output. Other possible sealed embedder structs may still need a separate verified pass before being documented.

@mvincentong
mvincentong requested a review from loic-sharma as a code owner May 21, 2026 04:00
@flutter-dashboard

flutter-dashboard Bot commented May 21, 2026

Copy link
Copy Markdown

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

@github-actions github-actions Bot added the engine flutter/engine related. See also e: labels. label May 21, 2026
@mvincentong

mvincentong commented May 21, 2026

Copy link
Copy Markdown
Contributor Author

This PR is documentation-only for the public embedder C header and does not change runtime behavior. Local verification: git diff --check passed. I also attempted clang-format --dry-run --Werror engine/src/flutter/shell/platform/embedder/embedder.h, but clang-format is not installed in this checkout and no buildtools clang-format binary is present.

gemini-code-assist[bot]
gemini-code-assist Bot reviewed May 21, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the embedder.h header to designate several structures as ABI-sealed, establishing a policy that these types must not have fields added to them to maintain binary compatibility. Instead, the documentation specifies that new versioned structures should be introduced for any necessary additions. This change affects a wide range of structures, including those for geometry, renderer configurations, backing stores, and task runners. I have no feedback to provide.

@gaaclarke
gaaclarke removed the request for review from loic-sharma June 2, 2026 17:41
gaaclarke
gaaclarke requested changes Jun 2, 2026
View reviewed changes

@gaaclarke gaaclarke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What basis did you use to determine if these structs are ABI-sealed or not?

@loic-sharma

loic-sharma commented Jun 4, 2026

Copy link
Copy Markdown
Member

FYI we have this test to ensure frozen embedder APIs aren't modified: https://github.com/flutter/flutter/blob/master/engine/src/flutter/shell/platform/embedder/tests/embedder_frozen_unittests.cc

@mvincentong
mvincentong force-pushed the docs-embedder-sealed-structs branch from 29685f8 to 45f33e2 Compare June 4, 2026 03:18
@mvincentong

mvincentong commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

Updated this in 45f33e2260 to make the basis explicit.

The PR now documents only the structs already covered by the existing EmbedderFrozen golden test that Loic pointed to:

  • FlutterTransformation
  • FlutterRect
  • FlutterPoint
  • FlutterDamage
  • FlutterSemanticsNode
  • FlutterSemanticsCustomAction

I removed the broader ABI-sealed labels from renderer, backing-store, task, Dart object, and AOT structs since those were not backed by that test. I also updated the PR body to call out the remaining risk separately.

Verified:

  • git diff --check

I also retried clang-format --dry-run --Werror engine/src/flutter/shell/platform/embedder/embedder.h, but clang-format is still not installed in this checkout.

@mvincentong mvincentong changed the title Document sealed embedder ABI structs Document frozen embedder API structs Jun 4, 2026
@gaaclarke gaaclarke added the CICD Run CI/CD label Jun 4, 2026
gaaclarke
gaaclarke previously approved these changes Jun 4, 2026
View reviewed changes

@gaaclarke gaaclarke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now that we have proof that the docstrings are accurate.

@gaaclarke
gaaclarke requested a review from loic-sharma June 4, 2026 15:17
@mvincentong

mvincentong commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

The public presubmits are green, but Google testing is failing at the internal go/frob_pr/186842 link. I do not have access to that log from here; could someone with access share whether this needs an author change or a rerun?

@github-actions github-actions Bot removed the CICD Run CI/CD label Jun 25, 2026
@gaaclarke gaaclarke added the CICD Run CI/CD label Jun 25, 2026
@gaaclarke

gaaclarke commented Jun 25, 2026

Copy link
Copy Markdown
Member

Google testing wasn't holding you back, we need another reviewer. friendly ping @loic-sharma

@mvincentong

mvincentong commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

I checked the public Linux windows_arm_host_engine failure. The parent shard fails only because child build ci\host_release_arm64 failed in the RBE build step; the visible logs point at depsscannerclient / compiler-info parsing of a clang-cl -std=c11 warning, with no source diagnostic or embedder.h reference. This still looks unrelated to this docs-only diff.

@flutter-dashboard flutter-dashboard Bot removed the CICD Run CI/CD label Aug 3, 2026
@loic-sharma loic-sharma added the CICD Run CI/CD label Aug 3, 2026
loic-sharma
loic-sharma approved these changes Aug 3, 2026
View reviewed changes

@loic-sharma loic-sharma left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 3, 2026
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 3, 2026
@auto-submit

auto-submit Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

autosubmit label was removed for flutter/flutter/186842, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label.

@loic-sharma

loic-sharma commented Aug 5, 2026

Copy link
Copy Markdown
Member

Rerunning the check that failed...

@loic-sharma loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 5, 2026
@auto-submit
auto-submit Bot added this pull request to the merge queue Aug 5, 2026
Merged via the queue into flutter:master with commit 222295b Aug 5, 2026
34 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 5, 2026
This was referenced Aug 5, 2026
Closed
Closed
Closed
Closed
Closed
Closed
Merged
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Aug 10, 2026
@engine-flutter-autoroll
Manual roll Flutter from b766512c65d8 to 27b098811f3b (29 revisions) (#…
19a9f91 
…12420)

Manual roll requested by [email protected]

flutter/flutter@b766512...27b0988

2026-08-05 [email protected] reland(tool): remove redundant --enable-experiment=record-use flag (flutter/flutter#190591)
2026-08-05 [email protected] Windows: Propagate enabled accessibility state (flutter/flutter#190507)
2026-08-05 [email protected] Roll Fuchsia Test Scripts from ltbuIH9Z3T_yOuigu... to vcANVO8VIDQHasH1X... (flutter/flutter#190589)
2026-08-05 [email protected] Document frozen embedder API structs (flutter/flutter#186842)
2026-08-05 [email protected] Document super call order for State.didChangeDependencies (flutter/flutter#185945)
2026-08-05 [email protected] Move examples of `flutter/widgets` widgets out from `flutter/material` (flutter/flutter#189532)
2026-08-05 [email protected] [web] Use thread local strike caches in skwasm (flutter/flutter#190048)
2026-08-05 [email protected] doc: fix typo in see also section for PrimaryScrollController.maybeOf (flutter/flutter#190386)
2026-08-05 [email protected] Migrate the shell unit tests from legacy Dart native functions to FFI (flutter/flutter#190473)
2026-08-05 [email protected] render proxy box now defaults baseline calculation to null (flutter/flutter#190269)
2026-08-05 [email protected] Update Widgets Localizations from Translation Console (flutter/flutter#190503)
2026-08-04 [email protected] Revert: fix(tool): remove redundant --enable-experiment=record-use flag (flutter/flutter#190583)
2026-08-04 [email protected] Roll Fuchsia Test Scripts from 1frGe_KltAJKkeyPg... to ltbuIH9Z3T_yOuigu... (flutter/flutter#190561)
2026-08-04 [email protected] iOS,macOS: add tsan and ubsan support for Swift (flutter/flutter#190497)
2026-08-04 [email protected] fix: update on_message_ to nullptr after window destroy so that dart gets destroy message  (flutter/flutter#185807)
2026-08-04 [email protected] [flutter_tools] Gracefully handle locked Windows files during clean (flutter/flutter#190095)
2026-08-04 [email protected] iOS,macOS: make Logger thread-safe, conform to Sendable (flutter/flutter#190488)
2026-08-04 [email protected] iOS: Eliminate use of IOSContextNoop in platform view tests (reland) (flutter/flutter#190509)
2026-08-04 [email protected] fix(tool): remove redundant --enable-experiment=record-use flag (flutter/flutter#190475)
2026-08-04 [email protected] android_hardware_smoke_test: Detect blank image failures or EGL initialization warnings and retry (flutter/flutter#190110)
2026-08-04 [email protected] Bumps text gamma on windows to match skia. (flutter/flutter#190477)
2026-08-04 [email protected] Roll Skia from 48b58ee222f1 to a8583a0a2c11 (2 revisions) (flutter/flutter#190537)
2026-08-04 [email protected] [tool][web] Intercept dart2wasm errors & append JS migration footers (flutter/flutter#190476)
2026-08-04 [email protected] [record_use] Migrate IconTreeShaker to `package:record_use` (flutter/flutter#190225)
2026-08-04 [email protected] Handle unexpected exceptions during Azure metadata detection (flutter/flutter#189457)
2026-08-04 [email protected] Fix merge conflict from flutter/flutter#190369 (flutter/flutter#190544)
2026-08-04 [email protected] Prepare device support symbols (flutter/flutter#190369)
2026-08-04 [email protected] Roll Packages from ac87e65 to 3498b9d (1 revision) (flutter/flutter#190532)
2026-08-04 [email protected] Roll Skia from a08d918ebd6a to 48b58ee222f1 (11 revisions) (flutter/flutter#190527)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@loic-sharma loic-sharma mentioned this pull request Oct 16, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@loic-sharma loic-sharma loic-sharma approved these changes
@gaaclarke gaaclarke gaaclarke approved these changes
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CICD Run CI/CD engine flutter/engine related. See also e: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mvincentong @loic-sharma @gaaclarke