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

Moved from tabs to core option to control opening pages in custom tabs #5199

Merged
merged 3 commits into from
Apr 10, 2020
Merged

Moved from tabs to core option to control opening pages in custom tabs #5199

merged 3 commits into from
Apr 10, 2020

Conversation

samartnik
Copy link
Contributor

@samartnik samartnik commented Apr 8, 2020 •
edited
Loading

Resolves brave/brave-browser#9074

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.

@samartnik samartnik added CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS labels Apr 8, 2020
@samartnik samartnik added this to the 1.9.x - Nightly milestone Apr 8, 2020
@samartnik samartnik requested a review from bridiver as a code owner April 8, 2020 19:47
@samartnik samartnik self-assigned this Apr 8, 2020
@samartnik samartnik force-pushed the android_use_custom_tabs_option branch from 468a5c1 to 49ba8dc Compare April 8, 2020 19:52
SergeyZhukovsky
SergeyZhukovsky approved these changes Apr 8, 2020
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.

++

yrliou
yrliou approved these changes Apr 8, 2020
View reviewed changes
Copy link
Member

@yrliou yrliou left a comment

Choose a reason for hiding this comment

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

LGTM

@samartnik samartnik force-pushed the android_use_custom_tabs_option branch from 49ba8dc to ce7395b Compare April 9, 2020 12:37
bridiver
bridiver reviewed Apr 9, 2020
View reviewed changes
patches/chrome-android-java-src-org-chromium-chrome-browser-LaunchIntentDispatcher.java.patch Outdated
* @return Whether the intent is for launching a Custom Tab.
*/
public static boolean isCustomTabIntent(Intent intent) {
+ if(!BraveLaunchIntentDispatcher.useCustomTabs()) 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.

There are a couple different ways we could do this with asm. Let's dm to discuss

bridiver
bridiver reviewed Apr 10, 2020
View reviewed changes
build/android/bytecode/java/org/brave/bytecode/BraveClassVisitor.java Outdated
@@ -147,6 +155,28 @@ protected void makePublicMethod(String className, String methodName) {
methods.add(methodName);
}

private String shouldChangeOwner(String owner, String methodName) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

since the semantics of this are slightly different than the making methods public, maybe call it maybeChangeOwner and return the original owner instead of an empty string if it doesn't change? Then you don't need this https://github.com/brave/brave-core/pull/5199/files#diff-125eb2dc484493790df1e4b6cca9fd3bR77 and you can add the println inside shouldChangeOwner. Just a suggestion though

bridiver
bridiver reviewed Apr 10, 2020
View reviewed changes
android/java/org/chromium/chrome/browser/BraveLaunchIntentDispatcher.java
import org.chromium.base.ContextUtils;
import org.chromium.chrome.browser.preferences.BravePreferenceKeys;

public class BraveLaunchIntentDispatcher {
Copy link
Collaborator

Choose a reason for hiding this comment

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

in other classes I left a comment // see BraveLaunchIntentDispatcherClassAdapter so it's easier to track down how this works

@samartnik samartnik merged commit 7833812 into master Apr 10, 2020
@samartnik samartnik deleted the android_use_custom_tabs_option branch April 10, 2020 18:03
This was referenced Apr 10, 2020
Merged
Merged
@srirambv
Copy link
Contributor

Verification passed on OnePlus 6T with Android 10 running 1.9.21 x64 Nightly build

  • Verified Open tabs in custom tab is shown under Controls
    image
  • Verified opening a link in custom tab works when the setting is enabled
  • Verified custom tabs are not loaded when the setting is turned off
  • Verified toggle setting doesn't require browser restart
  • Logged [Android] Remove custom tabs option for tablets brave-browser#9220 to remove setting for Tablets

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

@SergeyZhukovsky SergeyZhukovsky SergeyZhukovsky approved these changes

@yrliou yrliou yrliou approved these changes

@bridiver bridiver bridiver approved these changes

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

@simonhong simonhong Awaiting requested review from simonhong

Assignees

@samartnik samartnik

Labels
CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64
Projects
None yet
Milestone
1.9.x - Release
Development

Successfully merging this pull request may close these issues.

[Android] Open tabs in Custom Tabs option
5 participants
@samartnik @srirambv @bridiver @yrliou @SergeyZhukovsky