Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Android] Ad notification shows up even when Brave-Beta is not running #7917 #4805

Merged
merged 1 commit into from
Mar 6, 2020
Merged

[Android] Ad notification shows up even when Brave-Beta is not running #7917 #4805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 79 additions & 2 deletions components/brave_ads/browser/ads_notification_handler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@
#include "brave/components/brave_ads/browser/ads_service_impl.h"
#include "content/public/browser/browser_context.h"

#if defined(OS_ANDROID)
#include "base/android/application_status_listener.h"
#include "chrome/browser/lifetime/application_lifetime_android.h"
#endif

namespace brave_ads {

int kUserDataKey; // The value is not important, the address is a key.
Expand All @@ -30,13 +35,18 @@ AdsNotificationHandler::~AdsNotificationHandler() {
void AdsNotificationHandler::OnShow(
Profile* profile,
const std::string& id) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: redundant line

if (!ads_service_) {
auto notification = base::BindOnce(&AdsNotificationHandler::OnShow,
base::Unretained(this), profile, id);
pending_notifications_.push(std::move(notification));
return;
}

#if defined (OS_ANDROID)
headless_shutdown_timer_.Stop();
#endif

ads_service_->OnShow(profile, id);
}

Expand All @@ -46,16 +56,27 @@ void AdsNotificationHandler::OnClose(
const std::string& id,
bool by_user,
base::OnceClosure completed_closure) {

base::OnceClosure completed_closure_local =
base::BindOnce(&AdsNotificationHandler::CloseOperationCompleted,
base::Unretained(this), id);
pending_close_callbacks_.emplace(id, std::move(completed_closure));

if (!ads_service_) {
auto notification = base::BindOnce(
&AdsNotificationHandler::OnClose, base::Unretained(this), profile,
origin, id, by_user, std::move(completed_closure));
origin, id, by_user, std::move(completed_closure_local));
pending_notifications_.push(std::move(notification));
return;
}

#if defined (OS_ANDROID)
last_dismissed_notification_id_ = id;
headless_shutdown_timer_.Stop();
#endif

ads_service_->OnClose(profile, origin, id, by_user,
std::move(completed_closure));
std::move(completed_closure_local));
}

void AdsNotificationHandler::OnClick(
Expand All @@ -65,6 +86,7 @@ void AdsNotificationHandler::OnClick(
const base::Optional<int>& action_index,
const base::Optional<base::string16>& reply,
base::OnceClosure completed_closure) {

if (!ads_service_) {
auto notification = base::BindOnce(
&AdsNotificationHandler::OnClick, base::Unretained(this), profile,
Expand All @@ -73,12 +95,20 @@ void AdsNotificationHandler::OnClick(
return;
}

#if defined (OS_ANDROID)
headless_shutdown_timer_.Stop();
#endif

ads_service_->ViewAdNotification(id);
}

void AdsNotificationHandler::DisableNotifications(
Profile* profile,
const GURL& origin) {

#if defined (OS_ANDROID)
headless_shutdown_timer_.Stop();
#endif
}

void AdsNotificationHandler::OpenSettings(
Expand All @@ -94,6 +124,10 @@ void AdsNotificationHandler::OpenSettings(
return;
}

#if defined (OS_ANDROID)
headless_shutdown_timer_.Stop();
#endif

ads_service_->ViewAdNotification(id);
}

Expand All @@ -120,4 +154,47 @@ const void* AdsNotificationHandler::UserDataKey() {
return &kUserDataKey;
}

void AdsNotificationHandler::CloseOperationCompleted(
const std::string& notification_id) {

auto iter = pending_close_callbacks_.find(notification_id);
if (iter != pending_close_callbacks_.end()) {
std::move(iter->second).Run();
pending_close_callbacks_.erase(iter);
}
#if defined(OS_ANDROID)
StartShutDownTimerIfNecessary(notification_id);
#endif
}

#if defined(OS_ANDROID)
bool AdsNotificationHandler::IsHeadless() {
using base::android::ApplicationState;
auto state = base::android::ApplicationStatusListener::GetState();
bool headless = (state == ApplicationState::APPLICATION_STATE_UNKNOWN ||
state == ApplicationState::APPLICATION_STATE_HAS_DESTROYED_ACTIVITIES);
return headless;
}

void AdsNotificationHandler::StartShutDownTimerIfNecessary(
const std::string & last_processed_notification_id) {
if (IsHeadless() && last_dismissed_notification_id_ ==
last_processed_notification_id) {
last_dismissed_notification_id_ = "";
// wait and close the browser if running headless
headless_shutdown_timer_.Start(FROM_HERE,
base::TimeDelta::FromSeconds(kWaitBeforeShutdownWhenRunHeadless),
this, &AdsNotificationHandler::ShutdownTimerCallback);
}
}

// Check if browser is runnning without UI and close it.
// If during shutdown a new notification event arrives-
// timer is stopped in corresponding handler function
void AdsNotificationHandler::ShutdownTimerCallback() {
if (IsHeadless()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it safe to assume that any time Android is running in a headless mode, it's due to our ad notifications and is therefore safe to terminate? It seems like it would cause problems if that turned out not to be the case. At a minimum, we should probably have some kind of log here I think.

It seems like the issue we're trying to deal with is that the browser starts up with no UI when a notification is dismissed. Do we understand why that happens though? Maybe there's a less agressive way to deal with that scenario? cc: @bridiver

Copy link
Contributor Author

@gdregalo gdregalo Mar 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I noticed, the Chromium 80 behaves the same way when a web notification is dismissed. But Brave starts serving ads in the background, which is not expected. We also don't want to consume extra battery resources when it's not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't serve ads when we're running in a headless state, would it be a decent compromise? Seems like it may be good to try to follow the "standard" Chromium behavior (such as it is) as much as possible, just to future-proof ourselves when they inevitably change something here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about to save some battery charge?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know too much about the battery cost, but it seems that if Google allows this mode for web notifications they probably decided it was a negligible cost to pay.

Anyway, will approve on my side - don't want to hold this up, but curious what others think.

chrome::TerminateAndroid();
}
}
#endif
} // namespace brave_ads
21 changes: 21 additions & 0 deletions components/brave_ads/browser/ads_notification_handler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#ifndef BRAVE_COMPONENTS_BRAVE_ADS_BROWSER_ADS_NOTIFICATION_HANDLER_H_
#define BRAVE_COMPONENTS_BRAVE_ADS_BROWSER_ADS_NOTIFICATION_HANDLER_H_

#include <map>
#include <queue>
#include <string>
#include <utility>
Expand All @@ -17,6 +18,10 @@
#include "chrome/browser/notifications/notification_handler.h"
#include "url/gurl.h"

#if defined(OS_ANDROID)
#include "base/timer/timer.h"
#endif

namespace content {
class BrowserContext;
} // namespace content
Expand Down Expand Up @@ -80,7 +85,23 @@ class AdsNotificationHandler : public NotificationHandler {

private:
void SendPendingNotifications();
void CloseOperationCompleted(const std::string& notification_id);

#if defined(OS_ANDROID)
static bool IsHeadless();
void StartShutDownTimerIfNecessary(
const std::string & last_processed_notification_id);
void ShutdownTimerCallback();

std::string last_dismissed_notification_id_;
// timer to wait before shutdwon when running UI-less
base::OneShotTimer headless_shutdown_timer_;

// wait before shutdown if running headless
const int64_t kWaitBeforeShutdownWhenRunHeadless = 30; // sec
#endif

std::map<std::string, base::OnceClosure> pending_close_callbacks_;
content::BrowserContext* browser_context_;
brave_ads::AdsServiceImpl* ads_service_;
base::queue<base::OnceClosure> pending_notifications_;
Expand Down