Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2129043002:
Split ShortcutHelper#addShortcut() into separate functions for each type of shortcut (Closed)
DescriptionSplit ShortcutHelper#addShortcut() into separate functions for each type of shortcut
BUG=619739
Committed: https://crrev.com/20667314938d2a6972f2c1baaccbc9b9c43f2aa6
Cr-Commit-Position: refs/heads/master@{#406334}
Patch Set 1 : Merge branch 'master' into webapk_builder_impl0 #
Total comments: 8
Patch Set 2 : Merge branch 'webapk_builder_impl00' into webapk_builder_impl0 #Patch Set 3 : Merge branch 'master' into webapk_builder_impl0 #
Total comments: 14
Patch Set 4 : Merge branch 'master' into webapk_builder_impl0 #
Messages
Total messages: 39 (17 generated)
Description was changed from ========== Split ShortcutHelper#addShortcut() into functions for each type of shortcut BUG=619739 ========== to ========== Split ShortcutHelper#addShortcut() into separate functions for each type of shortcut BUG=619739 ==========
Patchset #1 (id:1) has been deleted
pkotwicz@chromium.org changed reviewers: + hanxi@chromium.org
Xi can you please take a look? ShortcutHelper::AddShortcutInBackgroundWithSkBitmap() will hopefully soon talk to the server to generate a WebAPK. Talking to the server will occur in C++ via UrlFetcher. This requires moving the decision about which type of shortcut to create (WebAPK, non-WebAPK fullscreen web app, bookmark shortcut) from Java to C++.
Just leave some comments, overall looks good! https://codereview.chromium.org/2129043002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2129043002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:171: shortcutIntent.setPackage(context.getPackageName()); Please move line 166 to line 171 to createWebappShortcutIntent(). https://codereview.chromium.org/2129043002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:202: shortcutIntent.setPackage(context.getPackageName()); Please move line 199 to line 202 to createShortcutIntent(); https://codereview.chromium.org/2129043002/diff/20001/chrome/browser/android/... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2129043002/diff/20001/chrome/browser/android/... chrome/browser/android/shortcut_helper.cc:75: AddWebApkInBackgroundWithSkBitmap(info, webapp_id, icon_bitmap)) { If AddWebApkInBackgroundWithSkBitmap returns false, do we really want to fall back to create a Webapp shortcut? I remembered that we will show a toast when the WAM server fails to mint a WebAPK in the UI design. Since creating a webapp shortcut isn't the fallback anyway, I don't think we need to know nor check the return value of this function. Also the return value only mean whether the WebApkBuilder is available or not, not whether the WebAPK is installed or not, since the installation task is async.
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Xi can you please take another look? https://codereview.chromium.org/2129043002/diff/20001/chrome/browser/android/... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2129043002/diff/20001/chrome/browser/android/... chrome/browser/android/shortcut_helper.cc:75: AddWebApkInBackgroundWithSkBitmap(info, webapp_id, icon_bitmap)) { I have removed the fallback for now and have filed crbug.com/626950
Patchset #3 (id:100001) has been deleted
Patchset #2 (id:80001) has been deleted
https://codereview.chromium.org/2129043002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2129043002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:171: shortcutIntent.setPackage(context.getPackageName()); On 2016/07/08 19:10:38, Xi Han wrote: > Please move line 166 to line 171 to createWebappShortcutIntent(). It was my bad to ask you to make this changes, since I didn't realize lots of changes involved by changing this function. Sorry about this. I am totally ok if you also like to revert this part. https://codereview.chromium.org/2129043002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:202: shortcutIntent.setPackage(context.getPackageName()); On 2016/07/08 19:10:38, Xi Han wrote: > Please move line 199 to line 202 to createShortcutIntent(); Same as above. https://codereview.chromium.org/2129043002/diff/20001/chrome/browser/android/... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2129043002/diff/20001/chrome/browser/android/... chrome/browser/android/shortcut_helper.cc:75: AddWebApkInBackgroundWithSkBitmap(info, webapp_id, icon_bitmap)) { On 2016/07/10 19:25:00, pkotwicz wrote: > I have removed the fallback for now and have filed crbug.com/626950 Thanks for firing the bug!
Xi can you please take another look? https://codereview.chromium.org/2129043002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2129043002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:171: shortcutIntent.setPackage(context.getPackageName()); Reverted this part. I made the changes because you had a point :) You're right that its probably not worth making the changes as part of this CL though
lgtm! Thanks for making the revert!
pkotwicz@chromium.org changed reviewers: + dominickn@chromium.org
Dominick can you please take a look?
dominickn@ Ping!
Thanks for doing this, it looks a lot cleaner now. Mostly nits/comments/naming. https://codereview.chromium.org/2129043002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2129043002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:134: * Installs a WebAPK. Nit: "This method must not be called on the UI thread." Asserts only trigger on KK and older devices so it's good to have an explicit comment. https://codereview.chromium.org/2129043002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:142: Context context = ContextUtils.getApplicationContext(); Super nit: newline after the assert for consistency :) https://codereview.chromium.org/2129043002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:158: */ Nit: This method must not be called on the UI thread. https://codereview.chromium.org/2129043002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:161: private static void addWebappShortcut(String id, String url, String scopeUrl, Rename: "addWebapp" as per comment in shortcut_helper.h https://codereview.chromium.org/2129043002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:169: scopeUrl = getScopeFromUrl(url); This variable isn't used. Replace getScopeFromUrl(url) in the method call below with scopeUrl. https://codereview.chromium.org/2129043002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:178: // the intent. This allows us to distinguish where a shortcut was added from in metrics. This source comment is duplicated - you can probably remove it from both locations or shorten it to "The source distinguishes where a shortcut was added from in metrics". https://codereview.chromium.org/2129043002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:203: private static void addBookmarkShortcut(String url, String userTitle, Bitmap icon, int source) { Rename: "addShortcut" as per comment in shortcut_helper.h https://codereview.chromium.org/2129043002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:221: assert !ThreadUtils.runningOnUiThread(); I don't think this method needs the assert - it doesn't seem to be doing anything particularly expensive? Or does handler.post have to happen off the UI thread? https://codereview.chromium.org/2129043002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:223: // Alert the user about adding the shortcut. Remove redundant comment now this is a method. https://codereview.chromium.org/2129043002/diff/140001/chrome/browser/android... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2129043002/diff/140001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:26: // Adds a shortcut to the launcher using a SkBitmap. Nit: "The type of shortcut added depends on the properties in |info|. Calls one of AddWebApkInBackgroundWithSkBitmap, AddWebappInBackgroundWithSkBitmap, or AddBookmarkShortcutInBackgroundWithSkBitmap." Also, rename this method as per the below comment. https://codereview.chromium.org/2129043002/diff/140001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:36: static void AddWebApkInBackgroundWithSkBitmap( Perhaps InstallWebApkInBackgroundWithSkBitmap for consistency with the Java side? https://codereview.chromium.org/2129043002/diff/140001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:55: static void AddBookmarkShortcutInBackgroundWithSkBitmap( "Bookmark shortcut" is a bit misleading here because these are not equivalent to bookmarks and don't add a Chrome bookmark. How about "AddShortcutInBackgroundWithSkBitmap" instead, and the method above with that name can be "AddToLauncherInBackgroundWithSkBitmap?
Patchset #4 (id:160001) has been deleted
dominickn@ can you please take another look? https://codereview.chromium.org/2129043002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2129043002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:178: // the intent. This allows us to distinguish where a shortcut was added from in metrics. Ok, removed https://codereview.chromium.org/2129043002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:221: assert !ThreadUtils.runningOnUiThread(); Removed the assert and changed the Handler logic to use ThreadUtils#runOnUiThread() instead
lgtm
pkotwicz@chromium.org changed reviewers: + dfalcantara@chromium.org
dfalcantara@ for OWNERS rubber stamp
OWNERS review lgtm
Actually: can you ask mlamouri@ for a review, too?
(He usually remembers things I forget about shortcuts)
pkotwicz@chromium.org changed reviewers: + mlamouri@chromium.org
Dan, you are the only person I know who believes I have a decent memory :) This is basically moving the logic to split the different type of shortcuts outside of Java. It doesn't sound obviously better to me because it's adding more JNI but that's dfalcantara@'s land and he seems happy so lgtm.
The CQ bit was checked by pkotwicz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org Link to the patchset: https://codereview.chromium.org/2129043002/#ps180001 (title: "Merge branch 'master' into webapk_builder_impl0")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by pkotwicz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Split ShortcutHelper#addShortcut() into separate functions for each type of shortcut BUG=619739 ========== to ========== Split ShortcutHelper#addShortcut() into separate functions for each type of shortcut BUG=619739 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:180001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Split ShortcutHelper#addShortcut() into separate functions for each type of shortcut BUG=619739 ========== to ========== Split ShortcutHelper#addShortcut() into separate functions for each type of shortcut BUG=619739 Committed: https://crrev.com/20667314938d2a6972f2c1baaccbc9b9c43f2aa6 Cr-Commit-Position: refs/heads/master@{#406334} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/20667314938d2a6972f2c1baaccbc9b9c43f2aa6 Cr-Commit-Position: refs/heads/master@{#406334} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
