Skip to content

Commit efda224

Browse files
authored
Remove LoadAdResult from new Admob. (#723)
### Description Remove LoadAdResult and fold it's data into AdResult. The ResponseInfo member of LoadAdResult now resides in AdResult and will only be populated in Results that stem from `loadAd` requests. **Note:** With this new class / data member topography, there isn't an order in which I could declare AdResult in types.h without the compiler complaining that it needed a definition of its ResponseInfo member. I solved this by making the member a _pointer_ to a ResponseInfo and this made the compiler very happy. We're friends now.
1 parent bd8110e commit efda224

36 files changed

+273
-370
lines changed

admob/CMakeLists.txt

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ set(android_SRCS
3939
src/android/admob_android.cc
4040
src/android/banner_view_internal_android.cc
4141
src/android/interstitial_ad_internal_android.cc
42-
src/android/load_ad_result_android.cc
4342
src/android/response_info_android.cc)
4443

4544
# Source files used by the iOS implementation.
@@ -53,15 +52,13 @@ set(ios_SRCS
5352
src/ios/admob_ios.mm
5453
src/ios/banner_view_internal_ios.mm
5554
src/ios/interstitial_ad_internal_ios.mm
56-
src/ios/load_ad_result_ios.mm
5755
src/ios/response_info_ios.mm)
5856

5957
# Source files used by the stub implementation.
6058
set(stub_SRCS
6159
src/stub/ad_result_stub.cc
6260
src/stub/adapter_response_info_stub.cc
6361
src/stub/admob_stub.cc
64-
src/stub/load_ad_result_stub.cc
6562
src/stub/response_info_stub.cc)
6663

6764
if(ANDROID)

admob/integration_test/src/integration_test.cc

Lines changed: 13 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -509,12 +509,12 @@ TEST_F(FirebaseAdMobTest, TestBannerView) {
509509

510510
// Load the banner ad.
511511
firebase::admob::AdRequest request = GetAdRequest();
512-
firebase::Future<firebase::admob::LoadAdResult> load_ad_future =
512+
firebase::Future<firebase::admob::AdResult> load_ad_future =
513513
banner->LoadAd(request);
514514
WaitForCompletion(load_ad_future, "LoadAd");
515515
EXPECT_EQ(expected_num_bounding_box_changes,
516516
bounding_box_listener.bounding_box_changes_.size());
517-
const firebase::admob::LoadAdResult* result_ptr = load_ad_future.result();
517+
const firebase::admob::AdResult* result_ptr = load_ad_future.result();
518518
ASSERT_NE(result_ptr, nullptr);
519519
EXPECT_TRUE(result_ptr->is_successful());
520520
EXPECT_EQ(result_ptr->code(), firebase::admob::kAdMobErrorNone);
@@ -636,9 +636,6 @@ TEST_F(FirebaseAdMobTest, TestBannerView) {
636636
app_framework::ProcessEvents(2000);
637637

638638
WaitForCompletion(banner->Destroy(), "Destroy BannerView");
639-
640-
LogDebug("And again to ensure destruction callbacks are recorded.");
641-
app_framework::ProcessEvents(2000);
642639
banner->SetBoundingBoxListener(nullptr);
643640
delete banner;
644641

@@ -718,7 +715,7 @@ TEST_F(FirebaseAdMobTest, TestBannerViewAdOpenedAdClosed) {
718715

719716
// Load the banner ad.
720717
firebase::admob::AdRequest request = GetAdRequest();
721-
firebase::Future<firebase::admob::LoadAdResult> load_ad_future =
718+
firebase::Future<firebase::admob::AdResult> load_ad_future =
722719
banner->LoadAd(request);
723720
WaitForCompletion(load_ad_future, "LoadAd");
724721
WaitForCompletion(banner->Show(), "Show 0");
@@ -737,9 +734,6 @@ TEST_F(FirebaseAdMobTest, TestBannerViewAdOpenedAdClosed) {
737734
app_framework::ProcessEvents(1000);
738735
}
739736

740-
LogDebug("Waiting for a moment to ensure all callbacks are recorded.");
741-
app_framework::ProcessEvents(2000);
742-
743737
// Ensure all of the expected events were triggered on Android.
744738
EXPECT_EQ(ad_listener.num_on_ad_clicked_, 1);
745739
EXPECT_EQ(ad_listener.num_on_ad_impression_, 1);
@@ -853,16 +847,16 @@ TEST_F(FirebaseAdMobTest, TestBannerViewErrorLoadInProgress) {
853847
// successful ad loads instead of the expected
854848
// kAdMobErrorLoadInProgress error.
855849
firebase::admob::AdRequest request = GetAdRequest();
856-
firebase::Future<firebase::admob::LoadAdResult> first_load_ad =
850+
firebase::Future<firebase::admob::AdResult> first_load_ad =
857851
banner->LoadAd(request);
858-
firebase::Future<firebase::admob::LoadAdResult> second_load_ad =
852+
firebase::Future<firebase::admob::AdResult> second_load_ad =
859853
banner->LoadAd(request);
860854

861855
WaitForCompletion(second_load_ad, "Second LoadAd",
862856
firebase::admob::kAdMobErrorLoadInProgress);
863857
WaitForCompletion(first_load_ad, "First LoadAd");
864858

865-
const firebase::admob::LoadAdResult* result_ptr = second_load_ad.result();
859+
const firebase::admob::AdResult* result_ptr = second_load_ad.result();
866860
ASSERT_NE(result_ptr, nullptr);
867861
EXPECT_FALSE(result_ptr->is_successful());
868862
EXPECT_EQ(result_ptr->code(), firebase::admob::kAdMobErrorLoadInProgress);
@@ -890,12 +884,11 @@ TEST_F(FirebaseAdMobTest, TestBannerViewErrorBadAdUnitId) {
890884

891885
// Load the banner ad.
892886
firebase::admob::AdRequest request = GetAdRequest();
893-
firebase::Future<firebase::admob::LoadAdResult> load_ad =
894-
banner->LoadAd(request);
887+
firebase::Future<firebase::admob::AdResult> load_ad = banner->LoadAd(request);
895888
WaitForCompletion(load_ad, "LoadAd",
896889
firebase::admob::kAdMobErrorInvalidRequest);
897890

898-
const firebase::admob::LoadAdResult* result_ptr = load_ad.result();
891+
const firebase::admob::AdResult* result_ptr = load_ad.result();
899892
ASSERT_NE(result_ptr, nullptr);
900893
EXPECT_FALSE(result_ptr->is_successful());
901894
EXPECT_EQ(result_ptr->code(), firebase::admob::kAdMobErrorInvalidRequest);
@@ -1088,16 +1081,16 @@ TEST_F(FirebaseAdMobTest, TestInterstitialAdErrorLoadInProgress) {
10881081
// successful ad loads instead of the expected
10891082
// kAdMobErrorLoadInProgress error.
10901083
firebase::admob::AdRequest request = GetAdRequest();
1091-
firebase::Future<firebase::admob::LoadAdResult> first_load_ad =
1084+
firebase::Future<firebase::admob::AdResult> first_load_ad =
10921085
interstitial_ad->LoadAd(kInterstitialAdUnit, request);
1093-
firebase::Future<firebase::admob::LoadAdResult> second_load_ad =
1086+
firebase::Future<firebase::admob::AdResult> second_load_ad =
10941087
interstitial_ad->LoadAd(kInterstitialAdUnit, request);
10951088

10961089
WaitForCompletion(second_load_ad, "Second LoadAd",
10971090
firebase::admob::kAdMobErrorLoadInProgress);
10981091
WaitForCompletion(first_load_ad, "First LoadAd");
10991092

1100-
const firebase::admob::LoadAdResult* result_ptr = second_load_ad.result();
1093+
const firebase::admob::AdResult* result_ptr = second_load_ad.result();
11011094
ASSERT_NE(result_ptr, nullptr);
11021095
EXPECT_FALSE(result_ptr->is_successful());
11031096
EXPECT_EQ(result_ptr->code(), firebase::admob::kAdMobErrorLoadInProgress);
@@ -1120,12 +1113,12 @@ TEST_F(FirebaseAdMobTest, TestInterstitialAdErrorBadAdUnitId) {
11201113

11211114
// Load the interstitial ad.
11221115
firebase::admob::AdRequest request = GetAdRequest();
1123-
firebase::Future<firebase::admob::LoadAdResult> load_ad =
1116+
firebase::Future<firebase::admob::AdResult> load_ad =
11241117
interstitial_ad->LoadAd(kBadAdUnit, request);
11251118
WaitForCompletion(load_ad, "LoadAd",
11261119
firebase::admob::kAdMobErrorInvalidRequest);
11271120

1128-
const firebase::admob::LoadAdResult* result_ptr = load_ad.result();
1121+
const firebase::admob::AdResult* result_ptr = load_ad.result();
11291122
ASSERT_NE(result_ptr, nullptr);
11301123
EXPECT_FALSE(result_ptr->is_successful());
11311124
EXPECT_EQ(result_ptr->code(), firebase::admob::kAdMobErrorInvalidRequest);

admob/src/android/ad_result_android.cc

Lines changed: 68 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323

2424
#include "admob/src/android/ad_request_converter.h"
2525
#include "admob/src/android/admob_android.h"
26+
#include "admob/src/android/response_info_android.h"
2627
#include "admob/src/common/admob_common.h"
2728
#include "admob/src/include/firebase/admob.h"
2829

@@ -34,6 +35,11 @@ METHOD_LOOKUP_DEFINITION(ad_error,
3435
"com/google/android/gms/ads/AdError",
3536
ADERROR_METHODS);
3637

38+
METHOD_LOOKUP_DEFINITION(load_ad_error,
39+
PROGUARD_KEEP_CLASS
40+
"com/google/android/gms/ads/LoadAdError",
41+
LOADADERROR_METHODS);
42+
3743
const char* const AdResult::kUndefinedDomain = "undefined";
3844

3945
AdResult::AdResult() {
@@ -43,11 +49,17 @@ AdResult::AdResult() {
4349
internal_ = new AdResultInternal();
4450
internal_->is_successful = false;
4551
internal_->is_wrapper_error = true;
52+
internal_->is_load_ad_error = false;
4653
internal_->code = kAdMobErrorUninitialized;
4754
internal_->domain = "SDK";
4855
internal_->message = "This AdResult has not be initialized.";
4956
internal_->to_string = internal_->message;
5057
internal_->j_ad_error = nullptr;
58+
59+
// While most data is passed into this object through the AdResultInternal
60+
// structure (above), the response_info_ is constructed when parsing
61+
// the j_ad_error itself.
62+
response_info_ = new ResponseInfo();
5163
}
5264

5365
AdResult::AdResult(const AdResultInternal& ad_result_internal) {
@@ -57,7 +69,9 @@ AdResult::AdResult(const AdResultInternal& ad_result_internal) {
5769
internal_ = new AdResultInternal();
5870
internal_->is_successful = ad_result_internal.is_successful;
5971
internal_->is_wrapper_error = ad_result_internal.is_wrapper_error;
72+
internal_->is_load_ad_error = ad_result_internal.is_load_ad_error;
6073
internal_->j_ad_error = nullptr;
74+
response_info_ = new ResponseInfo();
6175

6276
// AdResults can be returned on success, or for errors encountered in the C++
6377
// SDK wrapper, or in the Android AdMob SDK. The structure is populated
@@ -104,17 +118,44 @@ AdResult::AdResult(const AdResultInternal& ad_result_internal) {
104118
internal_->message = util::JStringToString(env, j_message);
105119
env->DeleteLocalRef(j_message);
106120

107-
// To string.
108-
jobject j_to_string = env->CallObjectMethod(
109-
internal_->j_ad_error, ad_error::GetMethodId(ad_error::kToString));
110-
FIREBASE_ASSERT(j_to_string);
111-
internal_->to_string = util::JStringToString(env, j_to_string);
112-
env->DeleteLocalRef(j_to_string);
121+
// Differentiate between a com.google.android.gms.ads.AdError or its
122+
// com.google.android.gms.ads.LoadAdError subclass.
123+
if (!internal_->is_load_ad_error) {
124+
// AdError.
125+
jobject j_to_string = env->CallObjectMethod(
126+
internal_->j_ad_error, ad_error::GetMethodId(ad_error::kToString));
127+
FIREBASE_ASSERT(j_to_string);
128+
internal_->to_string = util::JStringToString(env, j_to_string);
129+
env->DeleteLocalRef(j_to_string);
130+
} else {
131+
// LoadAdError.
132+
jobject j_response_info = env->CallObjectMethod(
133+
internal_->j_ad_error,
134+
load_ad_error::GetMethodId(load_ad_error::kGetResponseInfo));
135+
136+
if (j_response_info != nullptr) {
137+
ResponseInfoInternal response_info_internal;
138+
response_info_internal.j_response_info = j_response_info;
139+
*response_info_ = ResponseInfo(response_info_internal);
140+
env->DeleteLocalRef(j_response_info);
141+
}
142+
143+
// A to_string value of this LoadAdError. Invoke the set_to_string
144+
// protected method of the AdResult parent class to overwrite whatever
145+
// it parsed.
146+
jobject j_to_string = env->CallObjectMethod(
147+
internal_->j_ad_error,
148+
load_ad_error::GetMethodId(load_ad_error::kToString));
149+
internal_->to_string = util::JStringToString(env, j_to_string);
150+
env->DeleteLocalRef(j_to_string);
151+
}
113152
}
114153
}
115154

116155
AdResult::AdResult(const AdResult& ad_result) : AdResult() {
156+
FIREBASE_ASSERT(ad_result.response_info_ != nullptr);
117157
// Reuse the assignment operator.
158+
this->response_info_ = new ResponseInfo();
118159
*this = ad_result;
119160
}
120161

@@ -127,46 +168,57 @@ AdResult& AdResult::operator=(const AdResult& ad_result) {
127168
FIREBASE_ASSERT(env);
128169
FIREBASE_ASSERT(internal_);
129170
FIREBASE_ASSERT(ad_result.internal_);
171+
FIREBASE_ASSERT(response_info_);
172+
FIREBASE_ASSERT(ad_result.response_info_);
130173

131174
AdResultInternal* preexisting_internal = internal_;
132175
{
133176
// Lock the parties so they're not deleted while the copying takes place.
134-
MutexLock(ad_result.internal_->mutex);
135-
MutexLock(internal_->mutex);
177+
MutexLock ad_result_lock(ad_result.internal_->mutex);
178+
MutexLock lock(internal_->mutex);
136179
internal_ = new AdResultInternal();
137180

138181
internal_->is_successful = ad_result.internal_->is_successful;
139182
internal_->is_wrapper_error = ad_result.internal_->is_wrapper_error;
183+
internal_->is_load_ad_error = ad_result.internal_->is_load_ad_error;
140184
internal_->code = ad_result.internal_->code;
141185
internal_->domain = ad_result.internal_->domain;
142186
internal_->message = ad_result.internal_->message;
143187
if (ad_result.internal_->j_ad_error != nullptr) {
144188
internal_->j_ad_error =
145189
env->NewGlobalRef(ad_result.internal_->j_ad_error);
146190
}
147-
148191
if (preexisting_internal->j_ad_error) {
149192
env->DeleteGlobalRef(preexisting_internal->j_ad_error);
150193
preexisting_internal->j_ad_error = nullptr;
151194
}
195+
196+
*response_info_ = *ad_result.response_info_;
152197
}
153198

154199
// Deleting the internal_ deletes the mutex within it, so we wait for
155200
// complete deletion until after the mutex lock leaves scope.
156201
delete preexisting_internal;
202+
157203
return *this;
158204
}
159205

160206
AdResult::~AdResult() {
161207
FIREBASE_ASSERT(internal_);
208+
FIREBASE_ASSERT(response_info_);
209+
162210
if (internal_->j_ad_error != nullptr) {
163211
JNIEnv* env = GetJNI();
164212
FIREBASE_ASSERT(env);
165213
env->DeleteGlobalRef(internal_->j_ad_error);
166214
internal_->j_ad_error = nullptr;
167215
}
216+
168217
delete internal_;
169218
internal_ = nullptr;
219+
220+
delete response_info_;
221+
response_info_ = nullptr;
170222
}
171223

172224
bool AdResult::is_successful() const {
@@ -193,7 +245,9 @@ std::unique_ptr<AdResult> AdResult::GetCause() const {
193245

194246
AdResultInternal ad_result_internal;
195247
ad_result_internal.is_wrapper_error = false;
248+
ad_result_internal.is_load_ad_error = false;
196249
ad_result_internal.j_ad_error = j_ad_error;
250+
197251
std::unique_ptr<AdResult> ad_result =
198252
std::unique_ptr<AdResult>(new AdResult(ad_result_internal));
199253
env->DeleteLocalRef(j_ad_error);
@@ -219,17 +273,16 @@ const std::string& AdResult::message() const {
219273
return internal_->message;
220274
}
221275

276+
const ResponseInfo& AdResult::response_info() const {
277+
FIREBASE_ASSERT(response_info_ != nullptr);
278+
return *response_info_;
279+
}
280+
222281
/// Returns a log friendly string version of this object.
223282
const std::string& AdResult::ToString() const {
224283
FIREBASE_ASSERT(internal_);
225284
return internal_->to_string;
226285
}
227286

228-
/// Protected method used by LoadAdResult, a subclass which constructs its
229-
/// to_string representation programatically after the AdResult construction.
230-
void AdResult::set_to_string(std::string to_string) {
231-
FIREBASE_ASSERT(internal_);
232-
internal_->to_string = to_string;
233-
}
234287
} // namespace admob
235288
} // namespace firebase

admob/src/android/ad_result_android.h

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,14 @@ namespace admob {
3838
X(ToString, "toString", "()Ljava/lang/String;")
3939
// clang-format on
4040

41+
#define LOADADERROR_METHODS(X) \
42+
X(GetResponseInfo, "getResponseInfo", \
43+
"()Lcom/google/android/gms/ads/ResponseInfo;"), \
44+
X(ToString, "toString", "()Ljava/lang/String;")
45+
// clang-format on
46+
4147
METHOD_LOOKUP_DECLARATION(ad_error, ADERROR_METHODS);
48+
METHOD_LOOKUP_DECLARATION(load_ad_error, LOADADERROR_METHODS);
4249

4350
struct AdResultInternal {
4451
// True if the result contains an error originating from C++/Java wrapper
@@ -48,13 +55,16 @@ struct AdResultInternal {
4855
// True if this was a successful result.
4956
bool is_successful;
5057

51-
// An error code
58+
// True if this error data represents a result from a LoadAd request.
59+
bool is_load_ad_error;
60+
61+
// An error code.
5262
AdMobError code;
5363

54-
// A cached value of com.google.android.gms.ads.AdError.domain
64+
// A cached value of com.google.android.gms.ads.AdError.domain.
5565
std::string domain;
5666

57-
// A cached value of com.google.android.gms.ads.AdError.message
67+
// A cached value of com.google.android.gms.ads.AdError.message.
5868
std::string message;
5969

6070
// A cached result from invoking com.google.android.gms.ads.AdError.ToString.

admob/src/android/adapter_response_info_android.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ AdapterResponseInfo::AdapterResponseInfo(
4444

4545
// Construct an AdResultInternal from the AdError in the AdapterResponseInfo.
4646
AdResultInternal ad_result_internal;
47+
ad_result_internal.is_load_ad_error = false;
4748
ad_result_internal.j_ad_error = env->CallObjectMethod(
4849
j_adapter_response_info,
4950
adapter_response_info::GetMethodId(adapter_response_info::kGetAdError));

0 commit comments

Comments
 (0)