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

android chromium-130 changes #86

Merged
merged 6 commits into from
Nov 10, 2024
Merged

android chromium-130 changes #86

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
10e4ed3
//mojo/public/mojom/
Nikhil7174 Nov 3, 2024
421be2f
Merge branch 'chrome-130' of https://github.com/Ping-browser/ping-cor…
Nikhil7174 Nov 3, 2024
03b874e
android chromium-130 changes
Nikhil7174 Nov 3, 2024
9e806c7
android minor changes
Nikhil7174 Nov 4, 2024
e71b990
brave-stats hidden
Nikhil7174 Nov 4, 2024
d3bcbe3
resolved commets
Nikhil7174 Nov 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions android/java/org/chromium/chrome/browser/ntp/BraveNewTabPageLayout.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,8 @@ public void run() {

private void setNtpRecyclerView(LinearLayoutManager linearLayoutManager) {
mIsTopSitesEnabled = NtpUtil.shouldDisplayTopSites();
mIsBraveStatsEnabled = NtpUtil.shouldDisplayBraveStats();
Copy link
Collaborator

Choose a reason for hiding this comment

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

return false from inside this function instead. we wouldn't have to make changes at multiple places then

// mIsBraveStatsEnabled = NtpUtil.shouldDisplayBraveStats();
mIsBraveStatsEnabled = false;

if (mNtpAdapter == null) {
if (mActivity != null && !mActivity.isDestroyed() && !mActivity.isFinishing()) {
Expand Down Expand Up @@ -883,7 +884,8 @@ private void initPreferenceObserver() {
mNtpAdapter.setTopSitesEnabled(mIsTopSitesEnabled);
} else if (TextUtils.equals(
key, BackgroundImagesPreferences.PREF_SHOW_BRAVE_STATS)) {
mIsBraveStatsEnabled = NtpUtil.shouldDisplayBraveStats();
// mIsBraveStatsEnabled = NtpUtil.shouldDisplayBraveStats();
mIsBraveStatsEnabled = false;
mNtpAdapter.setBraveStatsEnabled(mIsBraveStatsEnabled);
}
};
Expand Down
2 changes: 1 addition & 1 deletion android/java/org/chromium/chrome/browser/ntp/BraveNtpAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ public BraveNtpAdapter(Activity activity, OnBraveNtpListener onBraveNtpListener,
mIsNewsLoading = isNewsLoading;
mRecyclerViewHeight = recyclerViewHeight;
mIsTopSitesEnabled = isTopSitesEnabled;
mIsBraveStatsEnabled = isBraveStatsEnabled;
mIsBraveStatsEnabled = false;
mIsDisplayNewsFeed = false;
mIsDisplayNewsOptin = false;
}
Expand Down
30 changes: 15 additions & 15 deletions android/java/org/chromium/chrome/browser/ntp/IncognitoNewTabPageView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,21 +85,21 @@ protected void onFinishInflate() {
// any shortcut causes the UrlBar to be focused. See ViewRootImpl.leaveTouchMode().
mScrollView.setDescendantFocusability(FOCUS_BEFORE_DESCENDANTS);

mVpnCta = findViewById(R.id.tv_try_vpn);
if (BraveVpnUtils.isVpnFeatureSupported(getContext())
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we just added a false in this If block then then else block would have hidden this as well? and the number of changes would become minimum which reduces the chances of conflicts later on

&& !BraveVpnNativeWorker.getInstance().isPurchasedUser()) {
mVpnCta.setOnClickListener(
v -> {
if (!InternetConnection.isNetworkAvailable(getContext())) {
Toast.makeText(getContext(), R.string.no_internet, Toast.LENGTH_SHORT)
.show();
} else {
BraveVpnUtils.openBraveVpnPlansActivity(getContext());
}
});
} else {
mVpnCta.setVisibility(View.GONE);
}
// mVpnCta = findViewById(R.id.tv_try_vpn);
// if (BraveVpnUtils.isVpnFeatureSupported(getContext())
// && !BraveVpnNativeWorker.getInstance().isPurchasedUser()) {
// mVpnCta.setOnClickListener(
// v -> {
// if (!InternetConnection.isNetworkAvailable(getContext())) {
// Toast.makeText(getContext(), R.string.no_internet, Toast.LENGTH_SHORT)
// .show();
// } else {
// BraveVpnUtils.openBraveVpnPlansActivity(getContext());
// }
// });
// } else {
// mVpnCta.setVisibility(View.GONE);
// }
}

/**
Expand Down
5 changes: 3 additions & 2 deletions android/java/org/chromium/chrome/browser/ntp/NtpUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,9 @@ public static void setDisplayTopSites(boolean shouldDisplayTopSites) {
}

public static boolean shouldDisplayBraveStats() {
return ChromeSharedPreferences.getInstance()
.readBoolean(BackgroundImagesPreferences.PREF_SHOW_BRAVE_STATS, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

changing true to false here would also set this preference as false and thus would return false? and we wouldn't need to comment the whole line. minimizing changes done in existing brave code is important because it will help speed up syncing with latest brave branch in future

Copy link
Collaborator Author

@Nikhil7174 Nikhil7174 Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

here func will first check what value does BackgroundImagesPreferences.PREF_SHOW_BRAVE_STATS returns if it returns true then func will return true, if false then func will return false and if the value does not exists then it will return true by default. So a possibility of returning true will still remain even if default value is set to false.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this default preference is not set anywhere else then it is being set from here. If we make this false by default then it'll return false the first time and since we have hidden all other options to change this preference therefore preference will never change to true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok

// return ChromeSharedPreferences.getInstance()
// .readBoolean(BackgroundImagesPreferences.PREF_SHOW_BRAVE_STATS, true);
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

we are already returning false here so changes in other files where this is being used is not needed

}

public static void setDisplayBraveStats(boolean shouldDisplayBraveStats) {
Expand Down
1 change: 0 additions & 1 deletion android/java/res/layout/brave_shields_tooltip_layout.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
android:layout_marginTop="14dp"
android:padding="16dp"
android:drawablePadding="10dp"
app:drawableStartCompat="@drawable/ic_brave_36"
android:text="@string/shield_tooltip_desc"
android:textSize="14sp"
android:textColor="@color/onboarding_welcome_text_color"
Expand Down
31 changes: 15 additions & 16 deletions android/java/res/layout/new_tab_page_incognito.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
app:layout_constraintGuide_end="32dp" />

<!-- Keep the ID below as it's been tested in Chromium code by IncognitoNewTabPageStation. -->
<ImageView
<!-- <ImageView
Copy link
Collaborator

Choose a reason for hiding this comment

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

what if we set a property here hiding these views. we won't have to comment the whole block of code then

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i will have to set the visibility none, ht, wd , padding, margin = 0. to keep the ui aligned

android:id="@+id/new_tab_incognito_icon"
android:layout_width="@dimen/new_tab_page_incognito_logo_size"
android:layout_height="@dimen/new_tab_page_incognito_logo_size"
Expand All @@ -54,7 +54,7 @@
app:layout_constraintBottom_toBottomOf="@+id/title"
app:layout_constraintStart_toStartOf="@id/guideline_begin"
app:layout_constraintEnd_toStartOf="@+id/title"
/>
/> -->

<TextView
android:id="@+id/title"
Expand All @@ -64,14 +64,13 @@
android:layout_height="wrap_content"
android:text="@string/brave_new_tab_private_header"
android:textColor="@color/brave_white"
android:layout_marginStart="14dp"
android:layout_marginBottom="64dp"
app:layout_constraintVertical_chainStyle="packed"
app:layout_constraintTop_toTopOf="parent"
app:layout_constraintBottom_toTopOf="@+id/tv_first_title"
app:layout_constraintStart_toEndOf="@+id/new_tab_incognito_icon"
app:layout_constraintEnd_toEndOf="@id/guideline_end"
/>
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintHorizontal_bias="0.5"
android:layout_marginBottom="24dp"
/>

<ImageView
android:id="@+id/iv_first_icon"
Expand Down Expand Up @@ -160,7 +159,7 @@
app:layout_constraintEnd_toEndOf="@id/tv_second_title"
/>

<ImageView
<!-- <ImageView
android:id="@+id/iv_third_icon"
app:layout_constraintHorizontal_chainStyle="packed"
android:layout_width="@dimen/new_tab_page_incognito_icon_size"
Expand All @@ -171,9 +170,9 @@
app:layout_constraintTop_toTopOf="@+id/tv_third_title"
app:layout_constraintStart_toStartOf="@+id/guideline_begin"
app:layout_constraintEnd_toStartOf="@+id/tv_third_title"
/>
/> -->

<TextView
<!-- <TextView
android:id="@+id/tv_third_title"
style="@style/DefaultSemibold"
app:layout_constraintWidth_max="460dp"
Expand All @@ -186,9 +185,9 @@
app:layout_constraintBottom_toTopOf="@+id/tv_third_description"
app:layout_constraintStart_toEndOf="@id/iv_third_icon"
app:layout_constraintEnd_toEndOf="@id/guideline_end"
/>
/> -->

<TextView
<!-- <TextView
android:id="@+id/tv_third_description"
style="@style/DefaultRegular"
android:layout_width="0dp"
Expand All @@ -200,9 +199,9 @@
app:layout_constraintBottom_toTopOf="@+id/tv_try_vpn"
app:layout_constraintStart_toStartOf="@id/tv_third_title"
app:layout_constraintEnd_toEndOf="@id/tv_third_title"
/>
/> -->

<TextView
<!-- <TextView
android:id="@+id/tv_try_vpn"
style="@style/DefaultSemibold"
android:layout_width="0dp"
Expand All @@ -213,7 +212,7 @@
app:layout_constraintBottom_toBottomOf="parent"
app:layout_constraintStart_toStartOf="@id/tv_third_title"
app:layout_constraintEnd_toEndOf="@id/tv_third_title"
/>
/> -->

</androidx.constraintlayout.widget.ConstraintLayout>

Expand Down
Loading
Loading