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

[DO NOT MERGE]Migrate playlist kotlin module #26628

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

[DO NOT MERGE]Migrate playlist kotlin module #26628

wants to merge 19 commits into from
+11,024 −386

Conversation

deeppandya
Copy link
Contributor

@deeppandya deeppandya commented Nov 19, 2024 •
edited
Loading

Resolves brave/brave-browser#42359

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • 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 wiki
    • npm run presubmit wiki, 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:

@deeppandya deeppandya added this to the 1.75.x - Nightly milestone Nov 19, 2024
@deeppandya deeppandya self-assigned this Nov 19, 2024
@deeppandya deeppandya marked this pull request as ready for review November 19, 2024 04:11
@deeppandya deeppandya requested review from a team and bridiver as code owners November 19, 2024 04:11
mkarolin
mkarolin approved these changes Nov 19, 2024
View reviewed changes
Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

strings and patches LGTM

SergeyZhukovsky
SergeyZhukovsky approved these changes Nov 22, 2024
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.

++
Please create a follow up to cover with asserts all IO operations

@deeppandya deeppandya requested a review from a team as a code owner November 29, 2024 02:08
@deeppandya deeppandya force-pushed the migrate_playlist_kotlin_module branch from a7760c7 to 9cbc40e Compare December 2, 2024 02:08
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 2, 2024

Chromium major version is behind target branch (131.0.6778.85 vs 132.0.6834.15). Please rebase.

@github-actions github-actions bot added the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Dec 2, 2024
samartnik
samartnik approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@samartnik samartnik left a comment

Choose a reason for hiding this comment

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

++

@deeppandya deeppandya force-pushed the migrate_playlist_kotlin_module branch from 9cbc40e to df6c148 Compare December 3, 2024 07:09
@github-actions github-actions bot removed the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Dec 3, 2024
@deeppandya deeppandya force-pushed the migrate_playlist_kotlin_module branch from 7cab55b to 5fcb32b Compare December 17, 2024 19:56
bridiver
bridiver reviewed Dec 18, 2024
View reviewed changes
browser/playlist/android/BUILD.gn Outdated
import("//chrome/common/features.gni")
import("//tools/grit/grit_rule.gni")

android_library("playlist_java") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

playlist_ is redundant, the convention is to use java

bridiver
bridiver reviewed Dec 18, 2024
View reviewed changes
browser/playlist/android/BUILD.gn Outdated
resources_package = "org.chromium.chrome.browser.playlist"
}

android_resources("playlist_resources") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above re playlist_

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bridiver i have updated above as per : https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/autofill/android/BUILD.gn;l=143

bridiver
bridiver reviewed Dec 18, 2024
View reviewed changes
build/commands/lib/l10nUtil.js
@@ -193,7 +194,8 @@ const l10nUtil = {
braveSpecificGeneratedResourcesPath,
braveResourcesComponentsStringsPath,
braveExtensionMessagesPath,
braveAndroidBraveStringsPath
braveAndroidBraveStringsPath,
Copy link
Collaborator

@bridiver bridiver Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@goodov this seems error prone to have to manually add these, we should maybe just search for patterns like *_strings.grd?

bridiver
bridiver reviewed Dec 18, 2024
View reviewed changes
...r/playlist/android/java/src/org/chromium/chrome/browser/playlist/PlaylistItemDiffCallback.kt
* You can obtain one at https://mozilla.org/MPL/2.0/.
*/

package org.chromium.chrome.browser.playlist
Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems like these should be org.chromium.brave, but should probably have a discussion in the android channel

Copy link
Collaborator

Choose a reason for hiding this comment

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

would be a follow-up if we decide to change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a follow up issue here : brave/brave-browser#42964

@deeppandya deeppandya mentioned this pull request Dec 18, 2024
Open
@deeppandya deeppandya changed the title Migrate playlist kotlin module [DO NOT MERGE]Migrate playlist kotlin module Dec 19, 2024
deeppandya and others added 19 commits January 6, 2025 09:57
@deeppandya
Add kotlin classes from playlist kotlin module into brave-core
2dd39d3
@deeppandya
Add kotlin module layouts files
d599577
@deeppandya
Add colors, dimens, styles, ids from playlist module
75dcc60
@deeppandya
Update playlist changes with kotlin package changes
5b872bb
@deeppandya
Add kotlin resources
438cba7 
Remove kotlin enums

Resolve warnings with kotlin classes
@deeppandya
Add drawables from playlist module
9561f63
@deeppandya
Remove playlist deps
00d4d39
@deeppandya
Remove unused string patch for playlist module
e96ec16 
Remove unused strings
@deeppandya
Add playlist sources and deps to .gni
c074152
@deeppandya
Add missing deps for androidx_transition, constraintlayout and corout…
cc15390 
…ines
@bsclifton @deeppandya
Modularize playlist resources
2506381 
Rework playlist Java and Kotlin sources using `android_library`

Add missing resources into playlist

Add missing deps
@deeppandya
Update jni headers deps
24f76e3
@deeppandya
Add playlist strings into playlist module
d117602
@deeppandya
Update kotlin allow list patch
19d5d24 
Add missing deps
@deeppandya
Add playlist onboarding gradient colors
a462694 
Update playlist factory for android path
@deeppandya
Move kotlin module to brave/browser/playlist/android/
1367921 
Update deps after moving kotlin files

Remove unused import
@deeppandya
Add sources.gni for kotlin files
d7bf71f 
Update allow kt files patch

Update license header

Update playlist resources
@deeppandya
Revert java sources to master
de87601
@deeppandya
Update android_library as per the suggestions
362d4b0
@deeppandya deeppandya force-pushed the migrate_playlist_kotlin_module branch from 3c3ebed to 362d4b0 Compare January 6, 2025 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsclifton bsclifton bsclifton left review comments

@mkarolin mkarolin mkarolin approved these changes

@samartnik samartnik samartnik approved these changes

@SergeyZhukovsky SergeyZhukovsky SergeyZhukovsky approved these changes

@bridiver bridiver bridiver approved these changes

Assignees

@deeppandya deeppandya

Labels
None yet
Projects
None yet
Milestone
1.75.x - Beta
Development

Successfully merging this pull request may close these issues.

[Android] Move playlist kotlin module into Brave core
6 participants
@deeppandya @bridiver @bsclifton @SergeyZhukovsky @samartnik @mkarolin