Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2931433004:
[Android] Open a tab in the background from Browser Actions if ChromeTabbedActivity is available (Closed)
Description[Android] Open a tab in the background from Browser Actions if ChromeTabbedActivity is available
Implements the function that opens a tab for the given url in the
background of Chrome from Browser Actions menu. Also add a new
TabLaunchType (Browser Actions) which does not show transition and
always add to the top of the tab list. Besides, postpones operation
by showing a ProgressDialog to block UI until native libraries are
loaded.
BUG=730305
Review-Url: https://codereview.chromium.org/2931433004
Cr-Commit-Position: refs/heads/master@{#494316}
Committed: https://chromium.googlesource.com/chromium/src/+/fc32a59653e65cee968472d8c91fa060fdf981d9
Patch Set 1 #
Total comments: 16
Patch Set 2 : Update based on Yusuf's comment.s #
Total comments: 11
Patch Set 3 : Add Instrumentation Tests. #
Total comments: 22
Patch Set 4 : Update based on Yusuf's comments. #Patch Set 5 : Update test. #
Total comments: 16
Patch Set 6 : Update based on Yusuf's comments. #
Total comments: 6
Patch Set 7 : Update based on Yusuf's comments. #
Total comments: 6
Patch Set 8 : Update based on Yusuf's comments. #
Total comments: 32
Patch Set 9 : Upddate based on Teds' comments. #
Total comments: 2
Patch Set 10 : Update based on Ted's comments. #Patch Set 11 : Fix compile error. #Patch Set 12 : Fix test failures. #Patch Set 13 : Fix test failures. #Dependent Patchsets: Messages
Total messages: 44 (14 generated)
ltian@chromium.org changed reviewers: + tedchoc@chromium.org, yusufo@chromium.org
Can you take a look of the changes in this CL? Thanks!
We will definitely need tests for this one. Otherwise with the follow up CL, things will start getting out of hand. https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:8: import android.app.ProgressDialog; I can see this being used in other files as well, so it might be OK, but I wonder if we have a Chromium widget that is UI specced for these use cases. tedchoc@ or twellington@ would know better https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:42: private boolean mIsNatvieInitialized; Native https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:179: if (mProgressDialog != null && mProgressDialog.isShowing()) { I would say, when you find yourself adding this much view related logic to Activity and calling them outside Activity, then that is a good sign of Activity not being a good place for them. As long as we have an idea about Native being initialized, you shouldn't need anything else from the Activity. https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:186: * @return Whether Chrome native libraries are loaded. LibraryLoader.isLoaded does the above. finishNativeInitialization means All application and activity specific native initialization has been completed. https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:83: mActivity.finish(); one line https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:135: mDelegate.onOpenInBackground(mCurrentContextMenuParams.getLinkUrl()); maybe this is all the delegate's job? https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:66: public static final String EXTRA_ACTIVE_TAB_ID = I see this getting confused with IntentHandler.EXTRA_TAB_ID a lot. Can't we use that one in this context? https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:133: if (tab != null) { one line
https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:8: import android.app.ProgressDialog; On 2017/06/29 19:05:26, Yusuf wrote: > I can see this being used in other files as well, so it might be OK, but I > wonder if we have a Chromium widget that is UI specced for these use cases. > > tedchoc@ or twellington@ would know better I check there seems no Chromium widget for ProgressDialog. https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:42: private boolean mIsNatvieInitialized; On 2017/06/29 19:05:26, Yusuf wrote: > Native Done. https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:179: if (mProgressDialog != null && mProgressDialog.isShowing()) { On 2017/06/29 19:05:26, Yusuf wrote: > I would say, when you find yourself adding this much view related logic to > Activity and calling them outside Activity, then that is a good sign of Activity > not being a good place for them. > > As long as we have an idea about Native being initialized, you shouldn't need > anything else from the Activity. Put all Progress Dialog related code to helper class. https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:186: * @return Whether Chrome native libraries are loaded. On 2017/06/29 19:05:26, Yusuf wrote: > LibraryLoader.isLoaded does the above. > > finishNativeInitialization means All application and activity specific native > initialization has been completed. Done. https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:83: mActivity.finish(); On 2017/06/29 19:05:26, Yusuf wrote: > one line The "git cl format" forces this to be into multiple lines. https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:135: mDelegate.onOpenInBackground(mCurrentContextMenuParams.getLinkUrl()); On 2017/06/29 19:05:26, Yusuf wrote: > maybe this is all the delegate's job? I feel the delegate is only responsible for executing each task and the helper class takes charge of scheduling everything. So maybe moving all ProgressDialog related code to this class is better? https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:66: public static final String EXTRA_ACTIVE_TAB_ID = On 2017/06/29 19:05:26, Yusuf wrote: > I see this getting confused with IntentHandler.EXTRA_TAB_ID a lot. Can't we use > that one in this context? I am not quite sure whether they are the same. This one is used when ChromeTabbedActivity is not available, so the Notification will force tab merging and it indicates which tab should be the active tab. The IntentHandler.EXTRA_TAB_ID says it is used when creating a new Tab but does not mention whether the tab will be set as the active one. But I think it is not used for this CL, so I might just remove it for now. https://codereview.chromium.org/2931433004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:133: if (tab != null) { On 2017/06/29 19:05:26, Yusuf wrote: > one line Also, the "git cl format" forces this to be into multiple lines.
Carrying my below comment over in case it got unnoticed. "We will definitely need tests for this one. Otherwise with the follow up CL, things will start getting out of hand." https://codereview.chromium.org/2931433004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2931433004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:177: public boolean isNativeInitialized() { We actually dont even need this, since we already call the helper when native is initialized right? So the helper itself can keep that state and set a boolean to true only after the onNativeInitialized call. That was the only thing needed from here becomes calling helper once after finishNativeInitialization. https://codereview.chromium.org/2931433004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2931433004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:67: public BrowserActionsContextMenuHelper(BrowserActionActivity activity, ContextMenuParams params, Looking at how we are using the Activity here, if we had a public interface defined here as a MenuShownListener and moved onMenuShown there, then from the Activity side we can still pass it in both params, but here we can just set the activity to be a plain Activity instead of BrowserActionsActivity. https://codereview.chromium.org/2931433004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:218: public void handlePendingActions() { onNativeInitialized https://codereview.chromium.org/2931433004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2931433004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:112: boolean forMultiUrls = hasBrowserActionsNotification(); let's say MultipleUrls here, this reads as if it is a new type of url. https://codereview.chromium.org/2931433004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:123: return mSharedPreferences.getBoolean(PREF_HAS_BROWSER_ACTIONS_NOTIFICATION, false); From the notification creation, I now see that this is not necessarily a persistent notification. How we know at this point that the user has not dismissed the initial one somehow?
https://codereview.chromium.org/2931433004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2931433004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:177: public boolean isNativeInitialized() { On 2017/07/10 06:14:56, Yusuf wrote: > We actually dont even need this, since we already call the helper when native is > initialized right? So the helper itself can keep that state and set a boolean to > true only after the onNativeInitialized call. That was the only thing needed > from here becomes calling helper once after finishNativeInitialization. Done. https://codereview.chromium.org/2931433004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2931433004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:67: public BrowserActionsContextMenuHelper(BrowserActionActivity activity, ContextMenuParams params, On 2017/07/10 06:14:56, Yusuf wrote: > Looking at how we are using the Activity here, if we had a public interface > defined here as a MenuShownListener and moved onMenuShown there, then from the > Activity side we can still pass it in both params, but here we can just set the > activity to be a plain Activity instead of BrowserActionsActivity. Done. https://codereview.chromium.org/2931433004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:218: public void handlePendingActions() { On 2017/07/10 06:14:56, Yusuf wrote: > onNativeInitialized Done. https://codereview.chromium.org/2931433004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2931433004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:112: boolean forMultiUrls = hasBrowserActionsNotification(); On 2017/07/10 06:14:56, Yusuf wrote: > let's say MultipleUrls here, this reads as if it is a new type of url. Done. https://codereview.chromium.org/2931433004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:123: return mSharedPreferences.getBoolean(PREF_HAS_BROWSER_ACTIONS_NOTIFICATION, false); On 2017/07/10 06:14:56, Yusuf wrote: > From the notification creation, I now see that this is not necessarily a > persistent notification. How we know at this point that the user has not > dismissed the initial one somehow? For current implementation, we don't have a way to detect the notification is dismissed by users. But I think we can set a deleteIntent which will be launched when notification is dismissed. But I am not sure what we should do when users dismiss notifications. We can also prevent the notification being dismissed by calling setOngoing(true).
https://codereview.chromium.org/2931433004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2931433004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:123: return mSharedPreferences.getBoolean(PREF_HAS_BROWSER_ACTIONS_NOTIFICATION, false); On 2017/07/26 01:22:12, ltian wrote: > On 2017/07/10 06:14:56, Yusuf wrote: > > From the notification creation, I now see that this is not necessarily a > > persistent notification. How we know at this point that the user has not > > dismissed the initial one somehow? > > For current implementation, we don't have a way to detect the notification is > dismissed by users. But I think we can set a deleteIntent which will be launched > when notification is dismissed. But I am not sure what we should do when users > dismiss notifications. > > We can also prevent the notification being dismissed by calling > setOngoing(true). Yes, my intent was to point out that possibility. Before landing this lets figure out whether that notification should be permanent or not. If so, lets call setOngoing. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2931433004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:100: } else if (!IntentUtils.safeGetBooleanExtra(intent, EXTRA_SKIP_NEW_TASK, false) as long as you are not checking for something that only Chrome can generate, then this may be used for outside clients to get around NEW_TASK related bits. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2931433004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:81: public interface MenuShownListener { public void onMenuShown(); } no need for a one line here. It can be on its own line. Also no need to say public for the method, all interface methods will be public. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java (right): https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:60: private static class TestDelegate extends BrowserActionActivityDelegate { An overall comment on accessing various things here: I see that we mostly need some information from the helper (not the activity itself). So maybe the helper needs a test delegate itself? So instead of talking with the activity and routing things over from the helper. You only have a getHelperForTesting on the Activity. And then you call getContextMenuItems and also getCustomItemActions etc on the helper itself without adding anything new. Then you add the testdelegate class to the helper and that can also tell you about menu shown and native init now. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:133: @SmallTest I would test a single url being opened and multiple urls being opened in separate tests. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:134: public void testOpenTabInBackgroundWhenChromeAvaialalble() throws Exception { Available https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:136: mActivityTestRule.startMainActivityWithURL(TEST_URL); I would add two more URLs and test with more URLs. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:145: // Open 2 tab with TabLaunchType.FROM_BROWSER_ACTIONS type. two tabs https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:149: return mActivityTestRule.getActivity().getCurrentTabCreator().createNewTab( I think it is OK that this is a simulation and not an actual click, but it should test more of the actual pipeline than the eventual call I think. Specifically find a way to call onItemSelected(int itemId) with the right item id in the helper. Either through the test delegate or something else so that you can better simulate these with more coverage. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:163: Assert.assertEquals(tab2, mActivityTestRule.getActivity().getCurrentTabModel().getTabAt(1)); check the notification as well and whether it switches from using single tab string to multiple tabs string. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:164: Assert.assertEquals(tab3, mActivityTestRule.getActivity().getCurrentTabModel().getTabAt(2)); you can keep going here. keep the tabbed activity and its task and try bringing it to front. check if we are showing overview mode. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:210: intent.putExtra(BrowserActionActivity.EXTRA_SKIP_NEW_TASK, true); See my comment on the Activity about this. Maybe all you want is to check the creatorpackage and be OK with NEW_TASK if it is us?
https://codereview.chromium.org/2931433004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2931433004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:100: } else if (!IntentUtils.safeGetBooleanExtra(intent, EXTRA_SKIP_NEW_TASK, false) On 2017/07/26 18:35:39, Yusuf wrote: > as long as you are not checking for something that only Chrome can generate, > then this may be used for outside clients to get around NEW_TASK related bits. Done. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2931433004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:81: public interface MenuShownListener { public void onMenuShown(); } On 2017/07/26 18:35:39, Yusuf wrote: > no need for a one line here. It can be on its own line. Also no need to say > public for the method, all interface methods will be public. Done. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java (right): https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:60: private static class TestDelegate extends BrowserActionActivityDelegate { On 2017/07/26 18:35:39, Yusuf wrote: > An overall comment on accessing various things here: > > I see that we mostly need some information from the helper (not the activity > itself). So maybe the helper needs a test delegate itself? > > So instead of talking with the activity and routing things over from the helper. > You only have a getHelperForTesting on the Activity. And then you call > getContextMenuItems and also getCustomItemActions etc on the helper itself > without adding anything new. > > Then you add the testdelegate class to the helper and that can also tell you > about menu shown and native init now. Done. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:133: @SmallTest On 2017/07/26 18:35:39, Yusuf wrote: > I would test a single url being opened and multiple urls being opened in > separate tests. Done. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:134: public void testOpenTabInBackgroundWhenChromeAvaialalble() throws Exception { On 2017/07/26 18:35:39, Yusuf wrote: > Available Done. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:136: mActivityTestRule.startMainActivityWithURL(TEST_URL); On 2017/07/26 18:35:39, Yusuf wrote: > I would add two more URLs and test with more URLs. Done. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:145: // Open 2 tab with TabLaunchType.FROM_BROWSER_ACTIONS type. On 2017/07/26 18:35:39, Yusuf wrote: > two tabs Done. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:149: return mActivityTestRule.getActivity().getCurrentTabCreator().createNewTab( On 2017/07/26 18:35:39, Yusuf wrote: > I think it is OK that this is a simulation and not an actual click, but it > should test more of the actual pipeline than the eventual call I think. > Specifically find a way to call onItemSelected(int itemId) with the right item > id in the helper. Either through the test delegate or something else so that you > can better simulate these with more coverage. Done. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:163: Assert.assertEquals(tab2, mActivityTestRule.getActivity().getCurrentTabModel().getTabAt(1)); On 2017/07/26 18:35:39, Yusuf wrote: > check the notification as well and whether it switches from using single tab > string to multiple tabs string. Done. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:164: Assert.assertEquals(tab3, mActivityTestRule.getActivity().getCurrentTabModel().getTabAt(2)); On 2017/07/26 18:35:39, Yusuf wrote: > you can keep going here. > > keep the tabbed activity and its task and try bringing it to front. check if we > are showing overview mode. Done. https://codereview.chromium.org/2931433004/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:210: intent.putExtra(BrowserActionActivity.EXTRA_SKIP_NEW_TASK, true); On 2017/07/26 18:35:39, Yusuf wrote: > See my comment on the Activity about this. Maybe all you want is to check the > creatorpackage and be OK with NEW_TASK if it is us? Done.
I think this is going to be our final round. Thanks! https://codereview.chromium.org/2931433004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2931433004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:52: private static BrowserActionActivityDelegate sDelegate; why do we need this to be a static? https://codereview.chromium.org/2931433004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:126: @VisibleForTesting javadoc https://codereview.chromium.org/2931433004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2931433004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:73: @VisibleForTesting lets keep these as private but add getters for testing here. https://codereview.chromium.org/2931433004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:87: void onMenuShown(); javadoc https://codereview.chromium.org/2931433004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:173: @VisibleForTesting do you need to add this? @VisibleForTesting should only be needed to make sure proguard doesnt strip calls that are only used by tests. This one is used, so should not be stripped already? https://codereview.chromium.org/2931433004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2931433004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:123: @VisibleForTesting ditto my comment on VisibleForTesting. if it is not needed, lets not add this annotation. https://codereview.chromium.org/2931433004/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java (right): https://codereview.chromium.org/2931433004/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:63: public final CallbackHelper onBrowserActionsMenuShownCallback = new CallbackHelper(); mVariableName on all these. And feel free to move these to the test class, make this class a private class (missing static), so that you wont have to do mdelegate.mcallbackhelper calls. https://codereview.chromium.org/2931433004/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:181: @SmallTest You might need to run some of these on phones only.
https://codereview.chromium.org/2931433004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2931433004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:52: private static BrowserActionActivityDelegate sDelegate; On 2017/08/08 15:39:50, Yusuf wrote: > why do we need this to be a static? Because we have some static test functions (getActivityDelegate() and setDelegateForTests()) accessing this field. https://codereview.chromium.org/2931433004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:126: @VisibleForTesting On 2017/08/08 15:39:50, Yusuf wrote: > javadoc Done. https://codereview.chromium.org/2931433004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2931433004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:73: @VisibleForTesting On 2017/08/08 15:39:50, Yusuf wrote: > lets keep these as private but add getters for testing here. Done. https://codereview.chromium.org/2931433004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:87: void onMenuShown(); On 2017/08/08 15:39:50, Yusuf wrote: > javadoc Done. https://codereview.chromium.org/2931433004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:173: @VisibleForTesting On 2017/08/08 15:39:50, Yusuf wrote: > do you need to add this? @VisibleForTesting should only be needed to make sure > proguard doesnt strip calls that are only used by tests. This one is used, so > should not be stripped already? Done. https://codereview.chromium.org/2931433004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2931433004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:123: @VisibleForTesting On 2017/08/08 15:39:50, Yusuf wrote: > ditto my comment on VisibleForTesting. if it is not needed, lets not add this > annotation. Done. https://codereview.chromium.org/2931433004/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java (right): https://codereview.chromium.org/2931433004/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:63: public final CallbackHelper onBrowserActionsMenuShownCallback = new CallbackHelper(); On 2017/08/08 15:39:50, Yusuf wrote: > mVariableName on all these. And feel free to move these to the test class, make > this class a private class (missing static), so that you wont have to do > mdelegate.mcallbackhelper calls. Done. https://codereview.chromium.org/2931433004/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:181: @SmallTest On 2017/08/08 15:39:50, Yusuf wrote: > You might need to run some of these on phones only. Done.
https://codereview.chromium.org/2931433004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2931433004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:37: public static class BrowserActionActivityDelegate { I am a bit confused about this class. AFAICT we don't use the activity related implementation of this at all. (We have empty impls for all calls below). So if we are using it for tests only, is there a way to get this out of the activity? The helper seems to call all these anyway, why not make it a HelperDelegate? and have the test just talk with the Helper? https://codereview.chromium.org/2931433004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2931433004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:95: static List<Integer> getCustomActionsIdGroup() { rename with ForTesting in all of these below. https://codereview.chromium.org/2931433004/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java (right): https://codereview.chromium.org/2931433004/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:63: public final CallbackHelper mOnFinishNativeInitializationCallback = new CallbackHelper(); private final for all three
https://codereview.chromium.org/2931433004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2931433004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:37: public static class BrowserActionActivityDelegate { On 2017/08/09 17:02:54, Yusuf wrote: > I am a bit confused about this class. AFAICT we don't use the activity related > implementation of this at all. (We have empty impls for all calls below). > > So if we are using it for tests only, is there a way to get this out of the > activity? The helper seems to call all these anyway, why not make it a > HelperDelegate? and have the test just talk with the Helper? Done. https://codereview.chromium.org/2931433004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2931433004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:95: static List<Integer> getCustomActionsIdGroup() { On 2017/08/09 17:02:54, Yusuf wrote: > rename with ForTesting in all of these below. Done. https://codereview.chromium.org/2931433004/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java (right): https://codereview.chromium.org/2931433004/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:63: public final CallbackHelper mOnFinishNativeInitializationCallback = new CallbackHelper(); On 2017/08/09 17:02:54, Yusuf wrote: > private final for all three Done.
https://codereview.chromium.org/2931433004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2931433004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:80: private final BrowserActionsContextMenuItemDelegate mDelegate; lets rename this to mMenuItemDelegate to avoid confusion https://codereview.chromium.org/2931433004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:106: @VisibleForTesting javadoc https://codereview.chromium.org/2931433004/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java (right): https://codereview.chromium.org/2931433004/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:167: SparseArray<PendingIntent> actions = mTestDelegate.mCustomActions; does this compile for tests? This follows the same pattern we tried to avoid with callbackhelpers. Lets either add getters for these or make them members of the test class instead and access directly.
https://codereview.chromium.org/2931433004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2931433004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:80: private final BrowserActionsContextMenuItemDelegate mDelegate; On 2017/08/09 20:03:10, Yusuf wrote: > lets rename this to mMenuItemDelegate to avoid confusion Done. https://codereview.chromium.org/2931433004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:106: @VisibleForTesting On 2017/08/09 20:03:11, Yusuf wrote: > javadoc Done. https://codereview.chromium.org/2931433004/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java (right): https://codereview.chromium.org/2931433004/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:167: SparseArray<PendingIntent> actions = mTestDelegate.mCustomActions; On 2017/08/09 20:03:11, Yusuf wrote: > does this compile for tests? > > This follows the same pattern we tried to avoid with callbackhelpers. Lets > either add getters for these or make them members of the test class instead and > access directly. Done.
lgtm
While it is too late now, this CL does too many things IMO. We could have added the progress dialog, fixed strict mode, and some of the smaller changes separately and this would have been a bit easier to read. Just something to keep in mind in the future. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:110: @VisibleForTesting nit, usually goes below the comment https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:157: public String getCreatorPackageName() { where is this used? https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:99: public interface MenuShownListener { I'd just use a vanilla Runnable instead of introducing a new interface. We already use runnables when we call into other places so that keeps it more consistent. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:111: void setTestDelegateForTesting(BrowserActionsTestDelegate testDelegate) { I would put this below the constructor https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:113: mTestDelegate.initialize(mMenuItemDelegate, CUSTOM_BROWSER_ACTIONS_ID_GROUP, Instead of passing CUSTOM_BROWSER_ACTIONS_ID_GROUP, just make that protected or package protected. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:198: mMenuItemDelegate.onOpenInBackground(mCurrentContextMenuParams.getLinkUrl()); i'd pull this out to a small helper function and share with onNativeInitialized below https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:138: loadUrlParams, TabLaunchType.FROM_BROWSER_ACTIONS, null, false); if CTA is running, should we just use FROM_LONGPRESS_BACKGROUND here? https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:139: if (tab != null) { Should tab ever be null here? Should this be an assert tab != null? https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelOrderController.java (right): https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelOrderController.java:137: else if (type == TabLaunchType.FROM_BROWSER_ACTIONS) { I would put this all on a single line (or add braces above) https://codereview.chromium.org/2931433004/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java (right): https://codereview.chromium.org/2931433004/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:66: private BrowserActionsContextMenuItemDelegate mMenuItemDelegate; add a blank line above this https://codereview.chromium.org/2931433004/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:102: private TestDelegate mTestDelegate; I would put all the variables together and move your inner class definition down below them https://codereview.chromium.org/2931433004/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:297: int newTabId = mActivityTestRule.getActivity().getCurrentTabModel().getTabAt(2).getId(); at this point, i don't think you'll be able to guarantee the activity is alive. I would save the id above before triggering custom tabs. Basically, I would try running these tests with "Don't keep activities" turned on an make sure everything works. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:298: final Intent intent = buildNotificationIntent(instrumentation, newTabId, true); here you're not really using the production code, you're mimicking it. I think we should look at capturing the intent in the test code instead of trying to show a notification for real and then dispatch it here to ensure it does what we want. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:309: final Instrumentation instrumentation = InstrumentationRegistry.getInstrumentation(); can we replace any of this with ActivityUtils.waitForActivity?
https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:110: @VisibleForTesting On 2017/08/10 23:00:24, Ted C wrote: > nit, usually goes below the comment Done. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:157: public String getCreatorPackageName() { On 2017/08/10 23:00:24, Ted C wrote: > where is this used? Ops, looks like the functions call this have been deleted but I forget to delete this. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:99: public interface MenuShownListener { On 2017/08/10 23:00:24, Ted C wrote: > I'd just use a vanilla Runnable instead of introducing a new interface. We > already use runnables when we call into other places so that keeps it more > consistent. Done. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:111: void setTestDelegateForTesting(BrowserActionsTestDelegate testDelegate) { On 2017/08/10 23:00:24, Ted C wrote: > I would put this below the constructor Done. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:113: mTestDelegate.initialize(mMenuItemDelegate, CUSTOM_BROWSER_ACTIONS_ID_GROUP, On 2017/08/10 23:00:24, Ted C wrote: > Instead of passing CUSTOM_BROWSER_ACTIONS_ID_GROUP, just make that protected or > package protected. We previously made all these fields protected and then felt it was not a good idea to expose some fields to the whole package only for testing, so we add this initialize function to make them only accessible to the test files. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:198: mMenuItemDelegate.onOpenInBackground(mCurrentContextMenuParams.getLinkUrl()); On 2017/08/10 23:00:24, Ted C wrote: > i'd pull this out to a small helper function and share with onNativeInitialized > below Done. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:138: loadUrlParams, TabLaunchType.FROM_BROWSER_ACTIONS, null, false); On 2017/08/10 23:00:25, Ted C wrote: > if CTA is running, should we just use FROM_LONGPRESS_BACKGROUND here? FROM_LONGPRESS_BACKGROUND has a different behavior for showing animation and determining the insertion index. That's why we add this new TabLaunchType. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:139: if (tab != null) { On 2017/08/10 23:00:24, Ted C wrote: > Should tab ever be null here? Should this be an assert tab != null? Done. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelOrderController.java (right): https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelOrderController.java:137: else if (type == TabLaunchType.FROM_BROWSER_ACTIONS) { On 2017/08/10 23:00:25, Ted C wrote: > I would put this all on a single line (or add braces above) Somehow cl format prevents both of the options and keeps the code as before. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java (right): https://codereview.chromium.org/2931433004/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:66: private BrowserActionsContextMenuItemDelegate mMenuItemDelegate; On 2017/08/10 23:00:25, Ted C wrote: > add a blank line above this Done. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:102: private TestDelegate mTestDelegate; On 2017/08/10 23:00:25, Ted C wrote: > I would put all the variables together and move your inner class definition down > below them Done. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:297: int newTabId = mActivityTestRule.getActivity().getCurrentTabModel().getTabAt(2).getId(); On 2017/08/10 23:00:25, Ted C wrote: > at this point, i don't think you'll be able to guarantee the activity is alive. > I would save the id above before triggering custom tabs. > > Basically, I would try running these tests with "Don't keep activities" turned > on an make sure everything works. Done. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:298: final Intent intent = buildNotificationIntent(instrumentation, newTabId, true); On 2017/08/10 23:00:25, Ted C wrote: > here you're not really using the production code, you're mimicking it. I think > we should look at capturing the intent in the test code instead of trying to > show a notification for real and then dispatch it here to ensure it does what we > want. Done. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:309: final Instrumentation instrumentation = InstrumentationRegistry.getInstrumentation(); On 2017/08/10 23:00:25, Ted C wrote: > can we replace any of this with ActivityUtils.waitForActivity? I look into the ActivityUtils.waitForActivity and it seems the problem with that is there is no way to customize the Intent to trigger the activity. For Browser Actions, we need the Intent to have some extras such as url and default callback PendingIntent.
https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:113: mTestDelegate.initialize(mMenuItemDelegate, CUSTOM_BROWSER_ACTIONS_ID_GROUP, On 2017/08/11 02:24:39, ltian wrote: > On 2017/08/10 23:00:24, Ted C wrote: > > Instead of passing CUSTOM_BROWSER_ACTIONS_ID_GROUP, just make that protected > or > > package protected. > > We previously made all these fields protected and then felt it was not a good > idea to expose some fields to the whole package only for testing, so we add this > initialize function to make them only accessible to the test files. I certainly agree as I think protected fields can be somewhat off putting. But explicitly for the constant, I think that is something where we should be willing to expose it for testing. Passing a constant through a delegate seems overkill. For all other fields, this is a fine pattern. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:138: loadUrlParams, TabLaunchType.FROM_BROWSER_ACTIONS, null, false); On 2017/08/11 02:24:39, ltian wrote: > On 2017/08/10 23:00:25, Ted C wrote: > > if CTA is running, should we just use FROM_LONGPRESS_BACKGROUND here? > > FROM_LONGPRESS_BACKGROUND has a different behavior for showing animation and > determining the insertion index. That's why we add this new TabLaunchType. Ack. I guess for longpress background they really expect a parent tab, so that doesn't make sense. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelOrderController.java (right): https://codereview.chromium.org/2931433004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelOrderController.java:137: else if (type == TabLaunchType.FROM_BROWSER_ACTIONS) { On 2017/08/11 02:24:39, ltian wrote: > On 2017/08/10 23:00:25, Ted C wrote: > > I would put this all on a single line (or add braces above) > > Somehow cl format prevents both of the options and keeps the code as before. really?! That seems quite unfortunate. I guess in that case, I would recommend doing if (type == TabLaunchType.FROM_RESTORE || type == TabLaunchType.FROM_BROWSER_ACTIONS) return false; Then that at least will avoid this formatting weirdness. https://codereview.chromium.org/2931433004/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java (right): https://codereview.chromium.org/2931433004/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:309: final Instrumentation instrumentation = InstrumentationRegistry.getInstrumentation(); On 2017/08/11 02:24:39, ltian wrote: > On 2017/08/10 23:00:25, Ted C wrote: > > can we replace any of this with ActivityUtils.waitForActivity? > > I look into the ActivityUtils.waitForActivity and it seems the problem with that > is there is no way to customize the Intent to trigger the activity. For Browser > Actions, we need the Intent to have some extras such as url and default callback > PendingIntent. I think this is what this is for: https://cs.chromium.org/chromium/src/chrome/test/android/javatests/src/org/ch... waitForActivity(InstrumentationRegistry.getInstrumentation(), BrowserActionActivity.class, new Runnable() { @Override public void run() { sendBrowserActionIntent(instrumentation, url); } }); https://codereview.chromium.org/2931433004/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java (right): https://codereview.chromium.org/2931433004/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:226: Intent notificationIntent = mMenuItemDelegate.getNotificationIntent(); Is there a way to overwrite the MenuItemDelegate in the test code? Ideally, we wouldn't ever add notifications in test as they likely leave being traces of themselves between test runs. I think this is likely ok for now, but we should figure out a way where we don't really need to interact with the notification system for tests.
https://codereview.chromium.org/2931433004/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java (right): https://codereview.chromium.org/2931433004/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/browseractions/BrowserActionActivityTest.java:226: Intent notificationIntent = mMenuItemDelegate.getNotificationIntent(); On 2017/08/11 05:40:43, Ted C wrote: > Is there a way to overwrite the MenuItemDelegate in the test code? Ideally, we > wouldn't ever add notifications in test as they likely leave being traces of > themselves between test runs. I think this is likely ok for now, but we should > figure out a way where we don't really need to interact with the notification > system for tests. 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 Link to the patchset: https://codereview.chromium.org/2931433004/#ps180001 (title: "Update based on Ted's comments.")
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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/2931433004/#ps200001 (title: "Fix compile error.")
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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/2931433004/#ps220001 (title: "Fix test failures.")
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/2931433004/#ps240001 (title: "Fix test failures.")
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": 240001, "attempt_start_ts": 1502765376777470,
"parent_rev": "f0cabc438f627623dd131666ddc74e7a1a7bc4b4", "commit_rev":
"fc32a59653e65cee968472d8c91fa060fdf981d9"}
Message was sent while issue was closed.
Description was changed from ========== [Android] Open a tab in the background from Browser Actions if ChromeTabbedActivity is available Implements the function that opens a tab for the given url in the background of Chrome from Browser Actions menu. Also add a new TabLaunchType (Browser Actions) which does not show transition and always add to the top of the tab list. Besides, postpones operation by showing a ProgressDialog to block UI until native libraries are loaded. BUG=730305 ========== to ========== [Android] Open a tab in the background from Browser Actions if ChromeTabbedActivity is available Implements the function that opens a tab for the given url in the background of Chrome from Browser Actions menu. Also add a new TabLaunchType (Browser Actions) which does not show transition and always add to the top of the tab list. Besides, postpones operation by showing a ProgressDialog to block UI until native libraries are loaded. BUG=730305 Review-Url: https://codereview.chromium.org/2931433004 Cr-Commit-Position: refs/heads/master@{#494316} Committed: https://chromium.googlesource.com/chromium/src/+/fc32a59653e65cee968472d8c91f... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/fc32a59653e65cee968472d8c91f...
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2997163002/ by bengr@chromium.org. The reason for reverting is: Breaks testOpenMultipleTabInBackgroundWhenChromeAvailable on tablets.. |
