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

Rollout custom ad notifications #120

Merged
merged 1 commit into from
Sep 15, 2021
Merged

Rollout custom ad notifications #120

merged 1 commit into from
Sep 15, 2021

Conversation

tmancey
Copy link
Contributor

@tmancey tmancey commented Sep 14, 2021 •
edited by kjozwiak
Loading

Rollout custom ad notifications on the release channel to:

  • Should show custom ad notifications:

    • For Windows 10 build 16299 and older (i.e. Windows 7, 8 and earlier versions of Windows)
    • For all Windows versions If native notifications are disabled (should not be shown if Focus Assist is enabled)
    • For macOS 10.13 and older
    • For Android 7 and older
    • For all versions of Linux

  • Should show native ad notifications:

    • For Windows 10 build 17134 and newer (should not be shown if Focus Assist is enabled)
    • For macOS 10.14 and newer if native notifications are enabled
    • For Android 8 and newer

  • Should not show native ad notifications:

    • For all versions of Linux

@tmancey tmancey self-assigned this Sep 14, 2021
@tmancey tmancey requested a review from a team as a code owner September 14, 2021 14:35
SergeyZhukovsky
SergeyZhukovsky approved these changes Sep 14, 2021
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.

++

moritzhaller
moritzhaller approved these changes Sep 14, 2021
View reviewed changes
Copy link
Contributor

@moritzhaller moritzhaller left a comment

Choose a reason for hiding this comment

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

LGTM as per screen share

@tmancey tmancey force-pushed the rollout_custom_ad_notifications branch from 0fe8cc5 to 718354e Compare September 14, 2021 17:28
goodov
goodov reviewed Sep 15, 2021
View reviewed changes
seed/seed.json Outdated
@@ -589,7 +593,37 @@
],
"filter": {
"min_version": "92.1.30.19",
"channel": ["NIGHTLY", "BETA"],
"min_os_version": "10.0.17134*",
Copy link
Member

Choose a reason for hiding this comment

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

looks like this is incorrect, see TEST(VersionTest, CompareToWildcardString).
https://github.com/chromium/chromium/blob/96.0.4643.1/base/version_unittest.cc#L148-L196

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@goodov unfortunately, our converter doesn't support this filter :-/
we definitely need better validation for seed.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added support for min_os_version and max_os_version via #126

goodov
goodov reviewed Sep 15, 2021
View reviewed changes
seed/seed.json Outdated
],
"filter": {
"min_version": "92.1.30.19",
"max_os_version": "10.0.16299*",
Copy link
Member

@goodov goodov Sep 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

same here and few items down

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

@tmancey
Rollout custom ad notifcations
54afab1 
Windows 10 build 16299 and older or if native notifications are disabled
macOS 10.13 and older
Android 7 and older
All vesions of Linux
@tmancey tmancey force-pushed the rollout_custom_ad_notifications branch from 718354e to 54afab1 Compare September 15, 2021 15:30
@tmancey tmancey requested a review from goodov September 15, 2021 15:31
@tmancey tmancey merged commit 1b4beb0 into main Sep 15, 2021
@tmancey tmancey deleted the rollout_custom_ad_notifications branch September 15, 2021 15:37
@btlechowski
Copy link

btlechowski commented Sep 23, 2021 •
edited by kjozwiak
Loading

Verification passed on

Brave 1.30.84 Chromium: 94.0.4606.54 (Official Build) (64-bit)
Revision c8191a1d5cccbf64e8fe7269043f8ace8d74dd05-refs/branch-heads/4606@{#1130}
OS Ubuntu 18.04 LTS

Verified Custom ads are enabled by default on Linux

image image

Verification passed on

Brave 1.30.84 Chromium: 94.0.4606.54 (Official Build) (64-bit)
Revision c8191a1d5cccbf64e8fe7269043f8ace8d74dd05-refs/branch-heads/4606@{#1130}
OS Windows 7 Service Pack 1 (Build 7601.24544)

For Windows 7 with default settings:

image image

For Windows 7 with brave://flags/#enable-system-notifications disabled

image image

For Windows 7 native system notifications:
Windows 7 does not support native system notifications

Verification PASSED on macOS 11.5.2 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)

Default settings (native notifications enabled)

  • launched Brave using -args --enable-logging=stderr --variations-server-url=https://variations.bravesoftware.com/seed
  • restarted the browser so the new variations are pulled
  • enabled rewards and ensured that native notifications were being triggered/used

Ensured the following is being displayed in the terminal and BraveAds.ShowCustomAdNotificationOnMacStudy is not being enabled.

[9470:259:0924/134633.843270:VERBOSE1:features.cc(45)] Study BraveAds.FrequencyCappingStudy is active (ExcludeAdIfWithinTimeWindow=0h)
[9470:259:0924/134633.843304:VERBOSE1:features.cc(45)] Study BraveAds.UserActivityStudy is active (Triggers=EMPTY/Threshold=0.0/IdleTimeThreshold=5)
Example Example
Screen Shot 2021-09-27 at 8 14 11 PM Screen Shot 2021-09-27 at 8 20 25 PM

Verification PASSED on macOS 11.5.2 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)

Native notifications disabled

  • launched Brave using -args --enable-logging=stderr --variations-server-url=https://variations.bravesoftware.com/seed
  • disabled System Notifications via brave://flags/#enable-system-notifications
  • restarted the browser so the new variations are pulled
  • enabled rewards and ensured that custom ad notifications were being triggered/used
Example Example
Screen Shot 2021-09-27 at 9 54 54 PM Screen Shot 2021-09-27 at 9 54 42 PM

This was referenced Sep 24, 2021
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moritzhaller moritzhaller moritzhaller approved these changes

@SergeyZhukovsky SergeyZhukovsky SergeyZhukovsky approved these changes

@iefremov iefremov Awaiting requested review from iefremov

@goodov goodov Awaiting requested review from goodov

Assignees

@tmancey tmancey

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tmancey @btlechowski @moritzhaller @iefremov @goodov @SergeyZhukovsky