Add preferred alternatives to problematic terminology #40348
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ect#30789 - Add computeInCoordinatingFrame as alias for computeInMasterFrame - Add coordinator property as alias for master - Add isCoordinator property as alias for isMaster - Update documentation to reference preferred alternatives - Maintain backward compatibility with existing APIs
- Add test for computeInCoordinatingFrame function alias - Add tests for coordinator and isCoordinator property aliases - Add test for computeInCoordinatingFrame method delegation - Use AMP-style focused, minimal test approach - Achieve 100% coverage for new alias functionality
StarbirdTech
force-pushed
the
fix-terminology-issue-30789
branch
from
c604b88 to
a198e0c
Compare
StarbirdTech
force-pushed
the
fix-terminology-issue-30789
branch
from
35a3986 to
d5dce51
Compare
|
heya @StarbirdTech thanks for this! is the change purely a terminology change? (just double checking) @powerivq can you help review this |
erwinmombay
reviewed
Jul 7, 2025
| @@ -279,9 +279,13 @@ documentation with the new behaviors on user's consent choices. You can refer to | |||
|
|
|||
| #### JS reuse across iframes | |||
|
|
|||
| To allow ads to bundle HTTP requests across multiple ad units on the same page the object `window.context.master` will contain the window object of the iframe being elected master iframe for the current page. The `window.context.isMaster` property is `true` when the current frame is the master frame. | |||
| To allow ads to bundle HTTP requests across multiple ad units on the same page, use `window.context.coordinator` to access the coordinating iframe window object, and `window.context.isCoordinator` to check if the current frame is the coordinating frame. | |||
There was a problem hiding this comment.
@powerivq do you know if 3rd party code would need to make changes for this?
There was a problem hiding this comment.
They will. The best we can do is to offer an alias therefore.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces preferred aliases for “master” terminology across the 3p integration, externs, documentation, and tests, preserving full backward compatibility.
- Adds
computeInCoordinatingFrameas a functional alias tocomputeInMasterFrame - Introduces
coordinatorandisCoordinatorgetters inIntegrationAmpContext - Updates documentation, externs, and tests to lead with new terminology
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/test-3p.js | Added import and test for computeInCoordinatingFrame |
| test/unit/3p/test-ampcontext-integration.js | Added tests for coordinator, isCoordinator, and alias method |
| build-system/externs/amp.extern.js | Declared new externs context.coordinator and context.isCoordinator |
| ads/README.md | Updated JS reuse docs to use new terminology |
| 3p/ampcontext-integration.js | Added coordinator/isCoordinator getters and alias method |
| 3p/3p.js | Added computeInCoordinatingFrame alias to computeInMasterFrame |
Comments suppressed due to low confidence (2)
ads/README.md:282
- [nitpick] This line is formatted as a paragraph while adjacent items are list entries; consider adding a leading dash or matching the list style for consistency.
To allow ads to bundle HTTP requests across multiple ad units on the same page, use `window.context.coordinator` to access the coordinating iframe window object, and `window.context.isCoordinator` to check if the current frame is the coordinating frame.
test/unit/test-3p.js:2
- [nitpick] Align the indentation of imported names in the destructuring block for consistency with other imports.
computeInCoordinatingFrame,
Yes this should only be a terminology change. |
Co-authored-by: Copilot <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses issue #30789 by adding functional alternatives to problematic terminology while maintaining full backward compatibility.
Changes
computeInCoordinatingFrame()as alias forcomputeInMasterFrame()coordinatoras alias formasterisCoordinatoras alias forisMasterFiles Modified
3p/3p.js: AddedcomputeInCoordinatingFramefunction3p/ampcontext-integration.js: AddedcoordinatorandisCoordinatorgettersbuild-system/externs/amp.extern.js: Added extern definitionsads/README.md: Updated documentation to lead with new terminologytest/unit/test-3p.js: Added functional testtest/unit/3p/test-ampcontext-integration.js: Added alias testsBackward Compatibility
All existing APIs continue to work unchanged. This provides a migration path to new terminology without breaking existing code.
Test Plan
Fixes #30789