Make custom private page with DDG switch #3509

emerick · 2019-09-23T14:17:45Z

Fix brave/brave-browser#4824

Submitter Checklist:

Test Plan:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

emerick · 2019-10-07T20:07:55Z

patches/chrome-android-java-res-layout-new_tab_page_incognito.xml.patch

+            <TextView
+                android:id="@+id/ddg_offer_link"
+                android:layout_width="wrap_content"
+                android:layout_height="wrap_content"
+                android:gravity="center_vertical"
+                android:text="@string/ddg_offer_link"
+                android:textColor="@color/modern_grey_400"
+                android:drawableStart="@drawable/duckduckgo"
+                android:drawablePadding="15dp"
+                android:layout_marginTop="15dp"
+                android:textSize="28sp"
+                android:scaleX="0.5"
+                android:scaleY="0.5" />
+


Hoping there's a better approach I can take here to avoid such a large patch...

@emerick you can use include to include a layout inside a different layout https://github.com/brave/brave-core/blob/master/patches/chrome-android-java-res-layout-toolbar_phone.xml.patch#L17

Perfect, done.

SergeyZhukovsky · 2019-10-07T20:26:13Z

patches/chrome-android-java-res-layout-new_tab_page_incognito.xml.patch

+            <TextView
+                android:id="@+id/ddg_offer_link"
+                android:layout_width="wrap_content"
+                android:layout_height="wrap_content"
+                android:gravity="center_vertical"
+                android:text="@string/ddg_offer_link"
+                android:textColor="@color/modern_grey_400"
+                android:drawableStart="@drawable/duckduckgo"
+                android:drawablePadding="15dp"
+                android:layout_marginTop="15dp"
+                android:textSize="28sp"
+                android:scaleX="0.5"
+                android:scaleY="0.5" />
+


@emerick you can use include to include a layout inside a different layout https://github.com/brave/brave-core/blob/master/patches/chrome-android-java-res-layout-toolbar_phone.xml.patch#L17

SergeyZhukovsky · 2019-10-07T20:27:11Z

patches/chrome-android-java-res-values-v17-styles.xml.patch

         <item name="android:textAlignment">viewStart</item>
         <item name="android:textAppearance">@style/TextAppearance.BlackTitle1</item>
     </style>
 +    <style name="BraveSwitchTheme"> <item name="colorControlActivated">#fb542b</item> </style>
+    <style name="BraveDialogTheme" parent="Theme.AppCompat.Light.Dialog.Alert"> <item name="windowNoTitle">true</item> </style>


Just put that string to the end of previous string to make the patch one line. We don't care that it looks ugly, we want to have as less lines as we can in patches.

SergeyZhukovsky

++

simonhong

LGTM with BraveSearchEngineUtils usage :)

simonhong · 2019-10-10T04:47:57Z

patches/chrome-android-java-res-layout-new_tab_page_incognito.xml.patch

+                 android:textAppearance="@style/TextAppearance.IncognitoNewTabLearnMoreLinkModern"
+                 android:lineSpacingExtra="@dimen/md_incognito_ntp_line_spacing" />
+
+	    <include layout="@layout/brave_ddg_offer_link" android:layout_height="wrap_content" android:layout_width="match_parent" />


Curious we can do this by programatically instead of using layout xml. If so we can minimize patch?

Can we implement BraveIncognitoNewTabPageView as a super class of IncognitoNewTabPageView?
If we can, I think it can also reduce more patch files.

@simonhong It doesn't seem like a super class would work here, since I need to override a function. But maybe I'm misunderstanding?

IncognitoNewTabPageView onFinishInflate calls super.onFinishInflate so I think a superclass would work and would only be a single patch to IncognitoNewTabPageView to change the superclass

actually I think maybe we can simplify this by not overriding IncognitoNewTabPageView at all and just adding a view class here in the xml file for ddg

Yup, good idea - fixed.

bridiver · 2019-10-14T20:18:12Z

patches/chrome-android-java-res-values-styles.xml.patch

 --- a/chrome/android/java/res/values/styles.xml
 +++ b/chrome/android/java/res/values/styles.xml
 @@ -972,4 +972,5 @@
     <style name="DarkModeCompatibleVerticalScrolling">
         <item name="android:scrollbarThumbVertical">@color/default_scroll_thumb</item>
     </style>
-+    <style name="BraveSwitchTheme"> <item name="colorControlActivated">#fb542b</item> </style>
+    <style name="BraveSwitchTheme"> <item name="colorControlActivated">#fb542b</item> </style> <style name="BraveDialogTheme" parent="Theme.AppCompat.Light.Dialog.Alert"> <item name="windowNoTitle">true</item> </style>


continuing to add things here doesn't seem like the right approach to me. Where is this file loaded?

can we just create our own brave/android/java/res/values/styles.xml like we have for strings?

bridiver · 2019-10-14T20:22:37Z

patches/chrome-android-java-src-org-chromium-chrome-browser-ntp-IncognitoNewTabPage.java.patch

+     private Activity mActivity;
+
+     private String mTitle;
+-    protected IncognitoNewTabPageView mIncognitoNewTabPageView;


actually are these changes even necessary? BraveIncognitoNewTabPageView is a subclass of IncognitoNewTabPageView and inflate will create the correct class based on https://github.com/brave/brave-core/pull/3509/files#diff-ea88c09faf6ae0fc5fb51f009c05d03eR10

Yeah, you're right these aren't necessary at all - good catch!

emerick · 2019-10-15T20:44:33Z

@bridiver Ready for review again, thanks!

emerick · 2019-10-16T18:31:59Z

CI tests successful.

emerick self-assigned this Sep 23, 2019

emerick force-pushed the ddg-private-page-android branch 6 times, most recently from 7c10e06 to 74d5599 Compare September 27, 2019 14:08

emerick force-pushed the ddg-private-page-android branch 7 times, most recently from f75b9a0 to 56efdf3 Compare October 7, 2019 16:23

emerick marked this pull request as ready for review October 7, 2019 20:06

emerick requested a review from bridiver as a code owner October 7, 2019 20:06

emerick requested review from SergeyZhukovsky and simonhong October 7, 2019 20:06

emerick commented Oct 7, 2019

View reviewed changes

emerick force-pushed the ddg-private-page-android branch from 56efdf3 to 84abcc1 Compare October 7, 2019 20:08

SergeyZhukovsky requested changes Oct 7, 2019

View reviewed changes

emerick force-pushed the ddg-private-page-android branch 2 times, most recently from f523331 to a246b91 Compare October 8, 2019 15:12

emerick requested a review from SergeyZhukovsky October 8, 2019 15:28

SergeyZhukovsky previously approved these changes Oct 8, 2019

View reviewed changes

simonhong previously approved these changes Oct 8, 2019

View reviewed changes

simonhong reviewed Oct 10, 2019

View reviewed changes

emerick dismissed stale reviews from simonhong and SergeyZhukovsky via 3b19ab8 October 10, 2019 21:51

emerick force-pushed the ddg-private-page-android branch from a246b91 to 3b19ab8 Compare October 10, 2019 21:51

emerick force-pushed the ddg-private-page-android branch 2 times, most recently from 51eeb52 to 590ff65 Compare October 10, 2019 23:26

emerick added the CI/skip Do not run CI builds (except noplatform) label Oct 11, 2019

emerick force-pushed the ddg-private-page-android branch 2 times, most recently from 841ec78 to 1194f66 Compare October 11, 2019 18:15

bridiver reviewed Oct 14, 2019

View reviewed changes

emerick force-pushed the ddg-private-page-android branch 3 times, most recently from b11b9d5 to e2fce4b Compare October 15, 2019 20:42

emerick requested a review from bridiver October 15, 2019 20:44

emerick removed the CI/skip Do not run CI builds (except noplatform) label Oct 15, 2019

Make custom private page with DDG switch

4adecca

emerick force-pushed the ddg-private-page-android branch from e2fce4b to 4adecca Compare October 15, 2019 22:59

bridiver approved these changes Oct 16, 2019

View reviewed changes

emerick merged commit 5f9e4d4 into master Oct 16, 2019

emerick deleted the ddg-private-page-android branch October 16, 2019 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make custom private page with DDG switch #3509

Make custom private page with DDG switch #3509

emerick commented Sep 23, 2019 •

edited

Loading

emerick Oct 7, 2019

SergeyZhukovsky Oct 7, 2019

emerick Oct 8, 2019

SergeyZhukovsky Oct 7, 2019

SergeyZhukovsky Oct 7, 2019

emerick Oct 8, 2019

SergeyZhukovsky left a comment

simonhong left a comment

simonhong Oct 10, 2019

simonhong Oct 10, 2019

emerick Oct 11, 2019

bridiver Oct 14, 2019

bridiver Oct 14, 2019

emerick Oct 15, 2019

bridiver Oct 14, 2019

bridiver Oct 14, 2019

emerick Oct 15, 2019

bridiver Oct 14, 2019

emerick Oct 14, 2019

emerick commented Oct 15, 2019

emerick commented Oct 16, 2019

Make custom private page with DDG switch #3509

Make custom private page with DDG switch #3509

Conversation

emerick commented Sep 23, 2019 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeyZhukovsky left a comment

Choose a reason for hiding this comment

simonhong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emerick commented Oct 15, 2019

emerick commented Oct 16, 2019

emerick commented Sep 23, 2019 •

edited

Loading