RewardedAds for Android #741

DellaBitta · 2021-11-09T18:14:44Z

Description

Provide details of the change, and generalize the change in the PR title above.

Implemented RewardedAd on Android, and created stubs for iOS and Desktop builds.
Revamped C++ JNI function signatures to have much shorter names, and to reuse functions common to interstitial and rewarded ads.
Created a new internal base class for full screen event listeners to share its implementation across RewardedAds and InsterstitialAds. This allows us to reuse some JNI callback functions.
Grouped those integration tests which require user interaction so that they execute one after the next, and front load them to the start of the itest flow (after a few quick sanity checks). This should make running the manual tests easier.

Testing

iTest Run
Expanded AdMob integration tests.
Executed tests manually on Android phones.
Validated (via logs) that the SetServerSideVerificationOption strings are properly reaching the Android SDK space.

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.

DellaBitta · 2021-11-09T18:17:27Z

admob/integration_test/src/integration_test.cc

@@ -489,6 +506,229 @@ TEST_F(FirebaseAdMobTest, TestRequestConfigurationSetGet) {
                         retrieved_configuration.test_device_ids.end(), "3"));
 }

+// Simple Load Tests as a sanity check. These don't show the ad, just


Moved some BannerView and Interstitial tests around. Will call out the new tests via comments below so that you need not waste your time reviewing unchanged tests.

DellaBitta · 2021-11-09T18:17:36Z

admob/integration_test/src/integration_test.cc

+  delete interstitial;
+}
+
+TEST_F(FirebaseAdMobTest, TestRewardedAdLoad) {


DellaBitta · 2021-11-09T18:17:52Z

admob/integration_test/src/integration_test.cc

+  delete interstitial;
+}
+
+TEST_F(FirebaseAdMobTest, TestRewardedAdLoadAndShow) {


DellaBitta · 2021-11-09T18:18:38Z

admob/integration_test/src/integration_test.cc

@@ -1148,6 +1214,200 @@ TEST_F(FirebaseAdMobTest, TestInterstitialAdErrorBadExtrasClassName) {
  delete interstitial_ad;
 }

+// Other RewardedAd Tests.
+


New tests from here downwards.

DellaBitta · 2021-11-09T20:38:58Z

admob/src/common/interstitial_ad_internal.cc

@@ -53,61 +50,6 @@ InterstitialAdInternal* InterstitialAdInternal::CreateInstance(
        // FIREBASE_PLATFORM_TVOS
 }

-void InterstitialAdInternal::SetFullScreenContentListener(


Moved to a new internal full screen event listener base class which is shared with RewardedAds. This allows us to cast the nativepointer to a listener base class in the JNI callbacks, coalescing the two different JNI callback method sets into one.

DellaBitta · 2021-11-09T20:41:48Z

admob/src/ios/rewarded_ad_internal_ios.h

@@ -0,0 +1,70 @@
+/*


Stub implementation for now.

…ardedad

jonsimantov · 2021-11-15T18:38:57Z

admob/integration_test/src/integration_test.cc

+  TestUserEarnedRewardListener user_earned_reward_listener;
+  WaitForCompletion(rewarded->Show(&user_earned_reward_listener), "Show");
+
+  LogDebug(


I think you get credit for the ad even if you just watch it through, you don't have to install the thing.

While you get the reward the ad doesn't produce the ad clicked event that we're also looking for.

Clicking Install just brings you to the app store. I'll update the comment that you don't actually have to go through with the installation of the app, though.

jonsimantov · 2021-11-15T18:42:20Z

admob/integration_test/src/integration_test.cc

+// Stress tests.  These take a while so run them near the end.
+TEST_F(FirebaseAdMobTest, TestBannerViewStress) {
+  // AdMob cannot be tested on Firebase Test Lab, so disable tests on FTL.
+  TEST_REQUIRES_USER_INTERACTION;


AdMob cannot be tested on Firebase Test Lab, so disable tests on FTL.

What's the deal with this?

Also, even if we can't run this test on FTL, it would be better here to just ensure that you are specifically not running on FTL - the automated tests should still run on simulator, no?

Let's talk to @sunmou99 about seeing if there's a way to add a SKIP_TEST_ON_FIREBASE_TEST_LAB macro.

I've enabled this test successfully in testing another PR, so I'll just remove it here.

I think that this TEST_REQUIRES_USER_INTERACTION was originally for the main BannerView tests that required you to click things but I was confused by the comment that was there, which seems to make it sound like the tests don't run at all. I hadn't tested this until recentlly and it works fine.

jonsimantov · 2021-11-15T18:43:54Z

admob/src/android/admob_android.cc

 }

-extern "C" JNIEXPORT void JNICALL
-Java_com_google_firebase_admob_internal_cpp_InterstitialAdHelper_notifyAdImpressionEvent(  // NOLINT
+extern "C" JNIEXPORT void JNICALL JNI_notifyAdImpressionEvent(JNIEnv* env,


I believe you don't need any of the "extern "C" JNIEXPORT JNICALL stuff since we are registering natives directly - also it would probably be best to put these in an unnamed namespace. (along with the one I added!)

jonsimantov · 2021-11-15T18:48:22Z

admob/src/common/full_screen_ad_event_listener.cc

+void FullScreenAdEventListener::NotifyListenerOfAdClickedFullScreenContent() {
+  MutexLock lock(listener_mutex_);
+  if (full_screen_content_listener_ != nullptr) {
+    full_screen_content_listener_->OnAdClicked();


Just a question: is it safe to keep the mutex locked the whole time the user's callback is running? This could potentially be a long time. Typically if there's any way to avoid locking a mutex while a user callback runs we try not to lock it. Not really sure how you'd safely do this here though, to avoid the listener itself being deleted, so maybe never mind,.

Yes, I see your point.

I feel like tackling it this way: If this mutex lock is a problem then we'll have safe customer reports saying that it's in their way. But if we take the mutex out there, then we could have crazy crash reports due to corrupted memory somewhere later in the runtime. For now I'd like to play it safe and open up to dangers later, but only if we need to. WDYT?

jonsimantov · 2021-11-15T18:49:34Z

admob/src/common/rewarded_ad.cc

+  internal_ = internal::RewardedAdInternal::CreateInstance(this);
+
+  GetOrCreateCleanupNotifier()->RegisterObject(this, [](void* object) {
+    FIREBASE_ASSERT_MESSAGE(


The idea of CleanupNotifier is to invalidate the objects and WARN the user they did the wrong thing, not assert.

Noted. This was from the older implementation. Will fix it in banner and interstitial, too.

jonsimantov · 2021-11-15T18:52:45Z

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

+  private RewardedAd mRewarded;
+
+  // Synchronization object for thread safe access to:
+  // * mRewarded


It doesn't actually seem to be locking when mRewarded is used, is this because mRewarded is only ever used in the UI thread? If so you can mention that

Adding it for other operations...

jonsimantov · 2021-11-16T00:37:41Z

admob/integration_test/src/integration_test.cc

@@ -937,6 +937,9 @@ TEST_F(FirebaseAdMobTest, TestBannerView) {
            bounding_box_listener.bounding_box_changes_.size());

 #if defined(ANDROID) || TARGET_OS_IPHONE


Instead of disabling this test, is it possible to just add some wiggle room to the aspect ratio check on simulator?

github-actions · 2021-11-16T18:18:43Z

❌ Integration test FAILED

Requested by @DellaBitta on commit d852f3c
Last updated: Tue Nov 16 12:51 PST 2021
View integration test log & download artifacts

Failures	Configs
messaging	[TEST] [ERROR] [Android] [All os] [android_target]
storage	[TEST] [FAILURE] [iOS] [macos] [simulator_target] (1 failed tests) FirebaseStorageTest.TestLargeFilePauseResumeAndDownloadCancel

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

Android builds and runs. Desktop and iOS stubs.

1fc8428

google-cla bot added the cla: yes label Nov 9, 2021

DellaBitta mentioned this pull request Nov 9, 2021

Feature/admob 2021 admob rewardedad #740

Closed

DellaBitta commented Nov 9, 2021

View reviewed changes

revamp Android C++ JNI method signatures

4a5dabc

DellaBitta changed the title ~~Android builds and runs. Desktop and iOS stubs.~~ RewardedAds for Android Nov 9, 2021

DellaBitta commented Nov 9, 2021

View reviewed changes

admob/src/ios/rewarded_ad_internal_ios.h

@@ -0,0 +1,70 @@

/*

Copy link

Contributor Author

DellaBitta Nov 9, 2021

Choose a reason for hiding this comment

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

Stub implementation for now.

DellaBitta added 2 commits November 9, 2021 15:50

added admob/src/common/full_screen_ad_event_listener.cc/h

0a3c7d1

added ServerSideVerificationOptions

122a772

DellaBitta marked this pull request as ready for review November 10, 2021 16:50

DellaBitta requested a review from jonsimantov November 10, 2021 16:50

DellaBitta and others added 3 commits November 11, 2021 09:11

lint fix

0485c81

Merge branch 'feature/admob_2021' into feature/admob_2021_android_rew…

79ef124

…ardedad

lint fix - header order

32c1b62

jonsimantov approved these changes Nov 15, 2021

View reviewed changes

review fixes

10a3a4d

DellaBitta requested a review from jonsimantov November 15, 2021 21:54

jonsimantov approved these changes Nov 15, 2021

View reviewed changes

bounding box checks fail on simulators due to aspect ratios

f38cddb

jonsimantov reviewed Nov 16, 2021

View reviewed changes

DellaBitta added 3 commits November 16, 2021 08:43

simulator itests

5a9ecfd

bounding box changes on sims

5a046c3

stress tests

b9069da

DellaBitta merged commit d852f3c into feature/admob_2021 Nov 16, 2021

DellaBitta deleted the feature/admob_2021_android_rewardedad branch November 16, 2021 18:18

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

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

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

firebase locked and limited conversation to collaborators Dec 17, 2021

		@@ -937,6 +937,9 @@ TEST_F(FirebaseAdMobTest, TestBannerView) {
		bounding_box_listener.bounding_box_changes_.size());

		#if defined(ANDROID) \|\| TARGET_OS_IPHONE

RewardedAds for Android #741

RewardedAds for Android #741

Uh oh!

Conversation

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

Uh oh!

Description

Testing

Type of Change

Uh oh!

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

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!

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

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!

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

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!

Choose a reason for hiding this comment

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 16, 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 Nov 9, 2021 •

edited

Loading

DellaBitta Nov 9, 2021 •

edited

Loading

DellaBitta Nov 9, 2021 •

edited

Loading

DellaBitta Nov 15, 2021 •

edited

Loading

DellaBitta Nov 15, 2021 •

edited

Loading

github-actions bot commented Nov 16, 2021 •

edited

Loading