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.

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

Brave Ad notifications on desktop should timeout consistently across platforms #8546

Open
tmancey opened this issue Mar 5, 2020 · 9 comments
Open

Comments

@tmancey
Copy link
Collaborator

@tmancey tmancey commented Mar 5, 2020

Brave Ad notifications should timeout consistently across desktop platforms. So we need to change Windows and Linux to timeout after 2 minutes.

Currently:
macOS: 2 minutes
Windows: ∞
Linux: ∞

When an ad notification is ready to be shown we call AdsServiceImpl::ShowNotification which shows a native OS notification and then start a timer to force close the notification after 120 seconds (see IMPORTANT notes concerning non code-signed binaries).

The callstack is as follows:

AdsServiceImpl::ShowNotification
AdsServiceImpl::NotificationTimedOut (after 120 seconds)
AdsServiceImpl::CloseNotification
display_service_->Close(NotificationHandler::Type::BRAVE_ADS, uuid); (fails to close the notification on Windows and Linux)
...
notification_platform_bridge_<platform>.cc

Replace <platform> with win, linux, macos, i.e. notification_platform_bridge_win.cc

IMPORTANT

Confirm the ad notification is a native OS notification and if not, let me know so I can help enable native notifications within the browser (as for various reasons Chromium notifications can be shown).

Changes to src/chrome files should be via patches, reach out if you need assistance. However, to find the cause of the issue, changes can be made locally.

Builds which have not been code-signed may timeout sooner, i.e. notifications on macOS timeout after 5 seconds on builds which have not been code-signed, so for sanity testing the fix I would set the time-out to 1 second.

@tmancey tmancey added this to New issues in Ads via automation Mar 5, 2020
@tmancey tmancey moved this from New issues to Backlog in Ads Mar 5, 2020
@tmancey tmancey moved this from Backlog to In progress in Ads Mar 19, 2020
@masparrow
Copy link

@masparrow masparrow commented Mar 23, 2020

Confirmed behaviour on Windows (at least) seems to be correct - ads time out after 2 mins as expected. Tested Nightly on activated Windows install, and no notifications are shown, getting error Failed to get notification settings from toast notifier - on checking Windows notification settings - there is no entry for Brave or Brave Nightly allowing notifications - will catch up with @tmancey to discuss.

@tmancey
Copy link
Collaborator Author

@tmancey tmancey commented Mar 23, 2020

Confirmed behaviour on Windows (at least) seems to be correct - ads time out after 2 mins as expected. Tested Nightly on activated Windows install, and no notifications are shown, getting error Failed to get notification settings from toast notifier - on checking Windows notification settings - there is no entry for Brave or Brave Nightly allowing notifications - will catch up with @tmancey to discuss.

@masparrow Our testing only showed this working as expected on debug builds, can you please confirm

@masparrow
Copy link

@masparrow masparrow commented Mar 24, 2020

@tmancey Apologies - missed the key word there - it should read 'Confirmed behaviour on Windows debug (at least)...'

Regarding Nightly not showing notifications at all (on Brave iMac Windows VM and my own Windows Dev PC) I have been through the thread you supplied (https://bravesoftware.slack.com/archives/C43LW5U2H/p1577987813048600?thread_ts=1577912851.045600) and followed steps regarding Outlook, but with respect to Brave. Reinstalling Nightly does not fix the issue on my own Windows PC, and checking through the registry and policy settings, everything seems in order as the Microsoft Outlook support thread sets out. Still investigating.

@tmancey
Copy link
Collaborator Author

@tmancey tmancey commented Mar 24, 2020

@masparrow For what we can tell, this is a bug with Windows, please reply to the chat thread to see if others found a solution and did not post

@tmancey tmancey moved this from In progress to Review in Ads Mar 24, 2020
@tmancey
Copy link
Collaborator Author

@tmancey tmancey commented Mar 24, 2020

After debugging this issue we have found that ad notifications are working as expected using debug and release builds for all channels (nightly, beta and release) on the following platforms:

macOS: 2 minutes
Windows: 2 minutes
Linux: 2 minutes

@tmancey tmancey moved this from Review to Done in Ads Mar 24, 2020
@tmancey
Copy link
Collaborator Author

@tmancey tmancey commented Mar 24, 2020

@kjozwiak @LaurenWags If QA could test to confirm they are seeing the same results, that would be great. Thanks

@LaurenWags
Copy link
Collaborator

@LaurenWags LaurenWags commented Mar 30, 2020

I can confirm I'm seeing 2 minute timeout for ads on macOS Mojave.

cc @GeetaSarvadnya @btlechowski to confirm on Windows/Linux.

@kjozwiak
Copy link
Member

@kjozwiak kjozwiak commented Mar 30, 2020

I can confirm I'm seeing 2 minute timeout for ads on macOS Mojave.

Same results on macOS 10.15.4 x64 Catalina. I actually checked this ~a few days ago when I was running through ads and the timeout was 2mins.

@tmancey
Copy link
Collaborator Author

@tmancey tmancey commented Mar 31, 2020

@LaurenWags @kjozwiak Would be great to get confirmation on Windows and Linux too. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Ads
  
Done
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.