Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2845803003:
Upstream ChromeShortcutManagerInternal using reflection. (Closed)
|
Created:
3 years, 8 months ago by Marti Wong Modified:
3 years, 7 months ago CC:
chromium-reviews, dominickn+watch_chromium.org, pkotwicz+watch_chromium.org, zpeng+watch_chromium.org, agrieve+watch_chromium.org, Ted C Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpstream ChromeShortcutManagerInternal using reflection.
This CL upstreams the internal Android O code using reflection.
I learned from the 2 upstreaming examples below:
crrev.com/2834633002/
crrev.com/2765003002/
Added some TODOs for removing those reflection when compileSdk >=O
BUG=708360
Review-Url: https://codereview.chromium.org/2845803003
Cr-Commit-Position: refs/heads/master@{#468523}
Committed: https://chromium.googlesource.com/chromium/src/+/37235f0ea4c66711ae8cc694978bfb98657dd0c6
Patch Set 1 #Patch Set 2 : Fix a mistake in comment #
Total comments: 17
Patch Set 3 : fix #Patch Set 4 : fix #
Total comments: 4
Patch Set 5 : fix #Patch Set 6 : fix #
Messages
Total messages: 20 (11 generated)
Description was changed from ========== Upstream ChromeShortcutManagerInternal using reflection. This CL upstreams the internal Android O code using reflection. I learned from the 2 upstreaming examples below: crrev.com/2834633002/ crrev.com/2765003002/ Added some TODOs for removing those reflection when compileSdk >=O BUG=708360 ========== to ========== Upstream ChromeShortcutManagerInternal using reflection. This CL upstreams the internal Android O code using reflection. I learned from the 2 upstreaming examples below: crrev.com/2834633002/ crrev.com/2765003002/ Added some TODOs for removing those reflection when compileSdk >=O BUG=708360 ==========
martiw@chromium.org changed reviewers: + dfalcantara@chromium.org, dominickn@chromium.org
PTAL. 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...
Thanks Marti! Mostly minor nits. https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java (right): https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:32: private static final String TAG = "ChromeShortcutMgr"; Nit: make this the full "ChromeShortcutManager". https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:46: /* Returns the singleton instance of ChromeShortcutManager, creating it if needed. */ Nit: while we're here this should be a Javadoc style comment: /** * Returns the singleton instance of ChromeShortcutManager, creating it if needed. */ https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:49: sInstance = AppHooks.get().createChromeShortcutManager(); Instead of AppHooks, can we just call new ChromeShortcutManager() now? https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:69: public ChromeShortcutManager() { Nit: can this be private? Everything should use the singleton instance. https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:78: /* Nit: use leading // for comments here (reserve /* for Javadoc style annotations) https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:88: // mShortcutManager = Nit: remove the commented out code here? I think it's mostly confusing because you have the full lines in the comment above https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:127: The code in the try-block uses reflection in order to compile as it calls APIs newer than Nit: use leading // instead of /* */ here. https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:146: // shortcutBuilder.setShortLabel(title) Nit: similarly I'd remove each of the individual lines because it's in the comment above.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dfalcantara@chromium.org changed reviewers: + tedchoc@chromium.org
Ted: Are we supposed to be upstreaming any O code until upstream builds with the final SDK? Don't know what the policy is.
if it is good to upstream this, PTAL. thx~! https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java (right): https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:32: private static final String TAG = "ChromeShortcutMgr"; On 2017/04/27 06:38:38, dominickn wrote: > Nit: make this the full "ChromeShortcutManager". When doing "cl upload", presubmit said: "tag length is restricted at most 20 chars." That's why the shortform :P https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:46: /* Returns the singleton instance of ChromeShortcutManager, creating it if needed. */ On 2017/04/27 06:38:38, dominickn wrote: > Nit: while we're here this should be a Javadoc style comment: > > /** > * Returns the singleton instance of ChromeShortcutManager, creating it if > needed. > */ Done. https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:49: sInstance = AppHooks.get().createChromeShortcutManager(); Done. thx https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:69: public ChromeShortcutManager() { On 2017/04/27 06:38:38, dominickn wrote: > Nit: can this be private? Everything should use the singleton instance. I cannot do that by now coz it'll break ChromeShortcutManagerInternal which overrides this contructor. Added a TODO instead. https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:78: /* On 2017/04/27 06:38:38, dominickn wrote: > Nit: use leading // for comments here (reserve /* for Javadoc style annotations) Done. https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:88: // mShortcutManager = On 2017/04/27 06:38:38, dominickn wrote: > Nit: remove the commented out code here? I think it's mostly confusing because > you have the full lines in the comment above Done. https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:127: The code in the try-block uses reflection in order to compile as it calls APIs newer than On 2017/04/27 06:38:38, dominickn wrote: > Nit: use leading // instead of /* */ here. Done. https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:146: // shortcutBuilder.setShortLabel(title) On 2017/04/27 06:38:38, dominickn wrote: > Nit: similarly I'd remove each of the individual lines because it's in the > comment above. Done.
lgtm, thanks! https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java (right): https://codereview.chromium.org/2845803003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:32: private static final String TAG = "ChromeShortcutMgr"; On 2017/04/28 05:09:04, Marti Wong wrote: > On 2017/04/27 06:38:38, dominickn wrote: > > Nit: make this the full "ChromeShortcutManager". > > When doing "cl upload", presubmit said: > "tag length is restricted at most 20 chars." > That's why the shortform :P Acknowledged.
lgtm https://codereview.chromium.org/2845803003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java (right): https://codereview.chromium.org/2845803003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:39: // True when Android O's ShortcutManager.requestPinShortcut() is supported End comments with periods. https://codereview.chromium.org/2845803003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:86: // TODO(martiw) Remove the following reflection once compileSdk is bumped to O. be consistent about using the colon. TODO(martiw):
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, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2845803003/#ps100001 (title: "fix")
Thanks for the review~! commiting. https://codereview.chromium.org/2845803003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java (right): https://codereview.chromium.org/2845803003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:39: // True when Android O's ShortcutManager.requestPinShortcut() is supported On 2017/04/30 02:03:15, slow (dfalcantara) wrote: > End comments with periods. Done. https://codereview.chromium.org/2845803003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeShortcutManager.java:86: // TODO(martiw) Remove the following reflection once compileSdk is bumped to O. On 2017/04/30 02:03:15, slow (dfalcantara) wrote: > be consistent about using the colon. > > TODO(martiw): Done.
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": 1493684421465400,
"parent_rev": "1d37839e54566033090fbb82f89dd1dba04c3621", "commit_rev":
"37235f0ea4c66711ae8cc694978bfb98657dd0c6"}
Message was sent while issue was closed.
Description was changed from ========== Upstream ChromeShortcutManagerInternal using reflection. This CL upstreams the internal Android O code using reflection. I learned from the 2 upstreaming examples below: crrev.com/2834633002/ crrev.com/2765003002/ Added some TODOs for removing those reflection when compileSdk >=O BUG=708360 ========== to ========== Upstream ChromeShortcutManagerInternal using reflection. This CL upstreams the internal Android O code using reflection. I learned from the 2 upstreaming examples below: crrev.com/2834633002/ crrev.com/2765003002/ Added some TODOs for removing those reflection when compileSdk >=O BUG=708360 Review-Url: https://codereview.chromium.org/2845803003 Cr-Commit-Position: refs/heads/master@{#468523} Committed: https://chromium.googlesource.com/chromium/src/+/37235f0ea4c66711ae8cc694978b... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/37235f0ea4c66711ae8cc694978b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
