Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign up[WIP] Ad notifications fail to open if Brave is closed after the Ad notification is shown #2837
Conversation
tmancey
force-pushed the
issues/4102
branch
24 times, most recently
from
a486aa4
to
4fcc57b
tmancey
force-pushed the
issues/4102
branch
14 times, most recently
from
e745441
to
e43cee8
|
Just a few fixes needed for iOS |
| @@ -43,5 +42,6 @@ class NativeAdsClient : public ads::AdsClient { | |||
| void SetIdleThreshold(const int threshold) override; | |||
| uint32_t SetTimer(const uint64_t time_offset) override; | |||
| void ShowNotification(std::unique_ptr<ads::NotificationInfo> info) override; | |||
| void CloseNotification(const std::string& id) override; | |||
This comment has been minimized.
This comment has been minimized.
kylehickinson
Jul 3, 2019
Collaborator
Needs an accompanying implementation in vendor/brave-ios/Ads/Generated/NativeAdsClient.mm which calls the Obj-C bridge method you added in vendor/brave-ios/Ads/Generated/NativeAdsClientBridge.h
| { | ||
| return [self.commonOps generateUUID]; | ||
| } | ||
|
|
This comment has been minimized.
This comment has been minimized.
kylehickinson
Jul 3, 2019
Collaborator
Missing method added in vendor/brave-ios/Ads/Generated/NativeAdsClientBridge.h:
- (void)closeNotification:(const std::string &)id;
You can implement this as an empty method here but please just leave a reference to what its for/this PR
| ~NotificationInfo(); | ||
|
|
||
| const std::string ToJson() const; | ||
| Result FromJson( | ||
| const std::string& json, | ||
| std::string* error_description = nullptr); | ||
|
|
||
| std::string id; |
This comment has been minimized.
This comment has been minimized.
kylehickinson
Jul 3, 2019
Collaborator
Added a new field here but iOS has BATAdsNotification which needs the same field:
tmancey
force-pushed the
issues/4102
branch
2 times, most recently
from
112823d
to
0209f15
tmancey
force-pushed the
issues/4102
branch
3 times, most recently
from
04702db
to
23d6164
…ification is shown
|
Closing as superseded by #2876 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
tmancey commentedJul 1, 2019
Submitter Checklist:
npm run lint)git rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.