Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2786283002:
[Android] Add a transparent Activity in Chrome to handle Browser Action Intent (Closed)
Description[Android] Add a transparent Activity in Chrome to handle Browser Action Intent
Add a transparent Activity in Chrome to handle the Browser Action
Intent. This CL also deals with parsing the information of Browser
Action request from the Intent. Later, this Activity will open a
context menu from Chrome with the request information.
BUG=706696
Review-Url: https://codereview.chromium.org/2786283002
Cr-Commit-Position: refs/heads/master@{#467837}
Committed: https://chromium.googlesource.com/chromium/src/+/3e1e45d504e7f8d83b2fa9bf61de997ad79ec05f
Patch Set 1 #
Total comments: 8
Patch Set 2 : Update based on Yusuf's comments. #Patch Set 3 : Update based on the new changes from support library. #Patch Set 4 : Change package name of Browser Actions in Chrome #Patch Set 5 : Sync the support library changes. #Patch Set 6 : Update based on recent changes from support library. #Patch Set 7 : Sync DEPS to have support library change of Browser Actions. #
Total comments: 7
Patch Set 8 : Update based on Yusuf's comments. #Patch Set 9 : Update Yusuf's nit comment. #
Total comments: 12
Patch Set 10 : Update base on Ted's comments. #
Total comments: 19
Patch Set 11 : Update based on Ted's comments and add junit test for BrowserActionActivity. #
Total comments: 14
Patch Set 12 : Update based on Ted's new comments for JUnit test. #
Total comments: 10
Patch Set 13 : Update based on Ted's new comments. #Patch Set 14 : Roll out changes in CCT support library to DEPS. #Messages
Total messages: 31 (12 generated)
ltian@chromium.org changed reviewers: + tedchoc@chromium.org, yusufo@chromium.org
Could you also take a look of this CL to see whether it makes sense? Thanks!
https://codereview.chromium.org/2786283002/diff/1/chrome/android/java/Android... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2786283002/diff/1/chrome/android/java/Android... chrome/android/java/AndroidManifest.xml:668: <!-- Activities for browser action --> Lets use BrowserActions everywhere as the feature name (One word with s) https://codereview.chromium.org/2786283002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/browseraction/BrowserActionActivity.java (right): https://codereview.chromium.org/2786283002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseraction/BrowserActionActivity.java:32: /** Wether the native libraries have already loaded */ Whether https://codereview.chromium.org/2786283002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseraction/BrowserActionActivity.java:36: protected void setContentView() { if you are going to call beginLoadingLibrary, I am not sure why we need shouldDelayBrowserStartup. I thought you wanted to set the content here, or open the dialog first and then load the library. Maybe without the context menu opening bits, this is confusing? Where will the context menu opening related code will go here? https://codereview.chromium.org/2786283002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseraction/BrowserActionActivity.java:61: if (mIsNativeLoading || mIsNativeReady) return; not sure why we need these two booleans. setContentView always occurs before finishNativeInitialization. As long as we call this from setContentView only(or any other call but only once), we shouldn't need these.
https://codereview.chromium.org/2786283002/diff/1/chrome/android/java/Android... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2786283002/diff/1/chrome/android/java/Android... chrome/android/java/AndroidManifest.xml:668: <!-- Activities for browser action --> On 2017/03/31 23:43:12, Yusuf wrote: > Lets use BrowserActions everywhere as the feature name (One word with s) Done. https://codereview.chromium.org/2786283002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/browseraction/BrowserActionActivity.java (right): https://codereview.chromium.org/2786283002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseraction/BrowserActionActivity.java:32: /** Wether the native libraries have already loaded */ On 2017/03/31 23:43:12, Yusuf wrote: > Whether Done. https://codereview.chromium.org/2786283002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseraction/BrowserActionActivity.java:36: protected void setContentView() { On 2017/03/31 23:43:12, Yusuf wrote: > if you are going to call beginLoadingLibrary, I am not sure why we need > shouldDelayBrowserStartup. > > I thought you wanted to set the content here, or open the dialog first and then > load the library. > > Maybe without the context menu opening bits, this is confusing? Where will the > context menu opening related code will go here? New plan is here it should try to open a dialog and then listen to the callback of Dialog's onShowListener and then loads the library. Does it make more sense? https://codereview.chromium.org/2786283002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseraction/BrowserActionActivity.java:61: if (mIsNativeLoading || mIsNativeReady) return; On 2017/03/31 23:43:12, Yusuf wrote: > not sure why we need these two booleans. setContentView always occurs before > finishNativeInitialization. As long as we call this from setContentView only(or > any other call but only once), we shouldn't need these. Done.
https://codereview.chromium.org/2786283002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2786283002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:51: ChromeBrowserInitializer.getInstance(getApplicationContext()).handlePreNativeStartup(this); I think this might be a protected API now, pls rebase and check if this exists. https://codereview.chromium.org/2786283002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:60: public boolean shouldStartGpuProcess() { is this needed? https://codereview.chromium.org/2786283002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:104: private void onMenuShow() { onMenuShown who calls this?
https://codereview.chromium.org/2786283002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2786283002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:51: ChromeBrowserInitializer.getInstance(getApplicationContext()).handlePreNativeStartup(this); On 2017/04/21 19:59:29, Yusuf wrote: > I think this might be a protected API now, pls rebase and check if this exists. Done. https://codereview.chromium.org/2786283002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:60: public boolean shouldStartGpuProcess() { On 2017/04/21 19:59:29, Yusuf wrote: > is this needed? Yes,the function in AsyncInitializationActivity is abstract. https://codereview.chromium.org/2786283002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:104: private void onMenuShow() { On 2017/04/21 19:59:29, Yusuf wrote: > onMenuShown > > who calls this? It is a callback function called when the dialog is shown. In the next CL, it is called by a Runnable in Browser Actions' context menu helper.
lgtm with a single nit. https://codereview.chromium.org/2786283002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2786283002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:104: private void onMenuShow() { On 2017/04/21 21:36:05, ltian wrote: > On 2017/04/21 19:59:29, Yusuf wrote: > > onMenuShown > > > > who calls this? > > It is a callback function called when the dialog is shown. In the next CL, it is > called by a Runnable in Browser Actions' context menu helper. Then lets just rename it to onMenuShown.
The CQ bit was checked by ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/2786283002/#ps160001 (title: "Update Yusuf's nit comment.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2786283002/diff/160001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2786283002/diff/160001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:610: android:theme="@style/FullscreenTransparentActivityTheme" any reason we don't use @android:style/Theme.Translucent.NoTitleBar here? https://codereview.chromium.org/2786283002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2786283002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:32: if (getIntent() != null I think this block of code should probably go into: isStartedUpCorrectly(Intent intent) that is called by the super. We also should make sure that intent wasn't started with NEW_TASK or NEW_DOCUMENT as that doesn't make sense. https://codereview.chromium.org/2786283002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:38: super.onDestroy(); This doesn't actually do anything. https://codereview.chromium.org/2786283002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:52: ChromeBrowserInitializer.getInstance(getApplicationContext()).handlePreNativeStartup(this); I think this is what the super class does. Can we just delete this method? https://codereview.chromium.org/2786283002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:70: mUri = intent.getData(); Should we use IntentHandler#getUrlFromIntent? https://codereview.chromium.org/2786283002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:98: } should we consider logging something in an else like: if (title != null) { Log.e(TAG, "Missing action for item: " + i); } else if (action != null) { Log.e(TAG, "Missing title for item: " + i); } Something to help developers realize why their custom items aren't showing up
https://codereview.chromium.org/2786283002/diff/160001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2786283002/diff/160001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:610: android:theme="@style/FullscreenTransparentActivityTheme" On 2017/04/24 20:23:17, Ted C wrote: > any reason we don't use @android:style/Theme.Translucent.NoTitleBar here? The BrowserActionActivity extends AsyncInitializationActivity which needs an AppCompat theme and I cannot find a transparent + no title bar theme from AppCompat theme. The FullscreenTransparentActivityTheme extends Theme.AppCompat.Light. https://codereview.chromium.org/2786283002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2786283002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:32: if (getIntent() != null On 2017/04/24 20:23:17, Ted C wrote: > I think this block of code should probably go into: > > isStartedUpCorrectly(Intent intent) that is called by the super. > > We also should make sure that intent wasn't started with NEW_TASK or > NEW_DOCUMENT as that doesn't make sense. Done. https://codereview.chromium.org/2786283002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:38: super.onDestroy(); On 2017/04/24 20:23:17, Ted C wrote: > This doesn't actually do anything. Done. https://codereview.chromium.org/2786283002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:52: ChromeBrowserInitializer.getInstance(getApplicationContext()).handlePreNativeStartup(this); On 2017/04/24 20:23:17, Ted C wrote: > I think this is what the super class does. Can we just delete this method? Done. https://codereview.chromium.org/2786283002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:70: mUri = intent.getData(); On 2017/04/24 20:23:17, Ted C wrote: > Should we use IntentHandler#getUrlFromIntent? Done. https://codereview.chromium.org/2786283002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:98: } On 2017/04/24 20:23:17, Ted C wrote: > should we consider logging something in an else like: > > if (title != null) { > Log.e(TAG, "Missing action for item: " + i); > } else if (action != null) { > Log.e(TAG, "Missing title for item: " + i); > } > > Something to help developers realize why their custom items aren't showing up Done.
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/re... File chrome/android/java/res/values-v17/styles.xml (right): https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/re... chrome/android/java/res/values-v17/styles.xml:62: <style name="FullscreenTransparentActivityTheme" parent="Theme.AppCompat.Light" > Ahh...gotcha that this needs to extend from AppCompat. I forgot about that restriction. Thoughts about extending from: Theme.AppCompat.Light.NoActionBar Then I think we wouldn't need the windowActionBar param. https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/re... chrome/android/java/res/values-v17/styles.xml:65: <item name="android:windowIsFloating">true</item> do we need this? https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/re... chrome/android/java/res/values-v17/styles.xml:69: </style> Looking at the android source: https://android.googlesource.com/platform/frameworks/base/+/master/core/res/r... Should we add: <item name="colorBackgroundCacheHint">@null</item> <item name="windowIsTranslucent">true</item> Then we should have an app compat equivalent of the android version. https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:28: private int mType; nit, I would put a blank link above this. https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:31: private ArrayList<BrowserActionItem> mActions = new ArrayList<>(); let's make the left side a generic List<> https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:40: protected boolean isStartedUpCorrectly(Intent intent) { This is pretty nicely unit testable. Can we add some tests with this change (ideally junit) that can verify this behavior. Sets us up to add more tests easily down the road. https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:42: && !BrowserActionsIntent.ACTION_BROWSER_ACTIONS_OPEN.equals( Should this be || ? https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:43: getIntent().getAction())) { should we be using the intent param vs getIntent() here? https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:55: if (mUri == null || mCreatorPackageName == null I would put each of these conditions on a separate line and add logging like below that would tell developers what happened and what was wrong.
https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/re... File chrome/android/java/res/values-v17/styles.xml (right): https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/re... chrome/android/java/res/values-v17/styles.xml:62: <style name="FullscreenTransparentActivityTheme" parent="Theme.AppCompat.Light" > On 2017/04/25 17:30:33, Ted C wrote: > Ahh...gotcha that this needs to extend from AppCompat. I forgot about that > restriction. > > Thoughts about extending from: > Theme.AppCompat.Light.NoActionBar > > Then I think we wouldn't need the windowActionBar param. Done. https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/re... chrome/android/java/res/values-v17/styles.xml:65: <item name="android:windowIsFloating">true</item> On 2017/04/25 17:30:33, Ted C wrote: > do we need this? Yes, if we remove this, only the small row above the title of app will be overlapped by a transparent activity. https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/re... chrome/android/java/res/values-v17/styles.xml:69: </style> On 2017/04/25 17:30:33, Ted C wrote: > Looking at the android source: > https://android.googlesource.com/platform/frameworks/base/+/master/core/res/r... > > Should we add: > <item name="colorBackgroundCacheHint">@null</item> > <item name="windowIsTranslucent">true</item> > > Then we should have an app compat equivalent of the android version. Done. https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:28: private int mType; On 2017/04/25 17:30:33, Ted C wrote: > nit, I would put a blank link above this. Done. https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:31: private ArrayList<BrowserActionItem> mActions = new ArrayList<>(); On 2017/04/25 17:30:33, Ted C wrote: > let's make the left side a generic List<> Done. https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:40: protected boolean isStartedUpCorrectly(Intent intent) { On 2017/04/25 17:30:33, Ted C wrote: > This is pretty nicely unit testable. Can we add some tests with this change > (ideally junit) that can verify this behavior. Sets us up to add more tests > easily down the road. Done. https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:42: && !BrowserActionsIntent.ACTION_BROWSER_ACTIONS_OPEN.equals( On 2017/04/25 17:30:33, Ted C wrote: > Should this be || ? Oh, sorry, it is ||. Thanks for pointing out. https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:43: getIntent().getAction())) { On 2017/04/25 17:30:33, Ted C wrote: > should we be using the intent param vs getIntent() here? Done. https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:55: if (mUri == null || mCreatorPackageName == null On 2017/04/25 17:30:33, Ted C wrote: > I would put each of these conditions on a separate line and add logging like > below that would tell developers what happened and what was wrong. Done.
yay for tests! one last thing I realized was around validating the URLs we get. https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/re... File chrome/android/java/res/values-v17/styles.xml (right): https://codereview.chromium.org/2786283002/diff/180001/chrome/android/java/re... chrome/android/java/res/values-v17/styles.xml:65: <item name="android:windowIsFloating">true</item> On 2017/04/26 18:43:01, ltian wrote: > On 2017/04/25 17:30:33, Ted C wrote: > > do we need this? > > Yes, if we remove this, only the small row above the title of app will be > overlapped by a transparent activity. Acknowledged. https://codereview.chromium.org/2786283002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2786283002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:43: @VisibleForTesting since you are accessing this from the same package, you don't need this. VisibleForTesting is used to indicate that you broadened the visibility, but here it can remain protected and still be used by the tests. https://codereview.chromium.org/2786283002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:49: mUri = Uri.parse(IntentHandler.getUrlFromIntent(intent)); should we verify the scheme of the URL is either https or http? While our intent filter restricts to that, that is for implicit intents. We should also double check here to prevent people sending chrome:// or content:// In which case, we should also add a test. https://codereview.chromium.org/2786283002/diff/200001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java (right): https://codereview.chromium.org/2786283002/diff/200001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:39: private Intent mIntent; since this is only used in the one method, I would just declare it than instead of as a class variable https://codereview.chromium.org/2786283002/diff/200001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:48: doAnswer(new Answer() { I would pull out the Answer declaration to fix the indenting below as it is quite weird. It is a bug with clang format IMO and the best way to work around it is to not inline declare the class. https://codereview.chromium.org/2786283002/diff/200001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:66: assertEquals(true, mActivity.isStartedUpCorrectly(mIntent)); I'd put a space below this. https://codereview.chromium.org/2786283002/diff/200001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:68: assertEquals(false, mActivity.isStartedUpCorrectly(mIntent)); space below this https://codereview.chromium.org/2786283002/diff/200001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:70: new BrowserActionsIntent.Builder(mContext, Uri.parse(TEST_URL)).build().getIntent(); You are rebuilding the intent here and not directly above. I actually prefer being more verbose and I would do the same for NEW_TASK above. You could write a simple function that makes the intent construction a bit less verbose (like createBaseBrowserIntent(String url))
https://codereview.chromium.org/2786283002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2786283002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:43: @VisibleForTesting On 2017/04/26 20:50:03, Ted C wrote: > since you are accessing this from the same package, you don't need this. > VisibleForTesting is used to indicate that you broadened the visibility, but > here it can remain protected and still be used by the tests. Done. https://codereview.chromium.org/2786283002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:49: mUri = Uri.parse(IntentHandler.getUrlFromIntent(intent)); On 2017/04/26 20:50:02, Ted C wrote: > should we verify the scheme of the URL is either https or http? > > While our intent filter restricts to that, that is for implicit intents. We > should also double check here to prevent people sending chrome:// or content:// > > In which case, we should also add a test. Done. https://codereview.chromium.org/2786283002/diff/200001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java (right): https://codereview.chromium.org/2786283002/diff/200001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:39: private Intent mIntent; On 2017/04/26 20:50:03, Ted C wrote: > since this is only used in the one method, I would just declare it than instead > of as a class variable Done. https://codereview.chromium.org/2786283002/diff/200001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:48: doAnswer(new Answer() { On 2017/04/26 20:50:03, Ted C wrote: > I would pull out the Answer declaration to fix the indenting below as it is > quite weird. It is a bug with clang format IMO and the best way to work around > it is to not inline declare the class. Done. https://codereview.chromium.org/2786283002/diff/200001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:66: assertEquals(true, mActivity.isStartedUpCorrectly(mIntent)); On 2017/04/26 20:50:03, Ted C wrote: > I'd put a space below this. Done. https://codereview.chromium.org/2786283002/diff/200001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:68: assertEquals(false, mActivity.isStartedUpCorrectly(mIntent)); On 2017/04/26 20:50:03, Ted C wrote: > space below this Done. https://codereview.chromium.org/2786283002/diff/200001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:70: new BrowserActionsIntent.Builder(mContext, Uri.parse(TEST_URL)).build().getIntent(); On 2017/04/26 20:50:03, Ted C wrote: > You are rebuilding the intent here and not directly above. I actually prefer > being more verbose and I would do the same for NEW_TASK above. > > You could write a simple function that makes the intent construction a bit less > verbose (like createBaseBrowserIntent(String url)) Done.
https://codereview.chromium.org/2786283002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2786283002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:60: } else if (!mUri.getScheme().equals(UrlConstants.HTTP_SCHEME) getScheme() in theory can return null, it would be better to reverse the ordering here to avoid an NPE possibility UrlConstants.HTTP_SCHEME.equals(mUri.getScheme()) https://codereview.chromium.org/2786283002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:62: return false; add log here too https://codereview.chromium.org/2786283002/diff/220001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java (right): https://codereview.chromium.org/2786283002/diff/220001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:56: doAnswer(answer).when(mPendingIntent).getCreatorPackage(); I just realized that you don't actually need to use an answer here most likely. I think the following will work: doReturn("some.other.app.package.name").when(mPendingIntent).getCreatorPackage(); Answers are for when you need to interact with the input params or conditionally return something. Here I don't think you need either. https://codereview.chromium.org/2786283002/diff/220001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:62: assertEquals(false, mActivity.isStartedUpCorrectly(null)); replace assertEquals(false/true: assertTrue/assertFalse https://codereview.chromium.org/2786283002/diff/220001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:65: Intent mIntent = createBaseBrowserActionsIntent(HTTP_SCHEME_TEST_URL); I think we should add another test like: mIntent = createBaseBrowserActionsIntent(HTTP_SCHEME_TEST_URL); mIntent.removeExtra(BrowserActionsIntent.EXTRA_APP_ID); assertFalse(mActivity.isStartedUpCorrectly(mIntent));
https://codereview.chromium.org/2786283002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2786283002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:60: } else if (!mUri.getScheme().equals(UrlConstants.HTTP_SCHEME) On 2017/04/26 23:50:56, Ted C wrote: > getScheme() in theory can return null, it would be better to reverse the > ordering here to avoid an NPE possibility > > UrlConstants.HTTP_SCHEME.equals(mUri.getScheme()) Done. https://codereview.chromium.org/2786283002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:62: return false; On 2017/04/26 23:50:56, Ted C wrote: > add log here too Done. https://codereview.chromium.org/2786283002/diff/220001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java (right): https://codereview.chromium.org/2786283002/diff/220001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:56: doAnswer(answer).when(mPendingIntent).getCreatorPackage(); On 2017/04/26 23:50:56, Ted C wrote: > I just realized that you don't actually need to use an answer here most likely. > > I think the following will work: > doReturn("some.other.app.package.name").when(mPendingIntent).getCreatorPackage(); > > Answers are for when you need to interact with the input params or conditionally > return something. Here I don't think you need either. It seems not work because doAnswer() only accepts Answer object as parameter. https://codereview.chromium.org/2786283002/diff/220001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:62: assertEquals(false, mActivity.isStartedUpCorrectly(null)); On 2017/04/26 23:50:56, Ted C wrote: > replace assertEquals(false/true: > > assertTrue/assertFalse Done. https://codereview.chromium.org/2786283002/diff/220001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:65: Intent mIntent = createBaseBrowserActionsIntent(HTTP_SCHEME_TEST_URL); On 2017/04/26 23:50:56, Ted C wrote: > I think we should add another test like: > > mIntent = createBaseBrowserActionsIntent(HTTP_SCHEME_TEST_URL); > mIntent.removeExtra(BrowserActionsIntent.EXTRA_APP_ID); > assertFalse(mActivity.isStartedUpCorrectly(mIntent)); Done.
lgtm
The CQ bit was checked by ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2786283002/#ps260001 (title: "Roll out changes in CCT support library to DEPS.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1493332398198400,
"parent_rev": "ca74aee3773bb77867cf02d5f81725d10b33e4ab", "commit_rev":
"3e1e45d504e7f8d83b2fa9bf61de997ad79ec05f"}
Message was sent while issue was closed.
Description was changed from ========== [Android] Add a transparent Activity in Chrome to handle Browser Action Intent Add a transparent Activity in Chrome to handle the Browser Action Intent. This CL also deals with parsing the information of Browser Action request from the Intent. Later, this Activity will open a context menu from Chrome with the request information. BUG=706696 ========== to ========== [Android] Add a transparent Activity in Chrome to handle Browser Action Intent Add a transparent Activity in Chrome to handle the Browser Action Intent. This CL also deals with parsing the information of Browser Action request from the Intent. Later, this Activity will open a context menu from Chrome with the request information. BUG=706696 Review-Url: https://codereview.chromium.org/2786283002 Cr-Commit-Position: refs/heads/master@{#467837} Committed: https://chromium.googlesource.com/chromium/src/+/3e1e45d504e7f8d83b2fa9bf61de... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/3e1e45d504e7f8d83b2fa9bf61de... |
