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

Brave news android #11121

Merged
merged 2 commits into from
Nov 25, 2021
Merged

Brave news android #11121

merged 2 commits into from
Nov 25, 2021

Conversation

alexsafe
Copy link
Contributor

@alexsafe alexsafe commented Nov 16, 2021 •
edited by bsclifton
Loading

Resolves brave/brave-browser#16398

Contains the android implementation for Brave news using the mojom service.

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Figma for news: https://www.figma.com/file/vXS7viFBwQH9YlPwZDGCAT/Mobile-Brave-News?node-id=192%3A1391
Specs: https://docs.google.com/document/d/1X9lA00zHa8pKol63i0O23LA4UcSHDYoagIOH2DqwljI/edit#heading=h.6xjamuox37y8
There might be some intentional discrepancies between the docs/design and the implementation.

IMPORTANT! The feature is under a flag and is planned to be kept in Nightly for now. It can be managed from brave://flags .Please make sure nothing is broken in the NTP for when the feature is also disabled.

Test plan for DISABLED state

  • Ensure brave://flags for Brave News is disabled
  • Relaunch Brave if needed
  • Visit new tab page
  • Ensure page looks like it did before Brave News landed (ex: you can't scroll; it still shows image attribution at bottom).
  • Ensure page looks like it did before Brave News landed on different devices and orientations
  • Settings to turn off Brave News should not be visible
  • There are no other obvious regressions that might have been caused by the news merge
  • If there's a way to check network traffic, we'll want to verify the app does NOT call https://brave-today-cdn.brave.com/sources.json

@alexsafe alexsafe marked this pull request as ready for review November 16, 2021 20:11
@alexsafe alexsafe requested a review from a team as a code owner November 16, 2021 20:11
@alexsafe alexsafe added CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Nov 16, 2021
@alexsafe alexsafe added this to the 1.34.x - Nightly milestone Nov 16, 2021
tmancey
tmancey reviewed Nov 16, 2021
View reviewed changes
browser/ui/BUILD.gn Outdated Show resolved Hide resolved
tmancey
tmancey reviewed Nov 16, 2021
View reviewed changes
components/brave_today/common/features.cc Outdated
Comment on lines 17 to 20
#if BUILDFLAG(ENABLE_BRAVE_NEWS) && defined(OS_ANDROID)
base::FEATURE_ENABLED_BY_DEFAULT
#else
base::FEATURE_DISABLED_BY_DEFAULT
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this intentional to enable by default on Androiid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, i thought so since we will be keeping it in Nightly for now. @petemill does this look ok to you?

Copy link
Member

Choose a reason for hiding this comment

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

perhaps we should disable them by default and enable in nightly channel via griffin only?

Copy link
Member

Choose a reason for hiding this comment

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

yes, how about this:
Enable by default for non-android (i.e. desktop)
Enable by default for local / non-official build (for android local development)
Disable by default for android

we will then enable on android nightly via griffin

Please manually test with flag on and off to check NTP doesn't crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

tmancey
tmancey requested changes Nov 16, 2021
View reviewed changes
Copy link
Collaborator

@tmancey tmancey left a comment

Choose a reason for hiding this comment

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

See comments. Thanks

@alexsafe alexsafe force-pushed the bravenews-android branch 2 times, most recently from cccad1f to ffc56b0 Compare November 23, 2021 23:10
AlexeyBarabash
AlexeyBarabash approved these changes Nov 24, 2021
View reviewed changes
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash left a comment

Choose a reason for hiding this comment

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

++
looking forward to see it on Nightly

@petemill
Copy link
Member

The latest merge commit seemed to combine 2 duplicate branches and a merge commit of master. It resulted in reverted some needed code. The android build failure in CI was the desktop NTP WebUI compile failing due to the removal of the mojom dep. I fixed the duplicate commit issues by creating a new squashed commit on master (making sure to preserve your "author" status @alexsafe!).

Code differences (which should be the fixes) can be seen here:
https://github.com/brave/brave-core/compare/61a4a0a66818e5ece0095b8c19ba78134085c3ed..ca7b71907ad37841cb1d1d18ee923b5d7e0b1895

petemill
petemill approved these changes Nov 24, 2021
View reviewed changes
Copy link
Member

@petemill petemill left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great work @alexsafe for getting every facet of this feature for Android working well! It was fun working with you on it. Once CI is ✅ we should be good to merge.

@petemill @alexsafe
NTP WebUI hides Brave News if feature flag is disabled
9fab483 
Feature flag is favored over buildflag now, so remove it from remaining
places.
mariospr
mariospr approved these changes Nov 25, 2021
View reviewed changes
Copy link
Contributor

@mariospr mariospr left a comment

Choose a reason for hiding this comment

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

LGTM for //chromium_src/chrome/browser/flags/android/chrome_feature_list.cc

@alexsafe alexsafe merged commit dd08a04 into master Nov 25, 2021
@alexsafe alexsafe deleted the bravenews-android branch November 25, 2021 15:58
@bsclifton bsclifton mentioned this pull request Nov 29, 2021
Closed
@kjozwiak
Copy link
Member

kjozwiak commented Nov 29, 2021 •
edited by stephendonner
Loading

Note: these cases are to ensure that there's no obvious regressions via NTP when Brave News is disabled. The reason for this check is that Brave News is staying on Nightly so we need to ensure that once channel migrations occur and the code moves into 1.34.x, we don't regress Brave even though Brave News will be disabled on BETA.

Verification PASSED on Pixel 6 running Android 12 using the following build:

1.34.43 Chromium: 96.0.4664.45
  • ensured that Brave News isn't available/visible under NTP (attempt to scroll down and ensure it's not possible)
  • ensured that NTP & NTP-SI load without any issues
  • ensured that tapping on the logos under NTP-SI correctly loads the sponsored website within the same tab without any issues
  • ensured that tapping on the image attribution links at the bottom correctly loads within the same tab (note: some are pointing to https://community.brave.com)
  • ensured that you can switch between the Top Sites & Privacy Stats widget stacks without any issues
  • ensured that Brave News isn't being displayed when tapping on the Hamburger Menu
  • ensured that Brave News isn't being displayed under Settings via the Features section
  • ensured that https://brave-today-cdn.brave.com wasn't being called when Brave News is disabled
  • ensured that https://brave-today-cdn.brave.com is being called once a user enables Brave News via the NTP
News Disabled News Disabled
Screenshot_20211129-144333 Screenshot_20211129-144339
News Enabled News Enabled
Screenshot_20211129-140844 Screenshot_20211129-140854
News Disabled News Enabled
saveNews newsEnabled

Verified the below items using Asus Zenfone (x86) with Android 6 running 1.34.43 Bravex86.apk:

  • ensured that Brave News isn't available/visible under NTP (attempt to scroll down and ensure it's not possible)
  • ensured that NTP loads without any issues (no NTP SI on x86 devices)
  • ensured that tapping on the image attribution links at the bottom correctly loads within the same tab (note: I only got one NTP image, even though my component says it's up to date)
  • ensured that you can switch between the Top Sites & Privacy Stats widget stacks without any issues
  • ensured that Brave News isn't being displayed when tapping on the Hamburger Menu
  • ensured that Brave News isn't being displayed under Settings via the Features section

Verification passed on Brave v1.34.43 on Samsung s7 (Android 8.0).

  • ensured that Brave News isn't available/visible under NTP (attempt to scroll down and ensure it's not possible)
  • enusred that NTP & NTP-SI load without any issues
  • ensured that tapping on the logos under NTP-SI correctly loads the sponsored website within the same tab without any issues
  • ensured that tapping on the image attribution links at the bottom correctly loads within the same tab (note: some are pointing to https://community.brave.com)
  • ensured that you can switch between the Top Sites & Privacy Stats widget stacks without any issues
  • ensured that Brave News isn't being displayed when tapping on the Hamburger Menu
  • ensured that Brave News isn't being displayed under Settings via the Features section

@LaurenWags LaurenWags mentioned this pull request Dec 7, 2021
Closed
bridiver
bridiver reviewed Dec 27, 2021
View reviewed changes
browser/brave_news/brave_news_controller_factory.cc
@@ -29,6 +29,21 @@ BraveNewsController* BraveNewsControllerFactory::GetForContext(
GetInstance()->GetServiceForBrowserContext(context, true));
}

// static
mojo::PendingRemote<mojom::BraveNewsController>
BraveNewsControllerFactory::GetRemoteService(content::BrowserContext* context) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

GetForContext should return the pending remote, GetRemoteService should go away and GetControllerForContext duplicates the existing GetForContext so I don't understand why it was added. I see // Wire up JS mojom to service in BraveNewTabUI so GetControllerForContext can be a private method with a friend class for BraveNewTabUI until that is updated

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry, not GetControllerForContext because BraveNewsControllerFactory -> BraveNewsServiceFactory, but a private GetServiceForContext that returns the raw pointer is ok for now until BraveNewTabUI is updated to use the mojo interface directly

bridiver
bridiver reviewed Dec 27, 2021
View reviewed changes
browser/brave_news/BUILD.gn
@@ -0,0 +1,21 @@
source_set("brave_news") {
sources = [
"brave_news_controller_factory.cc",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be BraveNewsService/BraveNewsServiceFactory

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

@bridiver bridiver bridiver left review comments

@mattmcalister mattmcalister mattmcalister left review comments

@petemill petemill petemill approved these changes

@tmancey tmancey tmancey approved these changes

@mariospr mariospr mariospr approved these changes

@SergeyZhukovsky SergeyZhukovsky SergeyZhukovsky approved these changes

@AlexeyBarabash AlexeyBarabash AlexeyBarabash approved these changes

@samartnik samartnik Awaiting requested review from samartnik samartnik is a code owner

@bsclifton bsclifton Awaiting requested review from bsclifton

@deeppandya deeppandya Awaiting requested review from deeppandya deeppandya is a code owner

Assignees

@alexsafe alexsafe

Labels
None yet
Projects
None yet
Milestone
1.34.x - Release
Development

Successfully merging this pull request may close these issues.

(Android) Brave News on the New Tab Page
10 participants
@alexsafe @AlexeyBarabash @petemill @kjozwiak @bridiver @mattmcalister @tmancey @mariospr @deeppandya @SergeyZhukovsky