Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
304573002:
Upstreaming of FeatureUtilities (part one) (Closed)
|
Created:
6 years, 7 months ago by picksi1 Modified:
6 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionUpstreaming of FeatureUtilities (part one)
The FeatureUtilities class has been moved upstream. The original class
has been left in place; It will be removed and all package references
changed to the new version once the change has been rolled into
Clankium.
Unit Tests for FeatureUtilities.java have been added. Currently I have
added only tests for the speech recognition feature.
BUG=243060
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275524
Patch Set 1 #
Total comments: 62
Patch Set 2 : Unit tests and upstreaming of FeatureUtilities #Patch Set 3 : White space fixing #
Total comments: 30
Patch Set 4 : layout fixups #Patch Set 5 : White space/layout fixes and some work moved into the Constructor #
Total comments: 4
Patch Set 6 : Layout fixes #
Messages
Total messages: 13 (0 generated)
I've moved FeatureUtility upstream; the old downstream version is still in the code, I'll remove it and replace all references to it once the code has rolled. I've added a unit test that checks the 'isRecognitionIntentPresent' function works. There are other functions that remain untested (I currently am unsure how these can be tested).
thanks simon! mostly style nits and small suggestions below: https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... File chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java (right): https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:23: * Unit Test for FeatureUtilities.isRecognitionIntentPresent. nit: I dislike the style-checker mandating such oneliners :-/ but since it has to be here, maybe remove the ".isRecognitionIntentPresent" to allow more tests to be written. :) https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:25: nit: remove extra \n https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:30: private String mAction; nit: private final String https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:33: { nit: { goes at the end of the previous line https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:39: public List<ResolveInfo>queryIntentActivities(Intent intent, int flags) { nit: space after > https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:43: if (intent.getAction() == mAction) { nit: .equals(mAction) https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:56: private String mAction; nit: final https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:59: { nit: move { up https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:72: these annotations should be per test, not here. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:76: // Context can only be queried on a UI Thread. unindent this and the follow line https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:89: private static final boolean doNotUseCachedResult = false; nit: these would be UPPER_CASE_STYLE, i.e., USE_CACHED_RESULT, DO_NOT_USE_CACHED_RESULT https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:91: public void testSpeechFeatureAvailable() { nit: the annotations above should be for this method https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:95: RecognizerIntent.ACTION_RECOGNIZE_SPEECH ); nit: remove space before ) https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:104: public void testSpeechFeatureUnavailable() { nit: needs annotations https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:122: RecognizerIntent.ACTION_RECOGNIZE_SPEECH ); nit: this block is repeated on all tests.. probably better to have members of this class, initialized at the setUp() stage: Context mContextWithSpeech; Context mContextWithoutSpeech; @Override public void setUp() { mContextWithSpeech = new IntentTestMockContext( RecognizerIntent.ACTION_RECOGNIZE_SPEECH); mContextWithoutSpeech = new IntentTestMockContext( RecognizerIntent.ACTION_WEB_SEARCH); } https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:155: isRecognitionIntentPresent(contextWithoutSpeech, I suppose this test can be rolled in the one above.. there's a relative high-cost per test in java, so it's better to roll more test cases into one (without obviously degrading coverage / readability).
Thanks Simon, this looks pretty good. Is there a way we could test canAllowSync() too? https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... File chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java (right): https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:32: public IntentTestPackageManager(String recognisesAction) "recognizes" -- Chrome prefers US spelling :) Or should this be recognizedAction based on the functionality? https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:58: public IntentTestMockContext(String recognisesAction) Ditto about spelling. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:89: private static final boolean doNotUseCachedResult = false; On 2014/05/27 15:17:15, bulach wrote: > nit: these would be UPPER_CASE_STYLE, i.e., USE_CACHED_RESULT, > DO_NOT_USE_CACHED_RESULT Also, people generally just add a local descriptive variable at the call site (e.g., final boolean useCachedResult = false;) which they pass into the call instead of introducing constants. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:97: final boolean recognisesSpeech = recognizes https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:101: assertEquals(recognisesSpeech,true); Add space after comma. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:108: RecognizerIntent.ACTION_WEB_SEARCH ); Remove space before ")". https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:110: final boolean recognisesSpeech = Ditto about spelling. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:114: assertEquals(recognisesSpeech,false); Add space after comma. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:124: isRecognitionIntentPresent(contextWithSpeech,doNotUseCachedResult); Add space after comma. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:130: RecognizerIntent.ACTION_WEB_SEARCH ); Remove space before ")". https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:136: assertEquals(recognisesSpeech,true); Add space after comma. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:146: isRecognitionIntentPresent(contextWithSpeech,doNotUseCachedResult); Add space after comma. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:150: RecognizerIntent.ACTION_WEB_SEARCH ); No space before ")". https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:154: final boolean recognisesSpeech = Ditto about spelling. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:158: assertEquals(recognisesSpeech,false); Add space after comma.
I've fixed up everything from the first round of comments. I've also added two more tests (hasGoogleAccounts and hasGoogleAccountAuthenticator), in line with Sami's request, but have been unable to Mock hasSyncPermissions() so the tests omit this. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... File chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java (right): https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:23: * Unit Test for FeatureUtilities.isRecognitionIntentPresent. On 2014/05/27 15:17:15, bulach wrote: > nit: I dislike the style-checker mandating such oneliners :-/ but since it has > to be here, maybe remove the ".isRecognitionIntentPresent" to allow more tests > to be written. :) Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:25: On 2014/05/27 15:17:15, bulach wrote: > nit: remove extra \n Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:30: private String mAction; On 2014/05/27 15:17:15, bulach wrote: > nit: private final String Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:32: public IntentTestPackageManager(String recognisesAction) On 2014/05/27 16:17:55, Sami wrote: > "recognizes" -- Chrome prefers US spelling :) Or should this be recognizedAction > based on the functionality? Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:33: { On 2014/05/27 15:17:15, bulach wrote: > nit: { goes at the end of the previous line Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:39: public List<ResolveInfo>queryIntentActivities(Intent intent, int flags) { On 2014/05/27 15:17:15, bulach wrote: > nit: space after > Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:43: if (intent.getAction() == mAction) { On 2014/05/27 15:17:15, bulach wrote: > nit: .equals(mAction) Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:56: private String mAction; On 2014/05/27 15:17:15, bulach wrote: > nit: final Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:58: public IntentTestMockContext(String recognisesAction) On 2014/05/27 16:17:55, Sami wrote: > Ditto about spelling. Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:59: { On 2014/05/27 15:17:15, bulach wrote: > nit: move { up Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:72: On 2014/05/27 15:17:15, bulach wrote: > these annotations should be per test, not here. Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:76: // Context can only be queried on a UI Thread. I have re-indented this to read slightly better. It is a copy-n-paste from another file and is a little overly nested for my taste. Is it worth re-factoring this to use some locals to avoid all the nesting, or is this a common pattern? https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:89: private static final boolean doNotUseCachedResult = false; On 2014/05/27 15:17:15, bulach wrote: > nit: these would be UPPER_CASE_STYLE, i.e., USE_CACHED_RESULT, > DO_NOT_USE_CACHED_RESULT Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:89: private static final boolean doNotUseCachedResult = false; On 2014/05/27 16:17:55, Sami wrote: > On 2014/05/27 15:17:15, bulach wrote: > > nit: these would be UPPER_CASE_STYLE, i.e., USE_CACHED_RESULT, > > DO_NOT_USE_CACHED_RESULT > > Also, people generally just add a local descriptive variable at the call site > (e.g., final boolean useCachedResult = false;) which they pass into the call > instead of introducing constants. Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:91: public void testSpeechFeatureAvailable() { On 2014/05/27 15:17:15, bulach wrote: > nit: the annotations above should be for this method Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:95: RecognizerIntent.ACTION_RECOGNIZE_SPEECH ); On 2014/05/27 15:17:15, bulach wrote: > nit: remove space before ) Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:97: final boolean recognisesSpeech = On 2014/05/27 16:17:55, Sami wrote: > recognizes Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:101: assertEquals(recognisesSpeech,true); On 2014/05/27 16:17:55, Sami wrote: > Add space after comma. Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:104: public void testSpeechFeatureUnavailable() { On 2014/05/27 15:17:15, bulach wrote: > nit: needs annotations Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:108: RecognizerIntent.ACTION_WEB_SEARCH ); On 2014/05/27 16:17:55, Sami wrote: > Remove space before ")". Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:110: final boolean recognisesSpeech = On 2014/05/27 16:17:55, Sami wrote: > Ditto about spelling. Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:114: assertEquals(recognisesSpeech,false); On 2014/05/27 16:17:55, Sami wrote: > Add space after comma. Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:122: RecognizerIntent.ACTION_RECOGNIZE_SPEECH ); On 2014/05/27 15:17:15, bulach wrote: > nit: this block is repeated on all tests.. probably better to have members of > this class, initialized at the setUp() stage: > > Context mContextWithSpeech; > Context mContextWithoutSpeech; > > @Override > public void setUp() { > mContextWithSpeech = new IntentTestMockContext( > RecognizerIntent.ACTION_RECOGNIZE_SPEECH); > mContextWithoutSpeech = new IntentTestMockContext( > RecognizerIntent.ACTION_WEB_SEARCH); > } Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:124: isRecognitionIntentPresent(contextWithSpeech,doNotUseCachedResult); On 2014/05/27 16:17:55, Sami wrote: > Add space after comma. Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:130: RecognizerIntent.ACTION_WEB_SEARCH ); On 2014/05/27 16:17:55, Sami wrote: > Remove space before ")". Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:136: assertEquals(recognisesSpeech,true); On 2014/05/27 16:17:55, Sami wrote: > Add space after comma. Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:146: isRecognitionIntentPresent(contextWithSpeech,doNotUseCachedResult); On 2014/05/27 16:17:55, Sami wrote: > Add space after comma. Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:150: RecognizerIntent.ACTION_WEB_SEARCH ); On 2014/05/27 16:17:55, Sami wrote: > No space before ")". Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:154: final boolean recognisesSpeech = On 2014/05/27 16:17:55, Sami wrote: > Ditto about spelling. Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:155: isRecognitionIntentPresent(contextWithoutSpeech, On 2014/05/27 15:17:15, bulach wrote: > I suppose this test can be rolled in the one above.. > there's a relative high-cost per test in java, so it's better to roll more test > cases into one (without obviously degrading coverage / readability). Done. https://codereview.chromium.org/304573002/diff/1/chrome/android/javatests/src... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:158: assertEquals(recognisesSpeech,false); On 2014/05/27 16:17:55, Sami wrote: > Add space after comma. Done.
lgtm, mostly nits below. thanks! https://codereview.chromium.org/304573002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java (right): https://codereview.chromium.org/304573002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java:5: package org.chromium.chrome.browser.util; just noticed.. would webview ever need this? would it be better to put in components/ ? https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java (right): https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:39: private class IntentTestPackageManager extends MockPackageManager { nit: private static class (it doesn't access the parent object) https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:63: private class IntentTestMockContext extends MockContext { nit: static https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:78: private class MockAuthenticationAccountManager extends MockAccountManager { nit:static https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:107: new AuthenticatorDescription(mAccountType, "p1", 0, 0, 0, 0); nit: indent another +4 https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:136: mAccountTestingContext, nit: unindent by 2 (i.e., same as the 129-133 block) https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:144: new IntentTestMockContext(RecognizerIntent.ACTION_RECOGNIZE_SPEECH); nit: indent by another 4. also, I think normally is preferred to "pack" the first line and wrap at parens, so this would be something like: mContextWithSpeech = new IntentTestMockContext( ........RecognizerIntent.ACTION_RECOGNIZE_SPEECH); ditto for the following lines and some other places with =\n below
Thanks for adding the tests Simon. I think we have sufficient coverage here now. Just a few stylistic nits below. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java (right): https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:98: }; indent -4 https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:122: context, indent +1 https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:143: mContextWithSpeech = Could these be moved to the constructor? setUp() runs before every test so this is doing some redundant work. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:144: new IntentTestMockContext(RecognizerIntent.ACTION_RECOGNIZE_SPEECH); On 2014/06/03 15:05:51, bulach wrote: > nit: indent by another 4. > also, I think normally is preferred to "pack" the first line and wrap at parens, > so this would be something like: > mContextWithSpeech = new IntentTestMockContext( > ........RecognizerIntent.ACTION_RECOGNIZE_SPEECH); > > ditto for the following lines and some other places with =\n below Also, the max line length for Java code is 100 characters so I think there's some more space here to be used. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:162: isRecognitionIntentPresent(mContextWithSpeech, This could also look better if the line was broken at the "(" instead. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:174: isRecognitionIntentPresent(mContextWithoutSpeech, Ditto. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:214: mAccountTestingContext); De-indent to match line 219. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:236: assertEquals(hasAccounts, false); assertFalse would be a little more idiomatic. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:242: assertEquals(hasAuthenticator, false); assertFalse.
Fixed up all the layout issues, and changed AssertEquals to AssertTrue & AssertFalse (which look neater!). As a thought, should the pre-commit checks ensure that leading-whitespace is a multiple of 4 [or is that too severe?] - that would have many of the updates here. I couldn't move the setUp code into the constructor, it bombs out; but I moved the setUp function to the top of the class, which seems to be idiomatic. I also noticed I'd written some comments that had no fullstops on them :) https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java (right): https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:39: private class IntentTestPackageManager extends MockPackageManager { On 2014/06/03 15:05:51, bulach wrote: > nit: private static class (it doesn't access the parent object) Done. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:63: private class IntentTestMockContext extends MockContext { On 2014/06/03 15:05:51, bulach wrote: > nit: static Done. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:78: private class MockAuthenticationAccountManager extends MockAccountManager { On 2014/06/03 15:05:51, bulach wrote: > nit:static Done. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:98: }; On 2014/06/03 15:19:56, Sami wrote: > indent -4 Done. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:107: new AuthenticatorDescription(mAccountType, "p1", 0, 0, 0, 0); On 2014/06/03 15:05:51, bulach wrote: > nit: indent another +4 Done. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:122: context, On 2014/06/03 15:19:56, Sami wrote: > indent +1 Done. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:136: mAccountTestingContext, On 2014/06/03 15:05:51, bulach wrote: > nit: unindent by 2 (i.e., same as the 129-133 block) Done. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:143: mContextWithSpeech = I moved the code into the constructor but it threw an exception: java.lang.RuntimeException: Constructor thew an exception. Class: Org.chromium.chrome.browser.util.FeatureUtilitiesTest I assume there is some construction/execution order/dependency that prevents the objects being new'd up in the constructor? I've moved it back into setUp() [I also moved setUp() to the top of the class as it seems more idiomatic]. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:144: new IntentTestMockContext(RecognizerIntent.ACTION_RECOGNIZE_SPEECH); On 2014/06/03 15:05:51, bulach wrote: > nit: indent by another 4. > also, I think normally is preferred to "pack" the first line and wrap at parens, > so this would be something like: > mContextWithSpeech = new IntentTestMockContext( > ........RecognizerIntent.ACTION_RECOGNIZE_SPEECH); > > ditto for the following lines and some other places with =\n below Done. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:162: isRecognitionIntentPresent(mContextWithSpeech, On 2014/06/03 15:19:56, Sami wrote: > This could also look better if the line was broken at the "(" instead. Done. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:174: isRecognitionIntentPresent(mContextWithoutSpeech, On 2014/06/03 15:19:56, Sami wrote: > Ditto. Done. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:214: mAccountTestingContext); On 2014/06/03 15:19:56, Sami wrote: > De-indent to match line 219. Done. https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:236: assertEquals(hasAccounts, false); I've replaced all assertEquals with assertTrue or assertFalse https://codereview.chromium.org/304573002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:242: assertEquals(hasAuthenticator, false); On 2014/06/03 15:19:56, Sami wrote: > assertFalse. Done.
Layout updates done. Also moved some start up code out of setUp and into the constructor as suggested by Sami.
Thanks for bearing with us Simon. lgtm with two tiny nits. https://codereview.chromium.org/304573002/diff/70001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java (right): https://codereview.chromium.org/304573002/diff/70001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:40: { nit: This brace should go on the previous line. https://codereview.chromium.org/304573002/diff/70001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:52: { Ditto.
All changes made. Will commit the code now. https://codereview.chromium.org/304573002/diff/70001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java (right): https://codereview.chromium.org/304573002/diff/70001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:40: { On 2014/06/05 11:08:42, Sami wrote: > nit: This brace should go on the previous line. Done. https://codereview.chromium.org/304573002/diff/70001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/util/FeatureUtilitiesTest.java:52: { On 2014/06/05 11:08:42, Sami wrote: > Ditto. Done.
The CQ bit was checked by picksi@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/picksi@chromium.org/304573002/90001
Message was sent while issue was closed.
Change committed as 275524 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
