-
Notifications
You must be signed in to change notification settings - Fork 867
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.
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
enable android apk build from brave-core #2376
Conversation
9734562
to
9a31140
Compare
@bridiver what's the command for building the android output? Is it similar to https://github.com/brave/browser-android-tabs where we:
|
@petemill |
|
||
public_deps = [] | ||
deps = [ | ||
+ "//brave/utility", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't cause circular dependency?
//brave/utility
target has //chrome/utility
in its dep list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope
@@ -3,6 +3,9 @@ | |||
* 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 "extensions/buildflags/buildflags.h" | |||
|
|||
#if BUILDFLAG(ENABLE_EXTENSIONS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ToolbarView overriding is not just for extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this particular set of headers could not be included without triggering an error because they include other extension headers. This is likely because chromium doesn't test non-default options so while they aren't strictly for extensions, they cannot be used without extensions being enabled
@@ -38,8 +36,9 @@ bool RewriteManifestFile( | |||
// Add the public key | |||
DCHECK(!public_key.empty()); | |||
|
|||
FILE_PATH_LITERAL() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete?
@@ -33,7 +33,9 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) { | |||
registry); | |||
|
|||
// appearance | |||
#if !defined(OS_ANDROID) | |||
BraveThemeService::RegisterProfilePrefs(registry); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only this is in non-android? I think kLocationBarIsWide
is not related with android?
Or this is just for avoiding compile error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe the theme service exists on android and this fixed either compile or linking error
7bf5084
to
1a4b13d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like all file permissions are being changed from 100644 → 100755, could you reset that back to browser/extensions/brave_component_extension.cc 644?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some binary files to remove like ._BUILD.gn
@@ -33,7 +33,9 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) { | |||
registry); | |||
|
|||
// appearance | |||
#if !defined(OS_ANDROID) | |||
BraveThemeService::RegisterProfilePrefs(registry); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a dark theme on android from Chromium I believe behind a feature flag, but it might not be using this.
Doesn't block this PR in any case.
@@ -39,7 +37,7 @@ bool RewriteManifestFile( | |||
DCHECK(!public_key.empty()); | |||
|
|||
std::unique_ptr<base::DictionaryValue> final_manifest(manifest.DeepCopy()); | |||
final_manifest->SetString(extensions::manifest_keys::kPublicKey, public_key); | |||
final_manifest->SetString("key", public_key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal but we should define our own constants for these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem was using extensions::manifest_keys::*
in android
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if it really makes sense to define them again with the same values just for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's probably a better solution here though that separates extensions from components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
892754a
to
513d49d
Compare
@@ -524,8 +522,9 @@ bool ParsePaymentsPreferences(BraveLedger* ledger, | |||
break; | |||
|
|||
default: | |||
payments->min_visit_time = | |||
braveledger_ledger::_default_min_publisher_duration; | |||
payments->min_visit_time = 8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a better fix here would be to have a different gn file for this dep and move the constant there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, just wasn't sure if it was worth it for this since it's just migration from browser-laptop. I can open a ticket if you want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bridiver yes please, issue is fine for now. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"views/profiles/brave_profile_chooser_view.cc", | ||
"views/profiles/brave_profile_chooser_view.h", | ||
"webui/brave_welcome_ui.cc", | ||
"webui/brave_welcome_ui.h", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll want to have welcome_ui available in Android, we can fix later though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with landing as long as it passes CI or equivalent local builds if CI is being painful.
1ffdd2b
to
f02800e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also LGTM if builds are good. Nice refactoring 👍
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security
) onnpm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.