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

System/OS ad notifications are missing icons #18154

Closed
stephendonner opened this issue Sep 16, 2021 · 8 comments · Fixed by brave/brave-core#10172
Closed

System/OS ad notifications are missing icons #18154

stephendonner opened this issue Sep 16, 2021 · 8 comments · Fixed by brave/brave-core#10172
Assignees
@mkarolin
Labels
bug OS/Desktop OS/macOS priority/P2 A bad problem. We might uplift this to the next planned release. QA Pass-macOS QA/Yes regression release/blocking release-notes/exclude
Milestone
1.30.x - Release

Comments

@stephendonner
Copy link

stephendonner commented Sep 16, 2021 •
edited
Loading

Description

System/OS ad notifications are missing icons

Steps to Reproduce

  1. new profile
  2. launch Brave using --enable-logging=stderr --vmodule="*/variations/*"=6,"*/bat-native-ledger/*"=6,"*/brave_rewards/*"=6,"*/bat-native-ads/*"=6,"*/bat-native-confirmations/*"=6,"*/brave_ads/*"=9,"*/brave_user_model/*"=6 --brave-ads-staging --brave-ads-debug --rewards=staging=true,reconcile-interval=3
  3. open a new-tab page
  4. click on the Start using Rewards button
  5. open 3 or so more tabs
  6. wait a bit
  7. move the mouse/keyboard
  8. look at the system-level Brave Ads ad notification

Actual Result:
It's missing its icon/favicon

Screen Shot 2021-09-16 at 3 29 01 AM |
Screen Shot 2021-09-16 at 3 53 18 AM

Expected Result:

Should have its respective icon/favicon

Screen Shot 2021-09-16 at 3 43 23 AM

BTW: custom-ad notifications are fine 👍

Screen Shot 2021-09-16 at 3 09 12 AM

Reproduces how often:

100%

Brave version (brave://version info)

Brave 1.31.46 Chromium: 94.0.4606.41 (Official Build) nightly (x86_64)
Revision 333e85df3c9b656b518b5f1add5ff246365b6c24-refs/branch-heads/4606@{#845}
OS macOS Version 12.0 (Build 21A5506j)

cc @kjozwiak @tmancey @btlechowski @GeetaSarvadnya
@tmancey
Copy link
Contributor

tmancey commented Sep 16, 2021

@stephendonner can you please reboot your computer and see if that resolves the issue. System notifications show the app icon, so this bug is outside of the ads library. But first, lets see if a reboot resolves the issue.

@stephendonner
Copy link
Author

@stephendonner can you please reboot your computer and see if that resolves the issue. System notifications show the app icon, so this bug is outside of the ads library. But first, lets see if a reboot resolves the issue.

I still see this on two separate MacBook Pro laptops, both after rebooting.

@kjozwiak
Copy link
Member

Definitely looks like a regression as I managed to reproduce this on my MPB as well. Example:

Using 1.31.46 Chromium: 94.0.4606.41, I could reproduce the issue @stephendonner was seeing re: the icon missing form the ad notification. Example:

Custom ad notification (working) Default ad notification (Broken)
Screen Shot 2021-09-16 at 1 33 54 PM Screen Shot 2021-09-16 at 1 36 15 PM

I double checked 1.31.37 Chromium: 93.0.4577.63 which was still using 93.0.4577.63 and the icon was being displayed without any issues:

Screen Shot 2021-09-16 at 1 41 17 PM

@tmancey definitely a c94 regression that needs to be fixed for 1.30.x.

Builds used during verification:

Brave | 1.31.46 Chromium: 94.0.4606.41 (Official Build) nightly (x86_64)
--- | ---
Revision | 333e85df3c9b656b518b5f1add5ff246365b6c24-refs/branch-heads/4606@{#845}
OS | macOS Version 11.5.2 (Build 20G95)

Brave | 1.31.37 Chromium: 93.0.4577.63 (Official Build) nightly (x86_64)
--- | ---
Revision | ff5c0da2ec0adeaed5550e6c7e98417dac77d98a-refs/branch-heads/4577@{#1135}
OS | macOS Version 11.5.2 (Build 20G95)

@stephendonner
Copy link
Author

stephendonner commented Sep 16, 2021 •
edited
Loading

There's also this, on dev, even, so it feels lower-level than Ads:

Screen Shot 2021-09-16 at 10 46 06 AM

@jsecretan jsecretan added the priority/P2 A bad problem. We might uplift this to the next planned release. label Sep 16, 2021
@stephendonner
Copy link
Author

stephendonner commented Sep 16, 2021 •
edited
Loading

This is lower-level than Ads, so removing its keyword and clearing from Ads project.

@bsclifton
Copy link
Member

cc: @simonhong

@kjozwiak kjozwiak mentioned this issue Sep 20, 2021
Closed
@kjozwiak kjozwiak added this to the 1.30.x - Release milestone Sep 21, 2021
@mkarolin mkarolin mentioned this issue Sep 21, 2021
Merged
24 tasks
mkarolin added a commit to brave/brave-core that referenced this issue Sep 21, 2021
@mkarolin
Fixes MacOS notifications icon
aa44159 
Fixes brave/brave-browser#18154

Due to Chromium change below our prior patch to chrome/BUILD.gn was
always overwriting plist of helper apps when it was only meant to do
so for one app.

Chromium change:

https://source.chromium.org/chromium/chromium/src/+/56915522276f86acc55f86b143b4d7b260d6bc36

commit 56915522276f86acc55f86b143b4d7b260d6bc36
Author: Richard Knoll <knollr@chromium.org>
Date:   Fri Mar 5 16:07:34 2021 +0000

    Introduce alert notification helper .app

    This adds a new helper .app on macOS to display alert notifications.
    This app is required to show alert style notifications as the main app
    can only show banner style ones and the XPC service can not use the new
    UNNotification APIs.

    Bug: 1127306
This was referenced Sep 21, 2021
Merged
Merged
@LaurenWags
Copy link
Member

labelling as QA/Blocked until uplift is completed and an RC containing this fix is created so it can be tested.

@LaurenWags LaurenWags mentioned this issue Sep 22, 2021
Closed
@LaurenWags
Copy link
Member

LaurenWags commented Sep 23, 2021 •
edited by stephendonner
Loading

Verification PASSED on macOS 11.6 x64 using the following build:

Brave	1.30.84 Chromium: 94.0.4606.54 (Official Build) (x86_64)
Revision	c8191a1d5cccbf64e8fe7269043f8ace8d74dd05-refs/branch-heads/4606@{#1130}
OS	macOS Version 11.6 (Build 20G165)
Installer Applications Notification Prompt Ads Notification
Installer (1) Applications (1) Notification prompt (1) Ad Notification

Verification PASSED on macOS 10.15.7 x64 using the following build:

Brave | 1.30.84 Chromium: 94.0.4606.54 (Official Build) (x86_64)
-- | --
Revision | c8191a1d5cccbf64e8fe7269043f8ace8d74dd05-refs/branch-heads/4606@{#1130}
OS | macOS Version 10.15.7 (Build 19H1417)
Installer Applications Notification Prompt Ads Notification
Catalina - Installer Catalina - Applications Catalina - Notification Prompt Catalina - Ad Notification

Verification PASSED on macOS 11.6 x64 using the following build:

Brave | 1.30.84 Chromium: 94.0.4606.54 (Official Build) (x86_64)
--- | ---
Revision | c8191a1d5cccbf64e8fe7269043f8ace8d74dd05-refs/branch-heads/4606@{#1130}
OS | macOS Version 11.5.2 (Build 20G95)
Installer Applications Notification Prompt Ads Notification
Screen Shot 2021-09-23 at 12 01 58 PM Screen Shot 2021-09-23 at 12 02 17 PM Screen Shot 2021-09-23 at 12 02 26 PM Screen Shot 2021-09-23 at 12 04 50 PM

Verified PASSED using

Brave 1.30.84 Chromium: 94.0.4606.54 (Official Build) (x86_64)
Revision c8191a1d5cccbf64e8fe7269043f8ace8d74dd05-refs/branch-heads/4606@{#1130}
OS macOS Version 11.6 (Build 20G165)
Installer Applications Notification Prompt Ads Notification
Screen Shot 2021-09-23 at 9 03 24 AM Screen Shot 2021-09-23 at 9 04 09 AM Screen Shot 2021-09-23 at 9 04 21 AM Screen Shot 2021-09-23 at 9 05 42 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mkarolin mkarolin

Labels
bug OS/Desktop OS/macOS priority/P2 A bad problem. We might uplift this to the next planned release. QA Pass-macOS QA/Yes regression release/blocking release-notes/exclude
Projects
None yet
Milestone
1.30.x - Release
Development

Successfully merging a pull request may close this issue.

Fixes MacOS notifications icon brave/brave-core
8 participants
@jsecretan @stephendonner @tmancey @kjozwiak @bsclifton @simonhong @LaurenWags @mkarolin