Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1538573004:
Expose a Java API for base/feature_list.h. (Closed)
|
Created:
5 years ago by Alexei Svitkine (slow) Modified:
5 years ago CC:
chromium-reviews, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExpose a Java API for base/feature_list.h.
Because features are decentralized but Java code has to refer to
them by name, features exposed to Java code need to be added to
the kFeaturesExposedToJava array in base/android/feature_list.cc.
BUG=570461
Committed: https://crrev.com/cc3a08d12b7b00c1efe89d9e2fd59bff4e33d78b
Cr-Commit-Position: refs/heads/master@{#366231}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : #
Total comments: 5
Patch Set 3 : Add namespace. #
Messages
Total messages: 35 (16 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #1 (id:120001) has been deleted
asvitkine@chromium.org changed reviewers: + cco3@chromium.org, nyquist@chromium.org
Here's my proposed Java API for feature_list.h. PTAL, cco3, can you try it with your CL to make sure everything is working as expected?
On 2015/12/18 17:59:36, Alexei Svitkine (slow) wrote: > Here's my proposed Java API for feature_list.h. > > PTAL, > > cco3, can you try it with your CL to make sure everything is working as > expected? OK, I know Gerrit is cool with me rebasing on top of changes and then uploading...I'll assume this codereview tool is as well...
https://codereview.chromium.org/1538573004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java (left): https://codereview.chromium.org/1538573004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. Is this change intended?
https://codereview.chromium.org/1538573004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java (left): https://codereview.chromium.org/1538573004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/12/18 18:08:03, cco3 wrote: > Is this change intended? Nope! Removed.
LGTM. It seems to work (it doesn't crash), although I'm not entirely sure how I'm supposed to check for the flag, if I'm supposed to continue doing that distinctly through ChromeSwitches or if I'm supposed to get that for free.
With the new approach, you shouldn't need a separate command line flag. You would just check the feature state using its name. For about_flags.cc, you can add an entry via FEATURE_VALUE_TYPE() macro. On Fri, Dec 18, 2015 at 4:33 PM, <cco3@chromium.org> wrote: > LGTM. It seems to work (it doesn't crash), although I'm not entirely sure > how > I'm supposed to check for the flag, if I'm supposed to continue doing that > distinctly through ChromeSwitches or if I'm supposed to get that for free. > > https://codereview.chromium.org/1538573004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Works great!
The CQ bit was checked by asvitkine@chromium.org
Chatted with Conley offline and he was able to test this to ensure everything was working as expected. Going ahead to cq it.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1538573004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1538573004/180001
https://codereview.chromium.org/1538573004/diff/180001/chrome/browser/android... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/1538573004/diff/180001/chrome/browser/android... chrome/browser/android/chrome_feature_list.cc:30: static jboolean IsEnabled(JNIEnv* env, Why is this not namespaces to chrome::android?
https://codereview.chromium.org/1538573004/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java (right): https://codereview.chromium.org/1538573004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java:13: public abstract class ChromeFeatureList { I don't really get why this is in //chrome and not //base?
The CQ bit was unchecked by asvitkine@chromium.org
https://codereview.chromium.org/1538573004/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java (right): https://codereview.chromium.org/1538573004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java:13: public abstract class ChromeFeatureList { On 2015/12/18 22:49:22, nyquist wrote: > I don't really get why this is in //chrome and not //base? Chatted with nyquist@ offline, but the answer is that the .cc code may need access to features define in chrome/, components/, etc and thus can't be in base. https://codereview.chromium.org/1538573004/diff/180001/chrome/browser/android... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/1538573004/diff/180001/chrome/browser/android... chrome/browser/android/chrome_feature_list.cc:30: static jboolean IsEnabled(JNIEnv* env, On 2015/12/18 22:47:39, nyquist wrote: > Why is this not namespaces to chrome::android? I tried this originally, but seems the header definition generated in ChromeFeatureList_jni.h wants it to be in the global namespace. Not sure if there's some trick I'm missing?
https://codereview.chromium.org/1538573004/diff/180001/chrome/browser/android... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/1538573004/diff/180001/chrome/browser/android... chrome/browser/android/chrome_feature_list.cc:30: static jboolean IsEnabled(JNIEnv* env, On 2015/12/18 22:54:45, Alexei Svitkine (slow) wrote: > On 2015/12/18 22:47:39, nyquist wrote: > > Why is this not namespaces to chrome::android? > > I tried this originally, but seems the header definition generated in > ChromeFeatureList_jni.h wants it to be in the global namespace. Not sure if > there's some trick I'm missing? Looks like I was missing the @JNINamespace annotation. Done!
thanks for fixing this so quickly. lgtm
The CQ bit was checked by asvitkine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cco3@chromium.org Link to the patchset: https://codereview.chromium.org/1538573004/#ps190001 (title: "Add namespace.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1538573004/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1538573004/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by asvitkine@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1538573004/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1538573004/190001
Message was sent while issue was closed.
Committed patchset #3 (id:190001)
Message was sent while issue was closed.
Description was changed from ========== Expose a Java API for base/feature_list.h. Because features are decentralized but Java code has to refer to them by name, features exposed to Java code need to be added to the kFeaturesExposedToJava array in base/android/feature_list.cc. BUG=570461 ========== to ========== Expose a Java API for base/feature_list.h. Because features are decentralized but Java code has to refer to them by name, features exposed to Java code need to be added to the kFeaturesExposedToJava array in base/android/feature_list.cc. BUG=570461 Committed: https://crrev.com/cc3a08d12b7b00c1efe89d9e2fd59bff4e33d78b Cr-Commit-Position: refs/heads/master@{#366231} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cc3a08d12b7b00c1efe89d9e2fd59bff4e33d78b Cr-Commit-Position: refs/heads/master@{#366231} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
