Skip to content

Conversation

@bparrishMines
Copy link
Contributor

@bparrishMines bparrishMines commented Sep 7, 2025

Pre-Review Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

Footnotes

  1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. 2 3

@bparrishMines bparrishMines changed the title Ima ad event ad [interactive_media_ads] Sep 10, 2025
@bparrishMines bparrishMines changed the title [interactive_media_ads] [interactive_media_ads] Adds support for accessing data for an Ad Sep 11, 2025
@stuartmorgan-g
Copy link
Collaborator

The new files here will need to be updated to remove the "All rights reserved" to pass checks post-merge.

@bparrishMines bparrishMines marked this pull request as ready for review September 30, 2025 19:15
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

Minor comments, but otherwise LGTM

final String? apiFramework;

/// The height of the companion in pixels.
final int? height;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do null height/width mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

final String? resourceValue;

/// The width of the companion in pixels.
final int? width;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Height and width should probably be together in the member list and the constructor params.

final PlatformAd platform;

/// The ad ID as specified in the VAST response.
String get adId => platform.adId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is inconsistent with the previous file's capitalization of ID; the Dart API surface should consistently use one or the other. (Same with other names below.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Android and iOS use different capitalization so I end up switching on accident. It looks like the precedent is already to use lowercase so I updated the other values.

int get vastMediaWidth => platform.vastMediaWidth;

/// The width of the selected creative if non-linear, else returns 0.
int get width => platform.width;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are width and height so far apart?

Copy link
Contributor Author

@bparrishMines bparrishMines Oct 1, 2025

Choose a reason for hiding this comment

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

I was doing it in the same order as Android and it looks like they order methods alphabetically: https://developers.google.com/interactive-media-ads/docs/sdks/android/client-side/api/reference/com/google/ads/interactivemedia/v3/api/Ad

It does makes since to have these two together though.

/// a playlist.
///
/// DAI live stream: For a preroll pod, returns 0. For midrolls, returns the
/// break ID. Returns -2 if pod index cannot be determined (internal error).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make constants for these magic values, and document those instead of the raw values?

Copy link
Contributor Author

@bparrishMines bparrishMines Oct 6, 2025

Choose a reason for hiding this comment

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

Hmm I don't know how to reconcile that this value can be any positive integer when it's a mid rolls. Also, postroll are represented differently depending if you are using client side ads or DAI VOD and there isn't a way to get the type in the class.

@bparrishMines bparrishMines added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 7, 2025
@auto-submit auto-submit bot merged commit 1d6c266 into flutter:main Oct 7, 2025
80 checks passed
@bparrishMines bparrishMines deleted the ima_ad_event_ad branch October 7, 2025 18:15
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 8, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 8, 2025
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Oct 8, 2025
flutter/packages@8ca6416...260f381

2025-10-08 jessiewong401@gmail.com Add Versioning to Gradle 9
Deprecation Change (flutter/packages#10187)
2025-10-07 julien@delarbre.biz [camera_avfoundation] iOS: Fix crash when
`enableAudio == false` by correcting guard condition
(flutter/packages#9949)
2025-10-07 49699333+dependabot[bot]@users.noreply.github.com
[dependabot]: Bump androidx.fragment:fragment from 1.8.8 to 1.8.9 in
/packages/local_auth/local_auth_android/android (flutter/packages#10111)
2025-10-07 10687576+bparrishMines@users.noreply.github.com
[interactive_media_ads] Adds support for accessing data for an Ad
(flutter/packages#9972)
2025-10-07 stuartmorgan@google.com [webview_flutter] Add Android
geolocation README (flutter/packages#10166)
2025-10-07 49699333+dependabot[bot]@users.noreply.github.com
[dependabot]: Bump com.google.guava:guava from 33.4.8-android to
33.5.0-android in /packages/espresso/android (flutter/packages#10047)

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-flutter-autoroll
Please CC flutter-ecosystem@google.com on the revert to ensure that a
human
is aware of the problem.

To file a bug in Flutter:
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App p: interactive_media_ads Plugin for IMA SDK platform-android platform-ios

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants