New Tab Page Branded Wallpapers (desktop) #4075
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
petemill
force-pushed
the
ntp-sbw
branch
2 times, most recently
from
5c4de7a to
26905bc
Compare
petemill
force-pushed
the
ntp-sbw
branch
3 times, most recently
from
05ed1ab to
32b76ae
Compare
petemill
added
CI/skip-android
petemill
force-pushed
the
ntp-sbw
branch
3 times, most recently
from
b4640ac to
4c3165e
Compare
petemill
force-pushed
the
ntp-sbw
branch
3 times, most recently
from
1a00222 to
59c61b8
Compare
petemill
requested review from
ryanml,
cezaraugusto,
NejcZdovc,
bsclifton and
simonhong
|
Tagging the following for review: @cezaraugusto - multiple NTP changes to tighten the grid, allow space collapsing and allow more automatic side-by-side placement. Please check out the layout changes especially. @ryanml - NTP Rewards widget updates to allow for multiple overlapping but offset notifications, and for messaging and control about the branded wallpaper @simonhong who will be adding a remote component fetcher to the backend |
petemill
added
CI/skip-ios
NTPSponsoredImagesService was used only for adding URLDataSource. and we have another service for ntp sponsorship. So, unified to ViewCounter service.
To NTPSponsoredImagesService because it doesn't manage component. And fixed lint errors and added missing deps.
…ide of ViewCounterServiceFactory
|
This has had a lot of testing last week and over the weekend. Lots of fixes, per review feedback. I believe this is ready for merge 😄 Going to merge and start a Nightly build so we can verify the feature |
|
Should be noted: last CI ran perfectly; there were some lint warnings which @petemill addressed and a |
simonhong
reviewed
Jan 27, 2020
components/ntp_sponsored_images/browser/ntp_sponsored_images_service.h
Outdated
Show resolved
Hide resolved
|
LGTM++ |
This was referenced Jan 27, 2020
|
Verified using a mix of 1.5.62 and 1.5.63
Additional testing done was as follows:
|
|
Verification passed on
Verification passed on
Verified test plan from: #4075 (comment)
|
|
Verified in Linux
|
petemill
pushed a commit
that referenced
this pull request
Jan 29, 2020
New Tab Page Branded Wallpapers (desktop)
This was referenced Jan 29, 2020
Merged
Merged
petemill
pushed a commit
that referenced
this pull request
Jan 29, 2020
New Tab Page Branded Wallpapers (desktop)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Branded Wallpapers for New Tab Page (NTP).
Enables a new type of wallpaper for NTP that shows every 4th page view (or 2nd after any browser or profile startup). This wallpaper comes with a branding 'logo' and a destination URL. The wallpaper can have multiple backgrounds that are shown in sequence for each page view which shows this special wallpaper.
Fixes:
Follow-ups and fixes after this PR:
"Turn on {Brave Rewards | Brave Ads}"stringschemaVersioncheckEnabling
The feature itself is behind a feature flag for now. Demo content for the feature is behind a second feature flag.
Data
Data is retrieved remotely via region-specific component. Component contains JSON file which describes the wallpaper image filenames, the logo image filename, the company name, image alt-text, and a destination url for clicking on the logo. The referenced image files are also contained within the component.
If the Demo feature flag is Enabled, then this remote component data will not be used. Instead, data built-in to the binary will be used.
If a CLI flag
--ntp-branded-data-pathis provided with a valid FS directory path, then that directory will be used and the remote component will not be used or fetched.NTP Rewards widget changes:
The rewards widget has been changed to inform users about the Branded Wallpaper for the following states:
It also has been adapted to show itself for the above states even when the Rewards Widget is turned OFF. In this context, it will be shown as dismiss-able and only when there is a Branded Wallpaper being displayed.
NTP other front-end general changes:
Many front-end changes to the NTP were also made to accommodate a less rigid grid which allows items to be side-by-side more often (mostly so that the Logo widget can fold 'upwards' next to the Rewards widget). The grid also allows the space of all items to collapse when the user removes each one.
Test plans (In progress...)
1. Without the feature flag, nothing happens
2. With the feature flag and demo feature flag, demo sponsored content is shown
3. Remote content is fetched
(only works whilst in pre-release phase)
0. Fresh or previous profile
0. System OS region set to "United States"
4. Blank Remote content is fetched and parsed
(only works whilst in pre-release phase)
0. Fresh or previous profile
0. System OS region set to "Canada"
5. Branded Wallpaper shows every 4th NTP view and on the 2nd after browser or profile startup
6. Multiple background images are shown in succession
7. Opting out works
8. Re-opting-in works
9. NTP Grid items fit side-by-side
different for different window widths
10. The space for hidden grid items collapses
Window is wider than ~980px
On any NTP page (sponsored or not)
-> Top sites should move up
-> Rewards widget should move up
-> Footer does not move above other elements (😬 totally possible if calculations are incorrect 😬 )
On Sponsored Wallpaper NTP
Window is smaller than 800px
[1]
All items collapse to single column when there is not enough space for 2 columns
Branded wallpaper logo is clickable
On any sponsored Image, clicking the logo will open the destination site (brave.com for demo ads) in the current tab.
Test plans for Rewards Widget when Sponsored Images are shown
The plans below are part of the following flow-chart (suggest right-click and "Open Image in New Tab").
11. Rewards widget shows opt-in state (rewards)
-> Displays wallpaper-related opt-in "to get paid" state (always)
12. Rewards widget shows opt-in state (ads)
-> Displays wallpaper-related opt-in-to-ads to get "paid" state (once)
-> Does not show on subsequent page views when dismissed
13. Rewards widget shows opt-in state even when disabled
-> Displays opt-in state (once)
-> Does not show on subsequent page views when dismissed
14. Rewards widget shows post-opt-in state
-> Displays "You are being paid" notification
-> Does not show on subsequent page views when dismissed
15. Rewards widget shows post-opt-in state
-> Displays "You are being paid" notification
-> Does not show on subsequent page views when dismissed
16. Ads not supported in region
17. Toggling Rewards resets notification state
If a notification has been dismissed, turning rewards OFF, ON or OFF and ON, should bring back the notification until it is dismissed again.
18. Toggling Ads resets notification state
If a notification has been dismissed, turning ads OFF, ON or OFF and ON, should bring back the notification until it is dismissed again.