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

merged 1 commit into from
Mar 6, 2020

Conversation

gdregalo
Copy link
Contributor

@gdregalo gdregalo commented Mar 2, 2020

Fixes brave/brave-browser#7917

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@gdregalo gdregalo added bug CI/skip-ios Do not run CI builds for iOS labels Mar 2, 2020
@gdregalo gdregalo requested a review from tmancey as a code owner March 2, 2020 22:54
@gdregalo gdregalo self-assigned this Mar 2, 2020
emerick
emerick reviewed Mar 2, 2020
View reviewed changes
components/brave_ads/browser/ads_notification_handler.cc Outdated
Comment on lines 59 to 64

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

If ads_service_ is null, will this code end up being called twice (due to the if clause below here that calls OnClose again)? Wasn't sure, but maybe something to check...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

components/brave_ads/browser/ads_notification_handler.cc Outdated
}

#if defined(OS_ANDROID)
bool AdsNotificationHandler::IsHeadless(){
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this function be const?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be static.

components/brave_ads/browser/ads_notification_handler.cc Outdated
const std::string& notification_id) {

auto iter = pending_callbacks_.find(notification_id);
std::move(iter->second).Run();
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we should check iter here before accessing it (or at least DCHECK if we're confident that it's impossible).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

components/brave_ads/browser/ads_notification_handler.cc Outdated
#if defined(OS_ANDROID)
bool AdsNotificationHandler::IsHeadless(){
auto state = base::android::ApplicationStatusListener::GetState();
bool uiless = (state ==
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Seems like we use headless, no_ui, and uiless. I would just stick with one everywhere for consistency so it's clear that they all refer to the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using headless everywhere

components/brave_ads/browser/ads_notification_handler.cc Outdated Show resolved Hide resolved
components/brave_ads/browser/ads_notification_handler.cc Outdated
last_processed_notification_id){
last_dismissed_notification_id_ = "";
//wait and close the browser if running headless
no_ui_shutdown_timer_.Stop();
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 think you need to explicitly call Stop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

components/brave_ads/browser/ads_notification_handler.cc
//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.

@gdregalo gdregalo requested a review from bridiver March 3, 2020 19:53
SergeyZhukovsky
SergeyZhukovsky approved these changes Mar 4, 2020
View reviewed changes
Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

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

++

tmancey
tmancey reviewed Mar 4, 2020
View reviewed changes
components/brave_ads/browser/ads_notification_handler.cc
@@ -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

tmancey
tmancey approved these changes Mar 4, 2020
View reviewed changes
Copy link
Collaborator

@tmancey tmancey left a comment

Choose a reason for hiding this comment

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

LGTM

@gdregalo
Copy link
Contributor Author

gdregalo commented Mar 5, 2020

rebased and squashed

@gdregalo gdregalo added this to the 1.7.x - Nightly milestone Mar 5, 2020
@gdregalo
Copy link
Contributor Author

gdregalo commented Mar 6, 2020

The only CI error (known issue) is:
[GoogleTest-1.8] - 1 test report file(s) were found with the pattern 'src/brave_browser_tests.xml' relative to '/Users/jenkins/jenkins/workspace/ild-pr_android_headless_shutdown' for the testing framework 'GoogleTest-1.8'.

@gdregalo gdregalo closed this Mar 6, 2020
@gdregalo gdregalo reopened this Mar 6, 2020
@gdregalo gdregalo merged commit f8f63c3 into master Mar 6, 2020
@gdregalo gdregalo deleted the android_headless_shutdown branch March 6, 2020 17:43
@bsclifton bsclifton mentioned this pull request Mar 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergeyZhukovsky SergeyZhukovsky SergeyZhukovsky approved these changes

@emerick emerick emerick approved these changes

@tmancey tmancey tmancey approved these changes

@bridiver bridiver Awaiting requested review from bridiver

Assignees

@gdregalo gdregalo

Labels
bug CI/skip-ios Do not run CI builds for iOS
Projects
None yet
Milestone
1.7.x - Release
Development

Successfully merging this pull request may close these issues.

Ad notification shows up even when Brave-Beta is not running
4 participants
@gdregalo @tmancey @emerick @SergeyZhukovsky