Skip to content

Remove LoadAdResult from new Admob. #723

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 69 commits into from
Nov 8, 2021

Conversation

DellaBitta
Copy link
Contributor

@DellaBitta DellaBitta commented Nov 2, 2021

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.


Testing

Local testing on iOS and Android phones.
iTestRun


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.

Notes

N/A.

@google-cla google-cla bot added the cla: yes label Nov 2, 2021
@DellaBitta DellaBitta changed the title Remove Admob LoadAdResult from new Impl. Remove LoadAdResult from new Admob. Nov 2, 2021
@@ -186,6 +190,10 @@ class AdResult {
/// Gets the message describing the error.
const std::string& message() const;

/// Gets the ResponseInfo if an loadAd error occurred, with a collection of
Copy link
Contributor

Choose a reason for hiding this comment

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

Will there always be a responseinfo? What if it's the situation where it wasn't, will it just be empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the call to this method will return an emtpy ResponseInfo object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the comments.

Base automatically changed from feature/admob_2021_adview_listeners to feature/admob_2021 November 8, 2021 20:58
@DellaBitta DellaBitta merged commit efda224 into feature/admob_2021 Nov 8, 2021
@DellaBitta DellaBitta deleted the feature/admob_2021_remove_loadadresult branch November 8, 2021 23:02
@github-actions github-actions bot added the tests: in-progress This PR's integration tests are in progress. label Nov 8, 2021
@github-actions
Copy link

github-actions bot commented Nov 8, 2021

❌  Integration test FAILED

Requested by @DellaBitta on commit efda224
Last updated: Mon Nov 8 17:35 PST 2021
View integration test log & download artifacts

Failures Configs
messaging [TEST] [ERROR] [Android] [All os] [android_target]

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

@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Nov 9, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Nov 9, 2021
@firebase firebase locked and limited conversation to collaborators Dec 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes tests: failed This PR's integration tests failed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants