Feature/admob 2021 adview, fullscreen listeners #713

DellaBitta · 2021-10-29T00:34:46Z

Description

- iOS & Android dependencies increased to latest AdMob version to support AdClicked events.
- Implemented AdListener, FullScreenContent, and PaidEvent listeners. These listener signatures track closely with the iOS and Android counterparts so that they may easily be expanded and supported.
- Removed PresentationStateListeners. Similar functionality is now facilaited by AdListeners and FullScreenContentListeners.
- Methods for ad placement renamed from MoveTo to SetPosition to align with AdMob's iOS and Android SDKs.
- Invoking methods on BannerView and InterstitialAds now properly fail on "uninitialized" objects. Previously empty / invalid futures were returned from most methods.
- Fixed an hang on loadAd calls for uninitialized BannerViews and InterstialAds.
- More thorough shut down / destruction of ad objects ensure consistency and reduce potential flake.
- Integration test updates:
- New manual tests track Bounding Box, AdView, FullScreenPresentation and Paid events when clicking through ads.
- Added higher-fidelity checks of bounding box state. Bounding box change behaviors across Android and iOS are now consistent.
- Added a "stress" test of loading and releasing 10 BannerView ads and 10 Interstitial Ads in tight succession.
- Destroy() is now called to properly clean up BannerViews.
- Expanded coverage of error scenarios: uninitialized ads, duplicate load requests, etc.

Testing

Updated iTest sources.
Manually tested on iOS and Android.
iTest CI Run

Type of Change

Place an x the applicable box:

Bug fix. Add the issue # below if applicable.
New feature. A non-breaking change which adds functionality.
Other, such as a build process or documentation change.

…21_adresult

admob/src/ios/FADRequest.mm

admob/src/ios/admob_ios.mm

DellaBitta · 2021-11-01T12:14:30Z

admob/src/android/banner_view_internal_android.cc

    CompleteFuture(kAdMobErrorAlreadyInitialized,
                   kAdAlreadyInitializedErrorMessage, future_handle,
                   &future_data_);
-  } else {


remove the else clause so this looks like a major deletion. It's not, it appears again below.

DellaBitta · 2021-11-01T12:16:31Z

admob/src/common/banner_view.cc

 Future<LoadAdResult> BannerView::LoadAd(const AdRequest& request) {
-  if (!CheckIsInitialized(internal_)) return Future<LoadAdResult>();


All of these methods were returning futures that hadn't been completed..!

DellaBitta · 2021-11-01T12:27:07Z

admob/src_java/com/google/firebase/admob/internal/cpp/BannerViewHelper.java

@@ -143,42 +139,39 @@ public void initialize(Activity activity) {
  public void destroy(final long callbackDataPtr) {
    // If the Activity isn't initialized, there is nothing to destroy.
    if (mActivity == null) {
+      completeBannerViewFutureCallback(callbackDataPtr,
+        ConstantsHelper.CALLBACK_ERROR_NONE,
+        ConstantsHelper.CALLBACK_ERROR_MESSAGE_NONE);
      return;


This fixes a bug when calling destroy on an uninitialized BannerView. Previously it would hang as the future was never completed.

…gad_error string

Updated dependencies to support new AdClicked callback interface.

jonsimantov

Approved with a few notes.

jonsimantov · 2021-11-08T18:05:55Z

admob/src_java/com/google/firebase/admob/internal/cpp/ConstantsHelper.java

-  // LINT.ThenChange(//depot_firebase_cpp/admob/client/cpp/src/include/firebase/admob/banner_view.h,
-  // //depot_firebase_cpp/admob/client/cpp/src/include/firebase/admob/native_express_ad_view.h)
-
+  // LINT.ThenChange(//depot_firebase_cpp/admob/client/cpp/src/include/firebase/admob/banner_view.h)


I don't think we still have IfThisThenThat, so it might be cleaner to get rid of these. Unless there is a GitHub check equivalent - that would be awesome.

Ok. Yeah, I'm actually removing some of the stuff in here in the next PR, so I'll remove those, too. I didn't know this was related to IFTTT!

jonsimantov · 2021-11-08T18:10:01Z

admob/src/android/admob_android.cc


+extern "C" JNIEXPORT void JNICALL
+Java_com_google_firebase_admob_internal_cpp_BannerViewHelper_notifyAdClosed(


Same as my comment on a previous PR, consider shortening these names since they are passed into RegisterNatives anyway. (And you can put them in an anonymous namespace as well.)

Yep, it'll be part of the RewardedAd PR for Android.

jonsimantov · 2021-11-08T18:11:49Z

admob/src/android/banner_view_internal_android.cc

@@ -53,11 +53,62 @@ METHOD_LOOKUP_DEFINITION(ad_view, "com/google/android/gms/ads/AdView",

 namespace internal {

+// Contains data to invoke Initailize from the main thread.
+struct InitializeOnMainThreadData {


👍 keep Java helper code as small as possible

jonsimantov · 2021-11-08T18:15:23Z

admob/src/common/admob_common.cc

+// Default no-op implementation for each listener method so that applications
+// need only implement the methods they're interested in.
+AdListener::~AdListener() {}
+void AdListener::OnAdClicked() {}


Aside from the destructor (which needs to be here), WDYT about inlining the other implementations so it's clear to developers writing subclasses that they don't need to call the base class methods?

Sure! I'll put this in the next change as well, to reduce merge conflicts.

github-actions · 2021-11-08T20:59:20Z

❌ Integration test FAILED

Requested by @DellaBitta on commit bd8110e
Last updated: Mon Nov 8 14:58 PST 2021
View integration test log & download artifacts

Failures	Configs
messaging	[TEST] [ERROR] [Android] [All os] [android_target]
storage	[TEST] [ERROR] [iOS] [macos] [ios_target]

Add flaky tests to go/fpl-cpp-flake-tracker

DellaBitta added 30 commits September 27, 2021 10:54

Android AdResult prototype

db93aaa

linter fixes

8116de4

cause becomes smart ptr. iOS and Desktop impls

0eb306e

linter fixes

15c7b55

Added AdapterResponseInfo & AdResponseInfo | AdResult fixes

41c9aed

linter fixes

903abd8

load_ad_result | response info iOS

9fd06d2

draft LoadAdResult future on Android

42643cd

successful load ad, need to trim debug info

0f9125f

removed debug logs

ee2a38d

Safe Futures

693435e

stubs build, reworked futures to use safe API

f740e43

merged feature/admob_2021 with RequestConfig changes

23bf809

Banner View Android Tests

89db0b3

Android, Desktop Interstitial tests

a8543d9

CreateAndCompleteFuture, enabled TestBannerViewMultithreadDeletion

eb97f88

ios and Android LoadAdResult rework

8d2f2b9

linter fixes

4476e68

added more comments

fa2e305

no lint on JNI function signatures

32865b9

skip loading in progress tests on desktop

4062d9a

multithread test

d8e2e58

Upgrade to GMA 8.12 for iOS

19a50b2

Merge branch 'feature/admob_ios_version_update' into feature/admob_20…

663adee

…21_adresult

lint fix

c89f0eb

add inherited linker flags to xcodeproject

531d924

exclude i386 in Admob itest project

4e1f165

review fixes

5519a0f

Android bannerview listeners

a269f07

most bannerview operations use c++ runonmainthread

33bd11a

DellaBitta commented Oct 31, 2021

View reviewed changes

admob/src/ios/FADRequest.mm Show resolved Hide resolved

DellaBitta commented Oct 31, 2021

View reviewed changes

admob/src/ios/admob_ios.mm Show resolved Hide resolved

DellaBitta added 2 commits October 31, 2021 14:46

comment / formatting pass

d6e1050

interstitial uninitialized checks

4e5c7b9

DellaBitta commented Nov 1, 2021

View reviewed changes

DellaBitta added 9 commits November 1, 2021 08:34

fixed missing itest line.

633d167

guard bannerview getlastresult use from returning a faulty loadAdResult

53ba827

AdResult::kUndefinedDomain now const char* const

0afef99

Format .java files

352a790

Removed LoadAdResultInternal. Now simply shares AdResultInternal struct

cd95460

load_ad_result_stub save error

e87ae0a

remove accidental change to reference_counted_future_impl.h

db7c046

use of stringWithFormat in adapter_response. CompleteFuture now uses …

051d3e8

…gad_error string

merge feature/admob_2021_adresult

8cf3960

Base automatically changed from feature/admob_2021_adresult to feature/admob_2021 November 1, 2021 17:56

DellaBitta and others added 5 commits November 1, 2021 15:54

merged feature/admob_2021

16837e0

create future replacement

5a9e5d7

banner view wait for listeners in itest

74c6594

Ad Clicked fix, Android link error. (#735)

09d8c7c

Updated dependencies to support new AdClicked callback interface.

removed keyvalue pair from types.h

a6829e4

jonsimantov approved these changes Nov 8, 2021

View reviewed changes

DellaBitta merged commit bd8110e into feature/admob_2021 Nov 8, 2021

DellaBitta deleted the feature/admob_2021_adview_listeners branch November 8, 2021 20:58

github-actions bot added the tests: in-progress This PR's integration tests are in progress. label Nov 8, 2021

github-actions bot added the tests: failed This PR's integration tests failed. label Nov 8, 2021

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Nov 8, 2021

firebase locked and limited conversation to collaborators Dec 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feature/admob 2021 adview, fullscreen listeners #713

Feature/admob 2021 adview, fullscreen listeners #713

Uh oh!

DellaBitta commented Oct 29, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

DellaBitta Nov 1, 2021

Uh oh!

DellaBitta Nov 1, 2021

Uh oh!

DellaBitta Nov 1, 2021

Uh oh!

jonsimantov left a comment

Uh oh!

jonsimantov Nov 8, 2021

Uh oh!

DellaBitta Nov 8, 2021

Uh oh!

jonsimantov Nov 8, 2021

Uh oh!

DellaBitta Nov 8, 2021

Uh oh!

jonsimantov Nov 8, 2021

Uh oh!

jonsimantov Nov 8, 2021

Uh oh!

DellaBitta Nov 8, 2021

Uh oh!

github-actions bot commented Nov 8, 2021 •

edited

Loading

Uh oh!

Uh oh!

		Future<LoadAdResult> BannerView::LoadAd(const AdRequest& request) {
		if (!CheckIsInitialized(internal_)) return Future<LoadAdResult>();


		extern "C" JNIEXPORT void JNICALL
		Java_com_google_firebase_admob_internal_cpp_BannerViewHelper_notifyAdClosed(

Feature/admob 2021 adview, fullscreen listeners #713

Feature/admob 2021 adview, fullscreen listeners #713

Uh oh!

Conversation

DellaBitta commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Type of Change

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonsimantov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Integration test FAILED

Uh oh!

Uh oh!

DellaBitta commented Oct 29, 2021 •

edited

Loading

github-actions bot commented Nov 8, 2021 •

edited

Loading