Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
563843002:
Refactor ShortcutHelper (Java/C++) to allow asynchronous init. (Closed)
|
Created:
6 years, 3 months ago by mlamouri (slow - plz ping) Modified:
6 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@manifest_manager_content Project:
chromium Visibility:
Public. |
DescriptionRefactor ShortcutHelper (Java/C++) to allow asynchronous init.
This is also changing some design with the C++ instance being owned by
its Java counterpart. ShortcutBuilder is also removed.
BUG=None
Committed: https://crrev.com/ecb9b408534ca2d4dde5b326b440605a7232a59c
Cr-Commit-Position: refs/heads/master@{#294578}
Patch Set 1 #
Total comments: 13
Patch Set 2 : review comments #
Total comments: 6
Patch Set 3 : #
Total comments: 7
Patch Set 4 : bauerb comments #Patch Set 5 : #
Messages
Total messages: 24 (6 generated)
mlamouri@chromium.org changed reviewers: + dfalcantara@chromium.org
mlamouri@chromium.org changed reviewers: + miguelg@chromium.org
Dan, could you review the changes in ShortcutHelper? The intent is to be able to initialize the object with a short delay. Miguel, could review this too given that Dan isn't an owner?
Bunch of nits, but lgtm. If you could try running the ShortcutHelperTests (disabled a while back), or update crbug.com/303486 to indicate that they may need to be rewritten, I'd appreciate it. https://chromiumcodereview.appspot.com/563843002/diff/1/chrome/android/java/s... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://chromiumcodereview.appspot.com/563843002/diff/1/chrome/android/java/s... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:47: private OnInitialized mCallback = null; these are all initialized by default to the values you set. don't need to make them explicit. https://chromiumcodereview.appspot.com/563843002/diff/1/chrome/android/java/s... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:49: private Tab mTab = null; private final Context mAppContext; private final Tab mTab; move above regular private fields https://chromiumcodereview.appspot.com/563843002/diff/1/chrome/android/java/s... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:50: private boolean mInitialized = false; nit: private boolean mIsInitialized; https://chromiumcodereview.appspot.com/563843002/diff/1/chrome/android/java/s... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:71: public boolean initialized() { isInitialized() https://chromiumcodereview.appspot.com/563843002/diff/1/chrome/android/java/s... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:91: if (mCallback == null) { just make this: if (mCallback != null) mCallback.onInitialized(title); https://chromiumcodereview.appspot.com/563843002/diff/1/chrome/browser/androi... File chrome/browser/android/shortcut_helper.cc (right): https://chromiumcodereview.appspot.com/563843002/diff/1/chrome/browser/androi... chrome/browser/android/shortcut_helper.cc:103: JNIEnv* env, jobject, jstring jtitle, jint launcher_large_icon_size) { nit: each variable on its own line
Thanks for the quick review. Comments applied with updated tests. FWIW, I ran them a couple of time and did not perceive any flakyness. https://codereview.chromium.org/563843002/diff/1/chrome/android/java/src/org/... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/563843002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:47: private OnInitialized mCallback = null; On 2014/09/11 18:24:36, dfalcantara wrote: > these are all initialized by default to the values you set. don't need to make > them explicit. I actually prefer explicit init but I've updated this. https://codereview.chromium.org/563843002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:49: private Tab mTab = null; On 2014/09/11 18:24:36, dfalcantara wrote: > private final Context mAppContext; > private final Tab mTab; > > move above regular private fields Done. https://codereview.chromium.org/563843002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:50: private boolean mInitialized = false; On 2014/09/11 18:24:36, dfalcantara wrote: > nit: private boolean mIsInitialized; Done. https://codereview.chromium.org/563843002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:71: public boolean initialized() { On 2014/09/11 18:24:36, dfalcantara wrote: > isInitialized() Done. https://codereview.chromium.org/563843002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:91: if (mCallback == null) { On 2014/09/11 18:24:36, dfalcantara wrote: > just make this: > > if (mCallback != null) mCallback.onInitialized(title); Done. https://codereview.chromium.org/563843002/diff/1/chrome/browser/android/short... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/563843002/diff/1/chrome/browser/android/short... chrome/browser/android/shortcut_helper.cc:103: JNIEnv* env, jobject, jstring jtitle, jint launcher_large_icon_size) { On 2014/09/11 18:24:37, dfalcantara wrote: > nit: each variable on its own line Done.
Still lgtm https://chromiumcodereview.appspot.com/563843002/diff/1/chrome/android/java/s... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://chromiumcodereview.appspot.com/563843002/diff/1/chrome/android/java/s... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:47: private OnInitialized mCallback = null; On 2014/09/11 19:31:02, Mounir Lamouri wrote: > On 2014/09/11 18:24:36, dfalcantara wrote: > > these are all initialized by default to the values you set. don't need to > make > > them explicit. > > I actually prefer explicit init but I've updated this. Eh, AFAIK it's been the standard to not define these if they are meant to have the default value, at least in the downstream tree. Can't see it in the Android style guide though, so maybe it's just convention. https://chromiumcodereview.appspot.com/563843002/diff/20001/chrome/android/ja... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://chromiumcodereview.appspot.com/563843002/diff/20001/chrome/android/ja... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:51: private boolean mIsInitialized = false; defaults to false https://chromiumcodereview.appspot.com/563843002/diff/20001/chrome/android/ja... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:52: private long mNativeShortcutHelper = 0; defaults to 0 https://chromiumcodereview.appspot.com/563843002/diff/20001/chrome/browser/an... File chrome/browser/android/shortcut_helper.cc (right): https://chromiumcodereview.appspot.com/563843002/diff/20001/chrome/browser/an... chrome/browser/android/shortcut_helper.cc:103: JNIEnv* env, jobject, missed jobject
https://chromiumcodereview.appspot.com/563843002/diff/20001/chrome/android/ja... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://chromiumcodereview.appspot.com/563843002/diff/20001/chrome/android/ja... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:51: private boolean mIsInitialized = false; On 2014/09/11 20:32:21, dfalcantara wrote: > defaults to false Interesting. https://chromiumcodereview.appspot.com/563843002/diff/20001/chrome/android/ja... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:52: private long mNativeShortcutHelper = 0; On 2014/09/11 20:32:21, dfalcantara wrote: > defaults to 0 Done. https://chromiumcodereview.appspot.com/563843002/diff/20001/chrome/browser/an... File chrome/browser/android/shortcut_helper.cc (right): https://chromiumcodereview.appspot.com/563843002/diff/20001/chrome/browser/an... chrome/browser/android/shortcut_helper.cc:103: JNIEnv* env, jobject, On 2014/09/11 20:32:21, dfalcantara wrote: > missed jobject Done.
Rubber stamp Owners LGTM. Dan I think you should be in the Owners file for this stuff as well.
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/563843002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mlamouri@chromium.org changed reviewers: + bauerb@chromium.org
Bernhard, can you have a look? Retvield tricked me in thinking that miguelg was owning the java file.
He's not? It also looks that way to me... https://codereview.chromium.org/563843002/diff/40001/chrome/browser/android/s... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/563843002/diff/40001/chrome/browser/android/s... chrome/browser/android/shortcut_helper.cc:56: java_ref_.reset(); This is unnecessary. https://codereview.chromium.org/563843002/diff/40001/chrome/browser/android/s... chrome/browser/android/shortcut_helper.cc:94: if (cancelable_task_tracker_.HasTrackedTasks()) { This is unnecessary too. https://codereview.chromium.org/563843002/diff/40001/chrome/browser/android/s... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/563843002/diff/40001/chrome/browser/android/s... chrome/browser/android/shortcut_helper.h:39: explicit ShortcutHelper(JNIEnv* env, Explicit is only necessary for single-argument constructors.
PTAL https://codereview.chromium.org/563843002/diff/40001/chrome/browser/android/s... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/563843002/diff/40001/chrome/browser/android/s... chrome/browser/android/shortcut_helper.cc:56: java_ref_.reset(); On 2014/09/12 11:30:48, Bernhard Bauer wrote: > This is unnecessary. Done. https://codereview.chromium.org/563843002/diff/40001/chrome/browser/android/s... chrome/browser/android/shortcut_helper.cc:94: if (cancelable_task_tracker_.HasTrackedTasks()) { On 2014/09/12 11:30:48, Bernhard Bauer wrote: > This is unnecessary too. Done. https://codereview.chromium.org/563843002/diff/40001/chrome/browser/android/s... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/563843002/diff/40001/chrome/browser/android/s... chrome/browser/android/shortcut_helper.h:39: explicit ShortcutHelper(JNIEnv* env, On 2014/09/12 11:30:48, Bernhard Bauer wrote: > Explicit is only necessary for single-argument constructors. Fixed. It used to be one argument. Forgot to update.
https://codereview.chromium.org/563843002/diff/40001/chrome/browser/android/s... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/563843002/diff/40001/chrome/browser/android/s... chrome/browser/android/shortcut_helper.cc:94: if (cancelable_task_tracker_.HasTrackedTasks()) { On 2014/09/12 11:48:43, Mounir Lamouri wrote: > On 2014/09/12 11:30:48, Bernhard Bauer wrote: > > This is unnecessary too. > > Done. Sorry, the whole thing is unnecessary. |cancelable_task_tracker_| will cancel all tasks when it's destroyed (that's what it's for), and |java_ref_| will also reset itself when destroyed. You only need the delete in this method. Sorry for being unclear.
PTAL
LGTM!
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/563843002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as 933bbb25d3adb604313a6ba1749f8f3b8bb972bf
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ecb9b408534ca2d4dde5b326b440605a7232a59c Cr-Commit-Position: refs/heads/master@{#294578} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
