Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2663373003:
[Android] Add options in the context menu of CCT to open in a new Chrome tab or incoginto tab (Closed)
Description[Android] Add options in the context menu of CCT to open in a new Chrome tab or incoginto tab
Add two options ("Open in new Chrome tab" and "Open in Chrome incognito
tab") in the context menu of CCT to open the link in a new Chrome tab or
a new Chrome incognito tab.
The "Open in new Chrome tab" behavior differs from the behavior in the
Chrome context menu, where it opens the tab in the background.
The "Open in Chrome incognito tab" behavior matches the behavior in the
Chrome context menu.
BUG=682708
Review-Url: https://codereview.chromium.org/2663373003
Cr-Commit-Position: refs/heads/master@{#452246}
Committed: https://chromium.googlesource.com/chromium/src/+/9695e1d22d1961510082f3c3444acd39e66a0190
Patch Set 1 #Patch Set 2 : Update the bug number for the CL. #Patch Set 3 : Add logic to behave differently based on whether current browser is the default browser #Patch Set 4 : Fix failure for test cases #
Total comments: 7
Patch Set 5 : Fix for Ted's comments. #
Total comments: 7
Patch Set 6 : Update based on Ted's comments. #Patch Set 7 : Fix Incorrect lazy initialization and update of static field compile error. #Patch Set 8 : Add lock in initialize function of DefaultBrowserInfo class. #
Total comments: 7
Patch Set 9 : Updates based on Maira's comments. #Patch Set 10 : Fix comflict. #
Total comments: 4
Patch Set 11 : Update based on Maria's comments. #
Total comments: 4
Patch Set 12 : Upddate based on Maria's nits. #Patch Set 13 : Rebase to master branch. #Messages
Total messages: 76 (52 generated)
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: This issue passed the CQ dry run.
ltian@chromium.org changed reviewers: + tedchoc@chromium.org
Hi, Ted Could you take a look about my changes in this CL? Thanks!
Description was changed from
==========
[Android] Add options in the context menu of CCT to open in a new Chrome tab or
incoginto tab
Add two options ("Open in new Chrome tab" and "Open in Chrome incognito
tab") in the context menu of CCT to open the link in a new Chrome tab or
a new Chrome incognito tab.
The "Open in new Chrome tab" behavior differs from the behavior in the
Chrome context menu, where it opens the tab in the background.
The "Open in Chrome incognito tab" behavior matches the behavior in the
Chrome context menu.
BUG=None
==========
to
==========
[Android] Add options in the context menu of CCT to open in a new Chrome tab or
incoginto tab
Add two options ("Open in new Chrome tab" and "Open in Chrome incognito
tab") in the context menu of CCT to open the link in a new Chrome tab or
a new Chrome incognito tab.
The "Open in new Chrome tab" behavior differs from the behavior in the
Chrome context menu, where it opens the tab in the background.
The "Open in Chrome incognito tab" behavior matches the behavior in the
Chrome context menu.
BUG=682708
==========
lgtm The slides in the bug show "Open in $Browser" as an option if Chrome is not the default browser. Is the plan to do that in a followup change?
On 2017/02/01 23:38:25, Ted C wrote: > lgtm > > The slides in the bug show "Open in $Browser" as an option if Chrome is not the > default browser. Is the plan to do that in a followup change? Oh actually, we might only want to show these two options if Chrome is the default browser. So maybe you need to add a check in ChromeContextMenuPopulator for that. Might want to ping sbirch@ about that before we land this.
On 2017/02/01 23:39:34, Ted C wrote: > On 2017/02/01 23:38:25, Ted C wrote: > > lgtm > > > > The slides in the bug show "Open in $Browser" as an option if Chrome is not > the > > default browser. Is the plan to do that in a followup change? > > Oh actually, we might only want to show these two options if Chrome is > the default browser. So maybe you need to add a check in > ChromeContextMenuPopulator > for that. Might want to ping sbirch@ about that before we land this. Check with Sam and the correct behavior is if Chrome is the default, show the two options, otherwise show only "Open in $Browser" option. However, because the code of this cl is on the machine I borrowed from Shanghai office which now I have already returned and since this cl is lgtm, could I first land it and have another cl to fix this part?
On 2017/02/06 18:38:01, ltian wrote: > On 2017/02/01 23:39:34, Ted C wrote: > > On 2017/02/01 23:38:25, Ted C wrote: > > > lgtm > > > > > > The slides in the bug show "Open in $Browser" as an option if Chrome is not > > the > > > default browser. Is the plan to do that in a followup change? > > > > Oh actually, we might only want to show these two options if Chrome is > > the default browser. So maybe you need to add a check in > > ChromeContextMenuPopulator > > for that. Might want to ping sbirch@ about that before we land this. > > Check with Sam and the correct behavior is if Chrome is the default, show the > two options, otherwise show only "Open in $Browser" option. However, because the > code of this cl is on the machine I borrowed from Shanghai office which now I > have already returned and since this cl is lgtm, could I first land it and have > another cl to fix this part? You can just do "git cl patch <Issue Number>" to your now local machine, make the edits and upload a new version w/o creating a new patch. It is better to land this all as one unit than piece by piece IMO.
On 2017/02/06 18:47:27, Ted C wrote: > On 2017/02/06 18:38:01, ltian wrote: > > On 2017/02/01 23:39:34, Ted C wrote: > > > On 2017/02/01 23:38:25, Ted C wrote: > > > > lgtm > > > > > > > > The slides in the bug show "Open in $Browser" as an option if Chrome is > not > > > the > > > > default browser. Is the plan to do that in a followup change? > > > > > > Oh actually, we might only want to show these two options if Chrome is > > > the default browser. So maybe you need to add a check in > > > ChromeContextMenuPopulator > > > for that. Might want to ping sbirch@ about that before we land this. > > > > Check with Sam and the correct behavior is if Chrome is the default, show the > > two options, otherwise show only "Open in $Browser" option. However, because > the > > code of this cl is on the machine I borrowed from Shanghai office which now I > > have already returned and since this cl is lgtm, could I first land it and > have > > another cl to fix this part? > > You can just do "git cl patch <Issue Number>" to your now local machine, make > the edits and upload a new version w/o creating a new patch. It is better > to land this all as one unit than piece by piece IMO. Cool, that is definitely a better way. Thanks for letting me know!
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: 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 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2663373003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2663373003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:329: if (isChromeDefault()) { Let's use ChromePreferenceManager.getInstance(context).getCachedChromeDefaultBrowser() here. It was added for instant apps so we can use it here instead of querying again. https://codereview.chromium.org/2663373003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java (right): https://codereview.chromium.org/2663373003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java:58: mDefaultBrowserFetcher = new AsyncTask<Void, Void, String>() { This is the same as in CustomTabAppMenuPropertiesDelegate. I think we should find a way to share this. DeferredStartupHandler already has cacheIsChromeDefaultBrowser. I wonder if we should unify that and this into a single place. We shouldn't need to save this value to shared prefs though since it isn't used on startup, but it would be good to put all the of this in one place. Maybe chrome/.../externalnav/DefaultBrowserUtils.java or "something" (adding Maria to give thoughts about name/location/and general concept). https://codereview.chromium.org/2663373003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java:288: Intent intent = new Intent(Intent.ACTION_VIEW, Uri.parse(url)); I think we want NEW_TASK here too
tedchoc@chromium.org changed reviewers: + mariakhomenko@chromium.org
+mariakhomenko for discussion on unifying our is chrome default/default browser logic into a helper (naming/location/etc..)
https://codereview.chromium.org/2663373003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java (right): https://codereview.chromium.org/2663373003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java:58: mDefaultBrowserFetcher = new AsyncTask<Void, Void, String>() { On 2017/02/11 00:08:46, Ted C wrote: > This is the same as in CustomTabAppMenuPropertiesDelegate. I think we should > find a way to share this. > > DeferredStartupHandler already has cacheIsChromeDefaultBrowser. > > I wonder if we should unify that and this into a single place. We shouldn't > need to save this value to shared prefs though since it isn't used on startup, > but it would be good to put all the of this in one place. > > Maybe chrome/.../externalnav/DefaultBrowserUtils.java or "something" (adding > Maria to give thoughts about name/location/and general concept). > I agree that having this in one place would be nice because more things related to default browsers are coming. I don't think it really belongs in externalnav since that just deals with navigation intercepts. I looked through the list of packages we have and there's nothing that matches well in my mind. Might have to be a class in browser/ folder. I would call it DefaultBrowserInfo maybe?
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...
https://codereview.chromium.org/2663373003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2663373003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:329: if (isChromeDefault()) { On 2017/02/11 00:08:46, Ted C wrote: > Let's use > ChromePreferenceManager.getInstance(context).getCachedChromeDefaultBrowser() > here. It was added for instant apps so we can use it here instead of querying > again. Done. https://codereview.chromium.org/2663373003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java (right): https://codereview.chromium.org/2663373003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java:58: mDefaultBrowserFetcher = new AsyncTask<Void, Void, String>() { On 2017/02/11 00:55:57, Maria wrote: > On 2017/02/11 00:08:46, Ted C wrote: > > This is the same as in CustomTabAppMenuPropertiesDelegate. I think we should > > find a way to share this. > > > > DeferredStartupHandler already has cacheIsChromeDefaultBrowser. > > > > I wonder if we should unify that and this into a single place. We shouldn't > > need to save this value to shared prefs though since it isn't used on startup, > > but it would be good to put all the of this in one place. > > > > Maybe chrome/.../externalnav/DefaultBrowserUtils.java or "something" (adding > > Maria to give thoughts about name/location/and general concept). > > > > I agree that having this in one place would be nice because more things related > to default browsers are coming. I don't think it really belongs in externalnav > since that just deals with navigation intercepts. > > I looked through the list of packages we have and there's nothing that matches > well in my mind. Might have to be a class in browser/ folder. I would call it > DefaultBrowserInfo maybe? Done. https://codereview.chromium.org/2663373003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java:288: Intent intent = new Intent(Intent.ACTION_VIEW, Uri.parse(url)); On 2017/02/11 00:08:46, Ted C wrote: > I think we want NEW_TASK here too Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2663373003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java (right): https://codereview.chromium.org/2663373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:23: private static final String SAMPLE_URL = "https://www.google.com"; I would use the one from cacheIsChromeDefaultBrowser http://www.madeupdomainforcheck123.com/ www.google.com could potentially be handled by a google app in the future, so it is safer to use something else IMO. https://codereview.chromium.org/2663373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:33: final Boolean isOpenedByChrome) throws InterruptedException, ExecutionException { Why capital B boolean? Also, do you need Activity here or can you ContextUtils.getApplicationContext? I would also not say openedByChrome, I would say something like "forceChromeAsDefault" or something more generic. https://codereview.chromium.org/2663373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:34: AsyncTask<Void, Void, String> defaultBrowserFetcher = new AsyncTask<Void, Void, String>() { Creating an AsyncTask and then calling get on it immediately doesn't provide any benefit. You need to add an initialize method to this class (whether it is through the singleton pattern or just another static method). Then you need to kick this off in DeferredStartupHandler like cacheIsChromeDefaultBrowser. Then in this method, you can just call get on the fetcher (potentially calling initialize yourself if it hadn't been called yet). Also, you shouldn't have this throw exceptions. I would just catch them and return the default label in that case. https://codereview.chromium.org/2663373003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java (right): https://codereview.chromium.org/2663373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java:43: private static final String SAMPLE_URL = "https://www.google.com"; no longer needed
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...)
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...)
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
https://codereview.chromium.org/2663373003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java (right): https://codereview.chromium.org/2663373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:23: private static final String SAMPLE_URL = "https://www.google.com"; On 2017/02/14 19:02:00, Ted C wrote: > I would use the one from cacheIsChromeDefaultBrowser > > http://www.madeupdomainforcheck123.com/ > > http://www.google.com could potentially be handled by a google app in the future, so it > is safer to use something else IMO. Done. https://codereview.chromium.org/2663373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:33: final Boolean isOpenedByChrome) throws InterruptedException, ExecutionException { On 2017/02/14 19:02:00, Ted C wrote: > Why capital B boolean? > > Also, do you need Activity here or can you ContextUtils.getApplicationContext? > > I would also not say openedByChrome, I would say something like > "forceChromeAsDefault" or something more generic. Done. https://codereview.chromium.org/2663373003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java (right): https://codereview.chromium.org/2663373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java:43: private static final String SAMPLE_URL = "https://www.google.com"; On 2017/02/14 19:02:00, Ted C wrote: > no longer needed Done.
https://codereview.chromium.org/2663373003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java (right): https://codereview.chromium.org/2663373003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:27: /** Prevents multi-threads from getting created simultaneously. */ A more useful comment would be to say something like "synchronizes background tasks to retrieve browser information" https://codereview.chromium.org/2663373003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:28: private static final Object DIR_CREATION_LOCK = new Object(); this should be named sDirCreationLock, it's final, but it's not a constant https://codereview.chromium.org/2663373003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:53: packageLabel = info.loadLabel(pm).toString(); is info.loadLabel() guaranteed to be non-null? Otherwise toString() call will throw https://codereview.chromium.org/2663373003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2663373003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:288: DefaultBrowserInfo.initBrowserFetcher(); Please either move the cache check to use the results of the browser fetcher or at least add a todo for doing so.
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2663373003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java (right): https://codereview.chromium.org/2663373003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:27: /** Prevents multi-threads from getting created simultaneously. */ On 2017/02/16 05:41:11, Maria wrote: > A more useful comment would be to say something like "synchronizes background > tasks to retrieve browser information" Done. https://codereview.chromium.org/2663373003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:28: private static final Object DIR_CREATION_LOCK = new Object(); On 2017/02/16 05:41:12, Maria wrote: > this should be named sDirCreationLock, it's final, but it's not a constant Done. https://codereview.chromium.org/2663373003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:53: packageLabel = info.loadLabel(pm).toString(); On 2017/02/16 05:41:12, Maria wrote: > is info.loadLabel() guaranteed to be non-null? Otherwise toString() call will > throw Done.
https://codereview.chromium.org/2663373003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java (right): https://codereview.chromium.org/2663373003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:80: ChromePreferenceManager.getInstance().setCachedChromeDefaultBrowser(isDefault); Just do this in initBrowserFetcher. E.g. on line 52 of initBrowserFetcher you have everything needed to figure out if the browser is default. Set the preference there and remove this call. https://codereview.chromium.org/2663373003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2663373003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:285: DefaultBrowserInfo.initBrowserFetcher(); should this be called from UI thread rather than from background since it just spins off an async task anyways?
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
lgtm https://codereview.chromium.org/2663373003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java (right): https://codereview.chromium.org/2663373003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:49: // Caches whether Chrome is set as a default browser on the device. nit: insert newline before comment https://codereview.chromium.org/2663373003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:54: // Check if there is a default handler for the Intent. If so, store its nit: insert newline above comment
https://codereview.chromium.org/2663373003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java (right): https://codereview.chromium.org/2663373003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:80: ChromePreferenceManager.getInstance().setCachedChromeDefaultBrowser(isDefault); On 2017/02/17 23:01:23, Maria wrote: > Just do this in initBrowserFetcher. > > E.g. on line 52 of initBrowserFetcher you have everything needed to figure out > if the browser is default. Set the preference there and remove this call. Done. https://codereview.chromium.org/2663373003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2663373003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:285: DefaultBrowserInfo.initBrowserFetcher(); On 2017/02/17 23:01:23, Maria wrote: > should this be called from UI thread rather than from background since it just > spins off an async task anyways? Done. https://codereview.chromium.org/2663373003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java (right): https://codereview.chromium.org/2663373003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:49: // Caches whether Chrome is set as a default browser on the device. On 2017/02/21 04:52:17, Maria wrote: > nit: insert newline before comment Done. https://codereview.chromium.org/2663373003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java:54: // Check if there is a default handler for the Intent. If so, store its On 2017/02/21 04:52:17, Maria wrote: > nit: insert newline above comment Done.
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, mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2663373003/#ps220001 (title: "Upddate based on Maria's nits.")
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by ltian@chromium.org
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
Failed to apply patch for
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java:
While running git apply --index -p1;
error: patch failed:
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java:4
error:
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java:
patch does not apply
Patch:
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java
Index:
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java
diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java
b/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java
index
af2153a70ce41b3aca16d9262c9c531f0a37940e..fd3003406344d76ebb668dbf621abbd10c27981a
100644
---
a/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java
+++
b/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java
@@ -4,18 +4,13 @@
package org.chromium.chrome.browser.customtabs;
-import android.content.Intent;
-import android.content.pm.PackageManager;
-import android.content.pm.ResolveInfo;
-import android.net.Uri;
-import android.os.AsyncTask;
import android.view.Menu;
import android.view.MenuItem;
-import org.chromium.base.BuildInfo;
import org.chromium.base.VisibleForTesting;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.ChromeActivity;
+import org.chromium.chrome.browser.DefaultBrowserInfo;
import org.chromium.chrome.browser.UrlConstants;
import org.chromium.chrome.browser.appmenu.AppMenuPropertiesDelegate;
import org.chromium.chrome.browser.download.DownloadUtils;
@@ -26,7 +21,6 @@ import org.chromium.chrome.browser.tab.Tab;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
-import java.util.concurrent.ExecutionException;
/**
* App menu properties delegate for {@link CustomTabActivity}.
@@ -38,10 +32,10 @@ public class CustomTabAppMenuPropertiesDelegate extends
AppMenuPropertiesDelegat
private final boolean mIsMediaViewer;
private final boolean mShowStar;
private final boolean mShowDownload;
+ private final boolean mIsOpenedByChrome;
private final List<String> mMenuEntries;
private final Map<MenuItem, Integer> mItemToIndexMap = new
HashMap<MenuItem, Integer>();
- private final AsyncTask<Void, Void, String> mDefaultBrowserFetcher;
private boolean mIsCustomEntryAdded;
@@ -57,29 +51,7 @@ public class CustomTabAppMenuPropertiesDelegate extends
AppMenuPropertiesDelegat
mIsMediaViewer = isMediaViewer;
mShowStar = showStar;
mShowDownload = showDownload;
-
- mDefaultBrowserFetcher = new AsyncTask<Void, Void, String>() {
- @Override
- protected String doInBackground(Void... params) {
- String packageLabel = null;
- if (isOpenedByChrome) {
- // If the Custom Tab was created by Chrome, Chrome should
open it.
- packageLabel = BuildInfo.getPackageLabel(activity);
- } else {
- // Check if there is a default handler for the Intent. If
so, grab its label.
- Intent intent = new Intent(Intent.ACTION_VIEW,
Uri.parse(SAMPLE_URL));
- PackageManager pm = activity.getPackageManager();
- ResolveInfo info = pm.resolveActivity(intent, 0);
- if (info != null && info.match != 0) {
- packageLabel = info.loadLabel(pm).toString();
- }
- }
-
- return packageLabel == null
- ?
activity.getString(R.string.menu_open_in_product_default)
- : activity.getString(R.string.menu_open_in_product,
packageLabel);
- }
- }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
+ mIsOpenedByChrome = isOpenedByChrome;
}
@Override
@@ -120,13 +92,8 @@ public class CustomTabAppMenuPropertiesDelegate extends
AppMenuPropertiesDelegat
menu.findItem(R.id.request_desktop_site_id).setVisible(false);
addToHomeScreenItem.setVisible(false);
} else {
- try {
- openInChromeItem.setTitle(mDefaultBrowserFetcher.get());
- } catch (InterruptedException | ExecutionException e) {
- openInChromeItem.setTitle(
-
mActivity.getString(R.string.menu_open_in_product_default));
- }
- updateBookmarkMenuItem(bookmarkItem, currentTab);
+ openInChromeItem.setTitle(
+
DefaultBrowserInfo.getTitleOpenInDefaultBrowser(mIsOpenedByChrome));
}
bookmarkItem.setVisible(mShowStar);
downloadItem.setVisible(mShowDownload);
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, mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2663373003/#ps240001 (title: "Rebase to master branch.")
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": 1487795106756670,
"parent_rev": "8356dc885118b09ecf9464a85d5a9fc6c599d422", "commit_rev":
"9695e1d22d1961510082f3c3444acd39e66a0190"}
Message was sent while issue was closed.
Description was changed from
==========
[Android] Add options in the context menu of CCT to open in a new Chrome tab or
incoginto tab
Add two options ("Open in new Chrome tab" and "Open in Chrome incognito
tab") in the context menu of CCT to open the link in a new Chrome tab or
a new Chrome incognito tab.
The "Open in new Chrome tab" behavior differs from the behavior in the
Chrome context menu, where it opens the tab in the background.
The "Open in Chrome incognito tab" behavior matches the behavior in the
Chrome context menu.
BUG=682708
==========
to
==========
[Android] Add options in the context menu of CCT to open in a new Chrome tab or
incoginto tab
Add two options ("Open in new Chrome tab" and "Open in Chrome incognito
tab") in the context menu of CCT to open the link in a new Chrome tab or
a new Chrome incognito tab.
The "Open in new Chrome tab" behavior differs from the behavior in the
Chrome context menu, where it opens the tab in the background.
The "Open in Chrome incognito tab" behavior matches the behavior in the
Chrome context menu.
BUG=682708
Review-Url: https://codereview.chromium.org/2663373003
Cr-Commit-Position: refs/heads/master@{#452246}
Committed:
https://chromium.googlesource.com/chromium/src/+/9695e1d22d1961510082f3c3444a...
==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/9695e1d22d1961510082f3c3444a... |
