Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2689993002:
Refactor the INSTALL_SHORTCUT broadcast code into ChromeShortcutManager (Closed)
|
Created:
3 years, 10 months ago by Marti Wong Modified:
3 years, 10 months ago CC:
chromium-reviews, dominickn+watch_chromium.org, pkotwicz+watch_chromium.org, zpeng+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor the INSTALL_SHORTCUT broadcast code into ChromeShortcutManager.
Currently, the INSTALL_SHORTCUT broadcast code is done inside
ShortcutHelper. This CL separates the INSTALL_SHORTCUT broadcasting
code from ShortcutHelper into ChromeShortcutManager.
BUG=680374, 679476
Review-Url: https://codereview.chromium.org/2689993002
Cr-Commit-Position: refs/heads/master@{#451549}
Committed: https://chromium.googlesource.com/chromium/src/+/12166f58c0e466bf565cdaf09789a1896cfd9946
Patch Set 1 #
Total comments: 14
Patch Set 2 : Make some changes according to review comments #
Total comments: 11
Patch Set 3 : Change according to comments #
Total comments: 5
Patch Set 4 : show toast after adding shortcut #Patch Set 5 : Sync and upload again #
Total comments: 26
Patch Set 6 : Change according to review comments. #Messages
Total messages: 57 (34 generated)
The CQ bit was checked by martiw@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...
Description was changed from ========== Refactor the INSTALL_SHORTCUT broadcasting code into ChromeShortcutManager. Currently, the INSTALL_SHORTCUT broadcast code is done inside ShortcutHelper. This CL separates the INSTALL_SHORTCUT broadcasting code from ShortcutHelper into ChromeShortcutManager. This is in preparation for using the new adding shortcut API in future release. BUG=680374 ========== to ========== Refactor the INSTALL_SHORTCUT broadcast code into ChromeShortcutManager. Currently, the INSTALL_SHORTCUT broadcast code is done inside ShortcutHelper. This CL separates the INSTALL_SHORTCUT broadcasting code from ShortcutHelper into ChromeShortcutManager. This is in preparation for using the new adding shortcut API in future release. BUG=680374 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
martiw@chromium.org changed reviewers: + dominickn@chromium.org
PTAL, thanks~!
Description was changed from ========== Refactor the INSTALL_SHORTCUT broadcast code into ChromeShortcutManager. Currently, the INSTALL_SHORTCUT broadcast code is done inside ShortcutHelper. This CL separates the INSTALL_SHORTCUT broadcasting code from ShortcutHelper into ChromeShortcutManager. This is in preparation for using the new adding shortcut API in future release. BUG=680374 ========== to ========== Refactor the INSTALL_SHORTCUT broadcast code into ChromeShortcutManager. Currently, the INSTALL_SHORTCUT broadcast code is done inside ShortcutHelper. This CL separates the INSTALL_SHORTCUT broadcasting code from ShortcutHelper into ChromeShortcutManager. BUG=680374 ==========
Thanks! This is very clean. I have mostly minor comments. :) https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:95: private static final String TAG = "ShortcutHelper"; You moved this TAG - perhaps put it back again? https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:109: public static class Delegate { This isn't feasible right now (this comment is for the future!), but I think a cleaner design might actually be to replace ShortcutHelper with ChromeShortcutManager. We can't do this right now, but when this all merges together we should consider that. :) https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:115: public void requestPinShortcut(final Context context, final Intent intent) { Perhaps call this method addShortcutToHomescreen? https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java (right): https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:34: public void addShortcutToHomeScreen(final Context context, final Intent intent) { Instead of taking an Intent, can this take the id, shortcutTitle, bitmap, and shortcutIntent so that the INSTALL_SHORTCUT constant can be moved into here? https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:40: public boolean isAddShortcutToHomeScreenSupported(final Context context) { Perhaps call this canAddShortcutToHomescreen() https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:48: public boolean canShowAddedToHomescreenToast(final Context context) { Maybe call this shouldShowAddedToHomescreenToast()? canShow makes it sound like there are times when you can't show it (as opposed to times when we should not show it)
pkotwicz@chromium.org changed reviewers: + pkotwicz@chromium.org
https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:202: context.sendBroadcast(createAddToHomeIntent(shortcutTitle, bitmap, i)); Should we call requestPinShortcut() here too?
PTAL. appreciate your help~! https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:95: private static final String TAG = "ShortcutHelper"; This TAG is moved to ChromeShortcutManager https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:109: public static class Delegate { Thanks for the advice~! https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:115: public void requestPinShortcut(final Context context, final Intent intent) { On 2017/02/13 05:18:02, dominickn wrote: > Perhaps call this method addShortcutToHomescreen? Done. https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:202: context.sendBroadcast(createAddToHomeIntent(shortcutTitle, bitmap, i)); Thanks for the comment! Intent i has no id stored and it will cause a crash in requestPinShortcut() in future update. Actually, WebAPK people will be removing this part of code soon. That's why we leave this one alone. https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java (right): https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:34: public void addShortcutToHomeScreen(final Context context, final Intent intent) { On 2017/02/13 05:18:02, dominickn wrote: > Instead of taking an Intent, can this take the id, shortcutTitle, bitmap, and > shortcutIntent so that the INSTALL_SHORTCUT constant can be moved into here? Done. https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:40: public boolean isAddShortcutToHomeScreenSupported(final Context context) { On 2017/02/13 05:18:02, dominickn wrote: > Perhaps call this canAddShortcutToHomescreen() Done. https://codereview.chromium.org/2689993002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:48: public boolean canShowAddedToHomescreenToast(final Context context) { On 2017/02/13 05:18:02, dominickn wrote: > Maybe call this shouldShowAddedToHomescreenToast()? canShow makes it sound like > there are times when you can't show it (as opposed to times when we should not > show it) Done.
The CQ bit was checked by martiw@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...
Thanks! https://codereview.chromium.org/2689993002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2689993002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:151: Context context = ContextUtils.getApplicationContext(); Remove the context variable, and call setPackage() inside addShortcutToHomescreen() (where you already have the context) https://codereview.chromium.org/2689993002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:165: Context context = ContextUtils.getApplicationContext(); This variable is no longer used: remove. https://codereview.chromium.org/2689993002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:212: Context context = ContextUtils.getApplicationContext(); Remove context from here, and call setPackage() inside addShortcutToHomescreen https://codereview.chromium.org/2689993002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java (right): https://codereview.chromium.org/2689993002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:25: protected static Context sContext; I had a look, and no other Java file in the repository has a static context field. Can you remove this, and just call ContextUtil.getApplicationContext() where you need it? https://codereview.chromium.org/2689993002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:29: public static final String INSTALL_SHORTCUT = "com.android.launcher.action.INSTALL_SHORTCUT"; Can this be private / protected?
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...)
PTAL thanks again https://codereview.chromium.org/2689993002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (left): https://codereview.chromium.org/2689993002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:289: } Moved to ChromeShortcutManager https://codereview.chromium.org/2689993002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2689993002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:151: Context context = ContextUtils.getApplicationContext(); Seems it's more straightforward that the shortcutIntent is well-set here before passing it to addShortcutToHomescreen. Also if we do getPackageManager in ChromeShortcutManager, some AddToHomescreenManagerTest will be broken. (as ChromeShortcutManager is bypassed during the test) https://codereview.chromium.org/2689993002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:165: Context context = ContextUtils.getApplicationContext(); On 2017/02/14 03:04:21, dominickn wrote: > This variable is no longer used: remove. Done. thanks! https://codereview.chromium.org/2689993002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:212: Context context = ContextUtils.getApplicationContext(); will keep getPackageName() below. So this context variable still needed here. https://codereview.chromium.org/2689993002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java (right): https://codereview.chromium.org/2689993002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:25: protected static Context sContext; On 2017/02/14 03:04:21, dominickn wrote: > I had a look, and no other Java file in the repository has a static context > field. Can you remove this, and just call ContextUtil.getApplicationContext() > where you need it? Done. https://codereview.chromium.org/2689993002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:29: public static final String INSTALL_SHORTCUT = "com.android.launcher.action.INSTALL_SHORTCUT"; changed to private. thanks~!
lgtm, thanks!
martiw@chromium.org changed reviewers: + dfalcantara@chromium.org
Hi dfalcantara, PTAL to ChromeApplication.java thanks~!
The CQ bit was checked by martiw@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...
Hi dfalcantara, Please hold on the review for now. I still need to make some changes on this CL. thanks~!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
https://codereview.chromium.org/2689993002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2689993002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:196: i.addCategory(Intent.CATEGORY_LAUNCHER); Should we call addShortcutToHomescreen() here?
Ditto on re-adding me when it's ready to review. Trying to keep better track of what's still pending in El Queue.
dfalcantara@chromium.org changed reviewers: - dfalcantara@chromium.org
The CQ bit was checked by martiw@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...
martiw@chromium.org changed reviewers: + dfalcantara@chromium.org
Hi Dfalcantara, we'd like to land this CL first and make the followup change in a separate CL. So PTAL to ChromeApplication.java to see if it's okay. thx~! https://codereview.chromium.org/2689993002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2689993002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:196: i.addCategory(Intent.CATEGORY_LAUNCHER); On 2017/02/14 16:25:56, pkotwicz wrote: > Should we call addShortcutToHomescreen() here? Thanks for the comment! Actually, WebAPK people will be removing this part of code soon. That's why we leave this one alone. And Intent i here has no id stored in it and it will cause a crash in addShortcutToHomescreen() in future update.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Just finished the followup change. Seems no need to start a separate CL. So I just upload the change to here.
Description needs work. Couldn't figure out why the CL needs to worry about toasting. https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:91: // This intent category is for the callback intent when using the new shortcut API 1) end comment with a period. 2) fix comment up on line 62 so it also ends with a period. 3) Should really be a one liner javadoc so it shows up in the IDE: /** Used for the callback Intent when using the new shortcut API. */ 4) Stick this with the other Intent-related strings up top (under EXTRA_WEBAPK_PACKAGE_NAME) https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:108: /** Request Android to add a shortcut to the home screen. */ This javadoc sounds more like a function comment and applies only to the top function. Maybe something totally generic like: /** Helper for generating home screen shortcuts. */ https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:114: * @param intent Intent to fire when the shortcut is activated. nit: Line up the @params to match rest of file? @param title Title of the shortcut. @param icon Image that represents the shortcut. @param intent Intent to fire when the shortcut is activated. https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:237: * the new shortcut API End all your comments with periods https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:240: String title = intent.getStringExtra(Intent.EXTRA_SHORTCUT_NAME); 1) Never trust an Intent; use IntentUtils#safeGetStringExtra 2) Should this function be doing the if (shouldShowToast) check instead of the callers? You can append "IfNeeded" or something to the function name. https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:248: public static boolean isShowtoastIntent(Intent intent) { nit: capitalize Toast https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:370: public static boolean isAddToHomeIntentSupported(Context context) { This doesn't use the context anymore. Might as well remove it. https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java (right): https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:20: * addShortcutToHomeScreen is supported or not. 1) Don't need to enumerate everything it does. Just say it handles adding shortcuts to the home screen, or something. 2) This class doesn't actually manage shortcuts; it just adds them. Maybe call it a ChromeShortcutHelper or ShortcutCreationDelegate or something https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:23: private static ChromeShortcutManager sInstance; nit: move private static below private static final https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:34: .createChromeShortcutManager(); 1) is this a git cl format thing? really struggling to understand how it decides how to indent in these cases. 2) don't get the application context from the application context. https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:41: * @param title Title of the shortcut. nit: line up params? https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:54: public void addShortcutToHomeScreen(String title, Bitmap icon, Intent shortcutIntent) { public method -> add javadoc
https://codereview.chromium.org/2689993002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2689993002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:196: i.addCategory(Intent.CATEGORY_LAUNCHER); - We will be removing this code path in M59 not M58 - If the effort to keep this working is minimal, I would advocate for keeping this working yfriedman@ has suggested keeping this codepath around post M59 for people with non GMS devices. So we are not completely sure that this codepath will go away
PTAL again. your help is much appreciated https://codereview.chromium.org/2689993002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2689993002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:196: i.addCategory(Intent.CATEGORY_LAUNCHER); Got it. Thanks for the advice. I will do this in another CL. https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:91: // This intent category is for the callback intent when using the new shortcut API On 2017/02/15 19:40:18, dfalcantara (load balance plz) wrote: > 1) end comment with a period. > > 2) fix comment up on line 62 so it also ends with a period. > > 3) Should really be a one liner javadoc so it shows up in the IDE: > /** Used for the callback Intent when using the new shortcut API. */ > > 4) Stick this with the other Intent-related strings up top (under > EXTRA_WEBAPK_PACKAGE_NAME) Done. https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:108: /** Request Android to add a shortcut to the home screen. */ On 2017/02/15 19:40:19, dfalcantara (load balance plz) wrote: > This javadoc sounds more like a function comment and applies only to the top > function. Maybe something totally generic like: > > /** Helper for generating home screen shortcuts. */ Done. https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:114: * @param intent Intent to fire when the shortcut is activated. On 2017/02/15 19:40:18, dfalcantara (load balance plz) wrote: > nit: Line up the @params to match rest of file? > > @param title Title of the shortcut. > @param icon Image that represents the shortcut. > @param intent Intent to fire when the shortcut is activated. Done. https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:237: * the new shortcut API On 2017/02/15 19:40:18, dfalcantara (load balance plz) wrote: > End all your comments with periods Done. https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:240: String title = intent.getStringExtra(Intent.EXTRA_SHORTCUT_NAME); On 2017/02/15 19:40:19, dfalcantara (load balance plz) wrote: > 1) Never trust an Intent; use IntentUtils#safeGetStringExtra > 2) Should this function be doing the if (shouldShowToast) check instead of the > callers? You can append "IfNeeded" or something to the function name. 1) Done, thanks~! 2) this function should just show toast without checking. It is called when a callback intent is requesting a show toast so there is no question of checking. https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:248: public static boolean isShowtoastIntent(Intent intent) { On 2017/02/15 19:40:18, dfalcantara (load balance plz) wrote: > nit: capitalize Toast Done. https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:370: public static boolean isAddToHomeIntentSupported(Context context) { On 2017/02/15 19:40:18, dfalcantara (load balance plz) wrote: > This doesn't use the context anymore. Might as well remove it. Done. thanks. Sorry for this careless mistake. https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java (right): https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:20: * addShortcutToHomeScreen is supported or not. On 2017/02/15 19:40:19, dfalcantara (load balance plz) wrote: > 1) Don't need to enumerate everything it does. Just say it handles adding > shortcuts to the home screen, or something. > > 2) This class doesn't actually manage shortcuts; it just adds them. Maybe call > it a ChromeShortcutHelper or ShortcutCreationDelegate or something It's really hard to think of a new name without confusing with ShortcutHelper or ShortcutHelper.Delegate. And this class will be merged up to ShortcutHelper in future CL. So I'd like to keep its name as it. https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:23: private static ChromeShortcutManager sInstance; On 2017/02/15 19:40:19, dfalcantara (load balance plz) wrote: > nit: move private static below private static final Done. https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:34: .createChromeShortcutManager(); On 2017/02/15 19:40:19, dfalcantara (load balance plz) wrote: > 1) is this a git cl format thing? really struggling to understand how it > decides how to indent in these cases. > > 2) don't get the application context from the application context. Sorry about that. fixed. https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:41: * @param title Title of the shortcut. On 2017/02/15 19:40:19, dfalcantara (load balance plz) wrote: > nit: line up params? Done. https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:54: public void addShortcutToHomeScreen(String title, Bitmap icon, Intent shortcutIntent) { On 2017/02/15 19:40:19, dfalcantara (load balance plz) wrote: > public method -> add javadoc Done.
lgtm https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:370: public static boolean isAddToHomeIntentSupported(Context context) { On 2017/02/17 05:10:48, martiw wrote: > On 2017/02/15 19:40:18, dfalcantara (load balance plz) wrote: > > This doesn't use the context anymore. Might as well remove it. > > Done. thanks. Sorry for this careless mistake. Pfft, nothing to apologize for. https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java (right): https://codereview.chromium.org/2689993002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:20: * addShortcutToHomeScreen is supported or not. On 2017/02/17 05:10:49, martiw wrote: > On 2017/02/15 19:40:19, dfalcantara (load balance plz) wrote: > > 1) Don't need to enumerate everything it does. Just say it handles adding > > shortcuts to the home screen, or something. > > > > 2) This class doesn't actually manage shortcuts; it just adds them. Maybe > call > > it a ChromeShortcutHelper or ShortcutCreationDelegate or something > > It's really hard to think of a new name without confusing with ShortcutHelper or > ShortcutHelper.Delegate. > And this class will be merged up to ShortcutHelper in future CL. So I'd like to > keep its name as it. Acknowledged.
https://codereview.chromium.org/2689993002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2689993002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:196: i.addCategory(Intent.CATEGORY_LAUNCHER); LGTM as long as you do this in follow up CL
The CQ bit was checked by martiw@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_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Refactor the INSTALL_SHORTCUT broadcast code into ChromeShortcutManager. Currently, the INSTALL_SHORTCUT broadcast code is done inside ShortcutHelper. This CL separates the INSTALL_SHORTCUT broadcasting code from ShortcutHelper into ChromeShortcutManager. BUG=680374 ========== to ========== Refactor the INSTALL_SHORTCUT broadcast code into ChromeShortcutManager. Currently, the INSTALL_SHORTCUT broadcast code is done inside ShortcutHelper. This CL separates the INSTALL_SHORTCUT broadcasting code from ShortcutHelper into ChromeShortcutManager. BUG=680374, 679476 ==========
The CQ bit was checked by martiw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org Link to the patchset: https://codereview.chromium.org/2689993002/#ps100001 (title: "Change according to review 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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by martiw@chromium.org
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": 100001, "attempt_start_ts": 1487557744463770,
"parent_rev": "cb8696135be40717770ba4422c140c79851d9f74", "commit_rev":
"12166f58c0e466bf565cdaf09789a1896cfd9946"}
Message was sent while issue was closed.
Description was changed from ========== Refactor the INSTALL_SHORTCUT broadcast code into ChromeShortcutManager. Currently, the INSTALL_SHORTCUT broadcast code is done inside ShortcutHelper. This CL separates the INSTALL_SHORTCUT broadcasting code from ShortcutHelper into ChromeShortcutManager. BUG=680374, 679476 ========== to ========== Refactor the INSTALL_SHORTCUT broadcast code into ChromeShortcutManager. Currently, the INSTALL_SHORTCUT broadcast code is done inside ShortcutHelper. This CL separates the INSTALL_SHORTCUT broadcasting code from ShortcutHelper into ChromeShortcutManager. BUG=680374, 679476 Review-Url: https://codereview.chromium.org/2689993002 Cr-Commit-Position: refs/heads/master@{#451549} Committed: https://chromium.googlesource.com/chromium/src/+/12166f58c0e466bf565cdaf09789... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/12166f58c0e466bf565cdaf09789... |
