Custom Notification master #6451
Conversation
yachtcaptain23
force-pushed
the
albert-custom-notification-master
branch
from
da2f74f to
eda228b
Compare
browser/notifications/notification_platform_bridge_brave_custom_notification.cc
Outdated
Show resolved
Hide resolved
|
With regards to Chromium message center code which we have based custom ad notifications from, are these any files which have not changed? |
padded_button.cc |
yachtcaptain23
force-pushed
the
albert-custom-notification-master
branch
from
43af083 to
412d5c1
Compare
|
@yachtcaptain23 can you please update the attached Android screenshot to include the BAT icon, thanks |
|
@yachtcaptain23 It does not appear the changes we made for timing out ad notifications on Android was merged? as the above code is wrapped in |
|
|
||
| namespace brave_custom_notification { | ||
|
|
||
| ThunkNotificationDelegate::ThunkNotificationDelegate( |
There was a problem hiding this comment.
why are we copying this code?
browser/BUILD.gn
Outdated
| ] | ||
|
|
||
| sources += [ | ||
| "brave_ads/android/brave_ads_native_helper.cc", |
There was a problem hiding this comment.
these should go in brave/browser/brave_ads/android/BUILD.gn
browser/notifications/BUILD.gn
Outdated
| "//skia", | ||
| ] | ||
| sources = [ | ||
| "brave_notification_platform_bridge_helper_android.cc", |
There was a problem hiding this comment.
implicit source filters are deprecated. Please use is_android guard
| ] | ||
| } | ||
| deps = [ | ||
| "//base", |
There was a problem hiding this comment.
these deps are not correct. You should have a dep for header in these files (excluding chrome/*) and only the headers in the files (no indirect deps)
There was a problem hiding this comment.
I changed it, do these look fine now?
| #include "base/memory/scoped_refptr.h" | ||
| #include "base/run_loop.h" | ||
| #include "base/task/post_task.h" | ||
| #include "brave/components/brave_ads/browser/ads_notification_handler.h" |
There was a problem hiding this comment.
this is definitely a problem. If this is a generic custom notification it should not have dependencies on brave_ads. If it is specific to ads then it should be in browser/brave_ads/notifications
| #include "brave/components/brave_ads/browser/ads_service_impl.h" | ||
| #include "brave/ui/brave_custom_notification/message_popup_view.h" | ||
| #include "brave/ui/brave_custom_notification/public/cpp/notification.h" | ||
| #include "chrome/browser/browser_process.h" |
There was a problem hiding this comment.
this header list does not appear to be correct. I don't see any references from browser_process.h and you're missing browser_context.h just as a few examples. Please ensure that you include what you use and only what you use https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYU.md
|
|
||
| #include "base/strings/string_util.h" | ||
| #include "base/strings/utf_string_conversions.h" | ||
| #include "bat/ads/ad_notification_info.h" | ||
| #include "brave/ui/brave_custom_notification/public/cpp/notification.h" |
There was a problem hiding this comment.
why do we have brave/ui and ui/message_center includes here?
| @@ -55,6 +56,7 @@ | |||
| #include "chrome/browser/profiles/profile_manager.h" | |||
| #include "chrome/browser/profiles/profile.h" | |||
| #if !defined(OS_ANDROID) | |||
| #include "brave/ui/brave_custom_notification/message_popup_view.h" | |||
There was a problem hiding this comment.
nit: brave_custom_notification seems excessively long and brave_ is redundant. I guess what we name this depends on whether it is actually a generic custom notification or specific to ads
ui/brave_custom_notification/DEPS
Outdated
| @@ -0,0 +1,18 @@ | |||
| include_rules = [ | |||
| "+chrome/browser/profiles", | |||
There was a problem hiding this comment.
this is not allowed. If components depends on //brave/ui/* then //brave/ui/* cannot depend on //chrome/*
| } | ||
|
|
||
| sources = [ | ||
| "ad_notification_view_md.cc", |
There was a problem hiding this comment.
nothing ads specific should go in //brave/ui
| * License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
| * You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| #include "brave/browser/notifications/notification_platform_bridge_brave_custom_notification.h" |
There was a problem hiding this comment.
why do we have a bridge when these aren't using platform notifications?
There was a problem hiding this comment.
AdsServiceImpl was built around using NotificationPlatformBridge, and originally Android was going to be in a separate PR. Since Android is part of this PR, we can rename it to something like AdsNotificationViewController or something similar. Let me know your thoughts.
ui/brave_custom_notification/OWNERS
Outdated
| @@ -0,0 +1,3 @@ | |||
| [email protected] | |||
There was a problem hiding this comment.
Can we please remove as we try to keep code owners for rewards to a minimum
|
@simonhong can you check this out? 😄 |
yachtcaptain23
force-pushed
the
albert-custom-notification-master
branch
from
1133053 to
2f560c6
Compare
tmancey
force-pushed
the
albert-custom-notification-master
branch
from
862ae5e to
6cdfd8f
Compare
yachtcaptain23
force-pushed
the
albert-custom-notification-master
branch
from
1c388ed to
66044c6
Compare
Support Android ads notifications including support for Poppins for header and body. Support OOBE on Android
yachtcaptain23
force-pushed
the
albert-custom-notification-master
branch
from
fd386d7 to
23ff849
Compare
Closes brave/brave-browser#9592