Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1603383003:
color, package and action bar configuration. (Closed)
|
Created:
4 years, 11 months ago by BigBossZhiling Modified:
4 years, 3 months ago Reviewers:
Ian Wen Base URL:
https://github.com/GoogleChrome/custom-tabs-client.git@master Target Ref:
refs/heads/master Project:
custom-tabs-client Visibility:
Public. |
Descriptioncolor, package and action bar configuration.
BUG=
Patch Set 1 #Patch Set 2 : Moved everything to demos. #Patch Set 3 : small fixes #Patch Set 4 : Included the spinner row xml file into the commit. #Patch Set 5 : Added package configuration and created a new advanced ui setting activity. #Patch Set 6 : Added action bar configuration item. #
Total comments: 52
Patch Set 7 : Address issues. #Patch Set 8 : fixes #
Total comments: 39
Patch Set 9 : address issues #
Total comments: 2
Patch Set 10 : fixes #
Total comments: 45
Patch Set 11 : fixes #
Total comments: 14
Messages
Total messages: 23 (7 generated)
Description was changed from ========== Added color configuration for tool bar. BUG= ========== to ========== Added color configuration for tool bar. BUG= ==========
hzl@google.com changed reviewers: + yusufo@chromium.org
Description was changed from ========== Added color configuration for tool bar. BUG= ========== to ========== Added color configuration of custom tab client for tool bar. BUG= ==========
hzl@google.com changed reviewers: + ianwen@chromium.org
Deleted the changes in application and added color configuration for tool bar in demos.
Deleted the changes in application and added color configuration for tool bar in demos.
Deleted the changes in application and added color configuration for tool bar in demos.
Description was changed from ========== Added color configuration of custom tab client for tool bar. BUG= ========== to ========== Created advanced ui setting page and added color,package configuration. BUG= ==========
https://codereview.chromium.org/1603383003/diff/100001/customtabs/src/android... File customtabs/src/android/support/customtabs/CustomTabsIntent.java (right): https://codereview.chromium.org/1603383003/diff/100001/customtabs/src/android... customtabs/src/android/support/customtabs/CustomTabsIntent.java:273: * Probably I should take this away.
Description was changed from ========== Created advanced ui setting page and added color,package configuration. BUG= ========== to ========== color, package and action bar configuration. BUG= ==========
ianwen@chromium.org changed reviewers: - yusufo@chromium.org
Lot's of comments, most of which you can check the code style guide here. https://source.android.com/source/code-style.html javadoc for public methods is a must, unused code is prohibited, static methods go first, public methods go before private ones. https://codereview.chromium.org/1603383003/diff/100001/demos/build.gradle File demos/build.gradle (right): https://codereview.chromium.org/1603383003/diff/100001/demos/build.gradle#new... demos/build.gradle:27: //compile 'com.android.support:customtabs:23.1.1' Unused/commented code is not permitted in clankium. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/Android... File demos/src/main/AndroidManifest.xml (right): https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/Android... demos/src/main/AndroidManifest.xml:22: /> Is this correctly formatted? https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/Android... demos/src/main/AndroidManifest.xml:66: android:launchMode="singleTop"> Why making it singletop? https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... File demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java (right): https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:1: // Copyright 2015 Google Inc. All Rights Reserved. 2016 https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:39: * Advanced UI setting. An activity for advanced UI settings. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:48: private String color; All memeber variables should start with an "m". https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:49: private Dialog addActionBarItemDialogue; Use Dialog instead of Dialogue. Our names are long enough. ;) https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:51: private int PICK_IMAGE_REQUEST = 1; Make this a static final and move it before dynamic variables. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:52: private Uri imageInAddItemDialogue; This is a uri, not a dialog. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:65: packageNameList = (List<String>) getIntent().getSerializableExtra("packageNameList"); Make these strings (eg "packageNameList") public constants of this class. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:80: mPackageSpinner.setAdapter(new ArrayAdapter<String>(this, R.layout.row, Use android.R.layout.simple_spinner_dropdown_item instead of writing your own. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:134: // Put uri, instead of Bitmap in intent to save space. Switch to javadoc and use "Puts" instead of "put". Same for all other javadocs. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:135: static Bitmap uriToBitMap(Context context, Uri uri) { Static methods should be placed before dynamic methods, just like static variables placed before dynamic ones. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:144: * Display of items stored in the arraylist from index startIndex to endInde. Displays items...... https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:144: * Display of items stored in the arraylist from index startIndex to endInde. endInde. Typo. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:166: setting.addView(item, startPosition); You can directly say addView, without giving it a position. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:168: Remove empty line. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:203: private void addActionBarItemDialogue() { This method shows a dialog, not "add". I would use a different method name. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/lay... File demos/src/main/res/layout/activity_custom_ui.xml (right): https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/lay... demos/src/main/res/layout/activity_custom_ui.xml:64: android:layout_height="@dimen/view_height"> Why change the layout's height? https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/lay... File demos/src/main/res/layout/dialogue_add_actionbar_item.xml (right): https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/lay... demos/src/main/res/layout/dialogue_add_actionbar_item.xml:6: android:paddingLeft="@dimen/activity_horizontal_margin" IIRC you can just use android:padding, instead of four duplicate lines. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/lay... demos/src/main/res/layout/dialogue_add_actionbar_item.xml:50: <LinearLayout What is the point of nesting a single button in a linear layout? https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/lay... File demos/src/main/res/layout/display_action_bar_item_setting.xml (right): https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/lay... demos/src/main/res/layout/display_action_bar_item_setting.xml:27: android:id="@+id/item_delete" If there is only one item in a layout, remove that layout and directly use the item. Same for all other UI elements in xml files. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/lay... File demos/src/main/res/layout/row.xml (right): https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/lay... demos/src/main/res/layout/row.xml:2: <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" Remove this file. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/val... File demos/src/main/res/values/dimens.xml (right): https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/val... demos/src/main/res/values/dimens.xml:20: <dimen name="view_height">50sp</dimen> Which view's height? Dimen's should not be named too vaguely. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/val... demos/src/main/res/values/dimens.xml:21: <dimen name="button_width">300sp</dimen> Usually you don't need to specify the button's width or height. Wrap content would just do the magic. Why do you need it here? https://codereview.chromium.org/1603383003/diff/100001/shared/src/main/java/o... File shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java (right): https://codereview.chromium.org/1603383003/diff/100001/shared/src/main/java/o... shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java:103: public static String getPackageNameToUse(Context context) { javadoc. /** @see getPackageNameToUse(Context, List) **/. Every public methods or public attributes must have javadoc.
https://codereview.chromium.org/1603383003/diff/100001/demos/build.gradle File demos/build.gradle (right): https://codereview.chromium.org/1603383003/diff/100001/demos/build.gradle#new... demos/build.gradle:27: //compile 'com.android.support:customtabs:23.1.1' On 2016/02/09 00:14:15, Ian Wen wrote: > Unused/commented code is not permitted in clankium. Done. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/Android... File demos/src/main/AndroidManifest.xml (right): https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/Android... demos/src/main/AndroidManifest.xml:22: /> On 2016/02/09 00:14:15, Ian Wen wrote: > Is this correctly formatted? Done. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/Android... demos/src/main/AndroidManifest.xml:66: android:launchMode="singleTop"> On 2016/02/09 00:14:15, Ian Wen wrote: > Why making it singletop? After coming back from advanced ui setting, I want the user to go back to the exact same custom ui activity that the user was on. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... File demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java (right): https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:1: // Copyright 2015 Google Inc. All Rights Reserved. On 2016/02/09 00:14:15, Ian Wen wrote: > 2016 Done. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:48: private String color; On 2016/02/09 00:14:15, Ian Wen wrote: > All memeber variables should start with an "m". Done. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:49: private Dialog addActionBarItemDialogue; On 2016/02/09 00:14:15, Ian Wen wrote: > Use Dialog instead of Dialogue. Our names are long enough. ;) Done. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:51: private int PICK_IMAGE_REQUEST = 1; On 2016/02/09 00:14:15, Ian Wen wrote: > Make this a static final and move it before dynamic variables. Done. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:52: private Uri imageInAddItemDialogue; On 2016/02/09 00:14:15, Ian Wen wrote: > This is a uri, not a dialog. Done. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:65: packageNameList = (List<String>) getIntent().getSerializableExtra("packageNameList"); On 2016/02/09 00:14:15, Ian Wen wrote: > Make these strings (eg "packageNameList") public constants of this class. Done. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:80: mPackageSpinner.setAdapter(new ArrayAdapter<String>(this, R.layout.row, On 2016/02/09 00:14:15, Ian Wen wrote: > Use android.R.layout.simple_spinner_dropdown_item instead of writing your own. I tried R.layout.simple_spinner_dropdown_item, but I think R.layout.simple_spinner_dropdown_item aligns elements to the left, but I want the text to align to the right. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:134: // Put uri, instead of Bitmap in intent to save space. On 2016/02/09 00:14:15, Ian Wen wrote: > Switch to javadoc and use "Puts" instead of "put". Same for all other javadocs. Done. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:135: static Bitmap uriToBitMap(Context context, Uri uri) { On 2016/02/09 00:14:15, Ian Wen wrote: > Static methods should be placed before dynamic methods, just like static > variables placed before dynamic ones. Done. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:144: * Display of items stored in the arraylist from index startIndex to endInde. On 2016/02/09 00:14:16, Ian Wen wrote: > endInde. Typo. Done. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:144: * Display of items stored in the arraylist from index startIndex to endInde. On 2016/02/09 00:14:16, Ian Wen wrote: > Displays items...... Done. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:166: setting.addView(item, startPosition); On 2016/02/09 00:14:15, Ian Wen wrote: > You can directly say addView, without giving it a position. Yes I can, but I think I don't want to add the views at the bottom of the layout. I want to add these action bar items before the "save" and "add action bar item" button. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:168: On 2016/02/09 00:14:15, Ian Wen wrote: > Remove empty line. Done. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:203: private void addActionBarItemDialogue() { On 2016/02/09 00:14:15, Ian Wen wrote: > This method shows a dialog, not "add". I would use a different method name. Done. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/lay... File demos/src/main/res/layout/activity_custom_ui.xml (right): https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/lay... demos/src/main/res/layout/activity_custom_ui.xml:64: android:layout_height="@dimen/view_height"> On 2016/02/09 00:14:16, Ian Wen wrote: > Why change the layout's height? I think it will make the height of each row more uniform. Otherwise, for example, rows with checkbox and spinner, if using the wrap_content height, will have different heights. I tried it and I don't think it looks good. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/lay... File demos/src/main/res/layout/dialogue_add_actionbar_item.xml (right): https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/lay... demos/src/main/res/layout/dialogue_add_actionbar_item.xml:6: android:paddingLeft="@dimen/activity_horizontal_margin" On 2016/02/09 00:14:16, Ian Wen wrote: > IIRC you can just use android:padding, instead of four duplicate lines. Done. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/val... File demos/src/main/res/values/dimens.xml (right): https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/val... demos/src/main/res/values/dimens.xml:20: <dimen name="view_height">50sp</dimen> On 2016/02/09 00:14:16, Ian Wen wrote: > Which view's height? Dimen's should not be named too vaguely. This literally is the height of all views except the image. Each row, no matter whether it is spinner, checkbox, button, they all have this height. I was thinking that it should look more uniform. https://codereview.chromium.org/1603383003/diff/100001/demos/src/main/res/val... demos/src/main/res/values/dimens.xml:21: <dimen name="button_width">300sp</dimen> On 2016/02/09 00:14:16, Ian Wen wrote: > Usually you don't need to specify the button's width or height. Wrap content > would just do the magic. Why do you need it here? For example, for 2 buttons whose text are of different length, I think if I do wrap content, then the buttons will have different widths. https://codereview.chromium.org/1603383003/diff/100001/shared/src/main/java/o... File shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java (right): https://codereview.chromium.org/1603383003/diff/100001/shared/src/main/java/o... shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java:103: public static String getPackageNameToUse(Context context) { On 2016/02/09 00:14:16, Ian Wen wrote: > javadoc. /** @see getPackageNameToUse(Context, List) **/. Every public methods > or public attributes must have javadoc. Done.
Looks better. Still some issues. :) 1. Empty @return or @params in javadoc is not allowed. 2. This demo's main concern is demonstrating CCT configuration, and we should favor simplicity rather than aesthetics. https://chromiumcodereview.appspot.com/1603383003/diff/100001/demos/src/main/... File demos/src/main/AndroidManifest.xml (right): https://chromiumcodereview.appspot.com/1603383003/diff/100001/demos/src/main/... demos/src/main/AndroidManifest.xml:66: android:launchMode="singleTop"> On 2016/02/09 01:03:11, BigBossZhiling wrote: > On 2016/02/09 00:14:15, Ian Wen wrote: > > Why making it singletop? > > After coming back from advanced ui setting, I want the user to go back to the > exact same custom ui activity that the user was on. If you use startActivityForResult() in CustomUIActivity and and receive result later. This way you do not need to set this activity to be singleTop. See http://stackoverflow.com/questions/10407159/how-to-manage-startactivityforres... https://chromiumcodereview.appspot.com/1603383003/diff/100001/demos/src/main/... File demos/src/main/java/org/chromium/customtabsdemos/Camera.java (right): https://chromiumcodereview.appspot.com/1603383003/diff/100001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/Camera.java:7: public class Camera extends AppCompatActivity { 1. Every public class must have javadoc. 2. Every new source file must have copyright info. https://chromiumcodereview.appspot.com/1603383003/diff/100001/demos/src/main/... File demos/src/main/res/layout/activity_custom_ui.xml (right): https://chromiumcodereview.appspot.com/1603383003/diff/100001/demos/src/main/... demos/src/main/res/layout/activity_custom_ui.xml:64: android:layout_height="@dimen/view_height"> On 2016/02/09 01:03:12, BigBossZhiling wrote: > On 2016/02/09 00:14:16, Ian Wen wrote: > > Why change the layout's height? > > I think it will make the height of each row more uniform. Otherwise, for > example, rows with checkbox and spinner, if using the wrap_content height, will > have different heights. I tried it and I don't think it looks good. If you want to make them look good, in general you should fall back to android's default size (which is wrap content). In this app you won't have this issue, but Chromium is translated to many languages, some of which are very verbose in essence. After translation we don't know the widgets size; so in general we delegate the view's height to the framework, and seldom do we set fixed height. https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... File demos/src/main/AndroidManifest.xml (right): https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... demos/src/main/AndroidManifest.xml:87: android:name=".Camera" s/Camera/CameraActivity. Always name objects according what they actually are. https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... File demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java (right): https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:14: package org.chromium.customtabsdemos; Add one line before #14. https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:60: private ArrayList<String> mDescriptions; Instead of holding three lists, make a static private internal class (say ActionButtonParams?) and store a list of this class. This way you don't need to call mDescriptions.size() to get the number of buttons, which is very confusing. https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:85: mDescriptions = (ArrayList<String>) getIntent() #85 - #90 are not formatted correctly. Java line wrap is 100. Same for #94. https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:96: Remove phantom empty lines. https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:100: // Select UI setting previously set by the user. Move the two comments to the javadoc of selectUISetting(). https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:108: case R.id.save_setting: Let's not mingle the onclick in dialog with onclick in this activity. IIRC in XML you can specify any method to android:onClick. I would create a new public method, say onClickDialog(View v), and route all dialog buttons clicking to that new function. https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:113: showActionBarItemDialogue(); In our codebase we never use the word "dialogue" (try grepping it in chromium); instead "dialog" is always preferred. https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:156: private void displayActionBarItems(int startIndex, int endIndex) { The two parameters are not necessary. You can get the ints in this method and you don't need to pass them around. https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:198: Remove phantom empty lines. https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:206: private void showActionBarItemDialogue() { Dialog. https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:222: return new String[] {"Send", "Select Contact", "Add Event", "Camera"}; 1. This method does not return "Intents"; it returns Strings instead. 2. You should make the String[] static final at the top of the file so that the memory is not allocated every time you call it.. https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:232: setResult(0, intent); You need to finish so that the parent activity can be shown. https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... File demos/src/main/java/org/chromium/customtabsdemos/CustomUIActivity.java (right): https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/CustomUIActivity.java:129: intent.putExtra(AdvancedUISettingActivity.KEY_PACKAGES_LIST, allSupportingPackageNames); you don't need to pass the list to the advanced activity. This can be acquired from a static method. https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... File demos/src/main/res/layout/dialogue_add_actionbar_item.xml (right): https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... demos/src/main/res/layout/dialogue_add_actionbar_item.xml:7: android:paddingRight="@dimen/activity_horizontal_margin" Two issues: 1. Use padding instead of four paddings 2. Margins are not paddings. Here I wouldn't bother give it a dimens name. You can directly say 5dp. https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... File demos/src/main/res/layout/row.xml (right): https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... demos/src/main/res/layout/row.xml:1: <?xml version="1.0" encoding="utf-8"?> Let's keep the demo simple and only concentrate on things that must be done to launch a CCT. I would remove this row and use simple_spinner_dropdown_item. :) https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... File demos/src/main/res/values/dimens.xml (right): https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... demos/src/main/res/values/dimens.xml:21: <dimen name="button_width">300sp</dimen> 1. It's in general a very bad idea to restrict buttons size. Users might use a different language, or they set different font size, or they are using a different system font, all of which will break the UI. 2. SP is only intended for font size. For layout, use dp. http://sebastien-gabriel.com/designers-guide-to-dpi/ https://chromiumcodereview.appspot.com/1603383003/diff/140001/shared/src/main... File shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java (right): https://chromiumcodereview.appspot.com/1603383003/diff/140001/shared/src/main... shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java:147: public static String[] getColors() { This class is copied to many 3p apps, and thus it should be very generic and only about custom tabs. Let's not put UI related code in this file. Move this function to Advanced UI activity. Same for #181. https://chromiumcodereview.appspot.com/1603383003/diff/140001/shared/src/main... shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java:189: public static String chromeStable() { Only make APIs if they are used immediately by our code, or they will be stripped away by proguard anyway. http://developer.android.com/tools/help/proguard.html
https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/Android... File demos/src/main/AndroidManifest.xml (right): https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/Android... demos/src/main/AndroidManifest.xml:87: android:name=".Camera" On 2016/02/10 02:36:06, Ian Wen wrote: > s/Camera/CameraActivity. Always name objects according what they actually are. I noticed that the previous activity, for example has name as ".CustomUIActivity", so that's why I called this Camera as ".Camera". Do you mean I should change it to ".CameraActivity"? https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/java/or... File demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java (right): https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:14: package org.chromium.customtabsdemos; On 2016/02/10 02:36:06, Ian Wen wrote: > Add one line before #14. Done. https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:60: private ArrayList<String> mDescriptions; On 2016/02/10 02:36:06, Ian Wen wrote: > Instead of holding three lists, make a static private internal class (say > ActionButtonParams?) and store a list of this class. This way you don't need to > call mDescriptions.size() to get the number of buttons, which is very confusing. Done. https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:85: mDescriptions = (ArrayList<String>) getIntent() On 2016/02/10 02:36:06, Ian Wen wrote: > #85 - #90 are not formatted correctly. Java line wrap is 100. Same for #94. Done. https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:96: On 2016/02/10 02:36:06, Ian Wen wrote: > Remove phantom empty lines. Done. https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:100: // Select UI setting previously set by the user. On 2016/02/10 02:36:06, Ian Wen wrote: > Move the two comments to the javadoc of selectUISetting(). Done. https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:108: case R.id.save_setting: On 2016/02/10 02:36:06, Ian Wen wrote: > Let's not mingle the onclick in dialog with onclick in this activity. > > IIRC in XML you can specify any method to android:onClick. I would create a new > public method, say onClickDialog(View v), and route all dialog buttons clicking > to that new function. Done. https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:113: showActionBarItemDialogue(); On 2016/02/10 02:36:06, Ian Wen wrote: > In our codebase we never use the word "dialogue" (try grepping it in chromium); > instead "dialog" is always preferred. Done. https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:156: private void displayActionBarItems(int startIndex, int endIndex) { On 2016/02/10 02:36:06, Ian Wen wrote: > The two parameters are not necessary. You can get the ints in this method and > you don't need to pass them around. Done. https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:198: On 2016/02/10 02:36:06, Ian Wen wrote: > Remove phantom empty lines. Done. https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:206: private void showActionBarItemDialogue() { On 2016/02/10 02:36:06, Ian Wen wrote: > Dialog. Done. https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:222: return new String[] {"Send", "Select Contact", "Add Event", "Camera"}; On 2016/02/10 02:36:06, Ian Wen wrote: > 1. This method does not return "Intents"; it returns Strings instead. > 2. You should make the String[] static final at the top of the file so that the > memory is not allocated every time you call it.. Done. https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:232: setResult(0, intent); On 2016/02/10 02:36:06, Ian Wen wrote: > You need to finish so that the parent activity can be shown. Done. https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/java/or... File demos/src/main/java/org/chromium/customtabsdemos/CustomUIActivity.java (right): https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/CustomUIActivity.java:129: intent.putExtra(AdvancedUISettingActivity.KEY_PACKAGES_LIST, allSupportingPackageNames); On 2016/02/10 02:36:06, Ian Wen wrote: > you don't need to pass the list to the advanced activity. This can be acquired > from a static method. Done. https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/res/lay... File demos/src/main/res/layout/dialogue_add_actionbar_item.xml (right): https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/res/lay... demos/src/main/res/layout/dialogue_add_actionbar_item.xml:7: android:paddingRight="@dimen/activity_horizontal_margin" On 2016/02/10 02:36:06, Ian Wen wrote: > Two issues: > 1. Use padding instead of four paddings > 2. Margins are not paddings. Here I wouldn't bother give it a dimens name. You > can directly say 5dp. Done. https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/res/lay... File demos/src/main/res/layout/row.xml (right): https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/res/lay... demos/src/main/res/layout/row.xml:1: <?xml version="1.0" encoding="utf-8"?> On 2016/02/10 02:36:06, Ian Wen wrote: > Let's keep the demo simple and only concentrate on things that must be done to > launch a CCT. I would remove this row and use simple_spinner_dropdown_item. :) Done. https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/res/val... File demos/src/main/res/values/dimens.xml (right): https://codereview.chromium.org/1603383003/diff/140001/demos/src/main/res/val... demos/src/main/res/values/dimens.xml:21: <dimen name="button_width">300sp</dimen> On 2016/02/10 02:36:06, Ian Wen wrote: > 1. It's in general a very bad idea to restrict buttons size. Users might use a > different language, or they set different font size, or they are using a > different system font, all of which will break the UI. > 2. SP is only intended for font size. For layout, use dp. > http://sebastien-gabriel.com/designers-guide-to-dpi/ I have set the dimension to dp. As with the width of button, I am wondering say if user is using a different language by changing string.xml, can the app use another version of dimens.xml too? Probably that is too much trouble. But what is the convention way to do if I want the button to be the same width and height? https://codereview.chromium.org/1603383003/diff/140001/shared/src/main/java/o... File shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java (right): https://codereview.chromium.org/1603383003/diff/140001/shared/src/main/java/o... shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java:147: public static String[] getColors() { On 2016/02/10 02:36:06, Ian Wen wrote: > This class is copied to many 3p apps, and thus it should be very generic and > only about custom tabs. Let's not put UI related code in this file. Move this > function to Advanced UI activity. Same for #181. Done. https://codereview.chromium.org/1603383003/diff/140001/shared/src/main/java/o... shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java:189: public static String chromeStable() { On 2016/02/10 02:36:06, Ian Wen wrote: > Only make APIs if they are used immediately by our code, or they will be > stripped away by proguard anyway. > > http://developer.android.com/tools/help/proguard.html Done.
Simpler than previous patch, which is good. https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... File demos/src/main/AndroidManifest.xml (right): https://chromiumcodereview.appspot.com/1603383003/diff/140001/demos/src/main/... demos/src/main/AndroidManifest.xml:87: android:name=".Camera" On 2016/02/17 05:18:09, BigBossZhiling wrote: > On 2016/02/10 02:36:06, Ian Wen wrote: > > s/Camera/CameraActivity. Always name objects according what they actually are. > > I noticed that the previous activity, for example has name as > ".CustomUIActivity", so that's why I called this Camera as ".Camera". Do you > mean I should change it to ".CameraActivity"? Yes. In code review, s/A/B means replace A with B (it's VIM's grammar). https://chromiumcodereview.appspot.com/1603383003/diff/160001/demos/src/main/... File demos/src/main/AndroidManifest.xml (right): https://chromiumcodereview.appspot.com/1603383003/diff/160001/demos/src/main/... demos/src/main/AndroidManifest.xml:65: android:launchMode="singleTop"> singleTop is not necessary to bring back the previous activity. Why do we need this? If you are in a child activity you can simply call finish() to go to the previous activity. https://chromiumcodereview.appspot.com/1603383003/diff/160001/demos/src/main/... File demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java (right): https://chromiumcodereview.appspot.com/1603383003/diff/160001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:274: class ActionButtonParams implements Serializable{ 1. indentation is wrong. 2. This class has to be a private static class. Check http://stackoverflow.com/questions/3106912/why-does-android-prefer-static-cla... for more info https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... File demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java (right): https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:68: static String[] getColors() { These methods should be private https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:80: * Convert color in string to int. s/Convert/Converts. Same for all the javadocs you write. https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:84: static int stringToColor(String color) { Remove this method and use Color.parse() instead. https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:96: static Bitmap uriToBitMap(Context context, Uri uri) { s/BitMap/Bitmap, make it private https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:303: class ActionButtonParams implements Serializable{ 1. Why is it package protected, not private? 2. The indent is incorrect. https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... File demos/src/main/java/org/chromium/customtabsdemos/Camera.java (right): https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/Camera.java:7: public class Camera extends AppCompatActivity { Rename it to CameraActivity. https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... File demos/src/main/java/org/chromium/customtabsdemos/CustomTabActivityHelper.java (right): https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/CustomTabActivityHelper.java:59: CustomTabsIntent customTabsIntent, Public methods should have javadocs https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... File demos/src/main/java/org/chromium/customtabsdemos/CustomUIActivity.java (right): https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/CustomUIActivity.java:49: private int advancedUISettintRequestCode; Member variables should start with an "m". Also this variable looks like a constant instead of a variable. https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... demos/src/main/java/org/chromium/customtabsdemos/CustomUIActivity.java:86: mToolbarColor = (String) data.getSerializableExtra Why is it SerializableExtra? You should use getStringExtra() and putExtra(). https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... File demos/src/main/res/layout/activity_custom_ui.xml (right): https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... demos/src/main/res/layout/activity_custom_ui.xml:45: android:layout_height="@dimen/view_height"> Setting height on textview is potentially dangerous, because the system font size is beyond our control. I recently just fixed a bug about this: https://chromiumcodereview.appspot.com/1757583002/ I would keep wrap_content. https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... File demos/src/main/res/layout/advanced_ui_setting.xml (right): https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... demos/src/main/res/layout/advanced_ui_setting.xml:19: <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" This file is not properly formatted. https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... File demos/src/main/res/layout/display_action_bar_item_setting.xml (right): https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... demos/src/main/res/layout/display_action_bar_item_setting.xml:2: xmlns:app="http://schemas.android.com/apk/res-auto" You miss copyright info in this file. https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... File demos/src/main/res/values/strings.xml (right): https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... demos/src/main/res/values/strings.xml:29: <string name="label_advanced_ui_setting">Advanced UI Setting</string> Advanced UI settings. Android only capitalize the first letter. Also it should be settings instead of setting. https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... demos/src/main/res/values/strings.xml:31: <string name="label_select_image">Select Action Image</string> Same here. https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... demos/src/main/res/values/strings.xml:33: <string name="label_add_action_bar_item">Add Action Bar Item</string> Same here. https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... demos/src/main/res/values/strings.xml:35: <string name="label_toolbar_color">Toolbar color </string> Remove space after color https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... File demos/src/main/res/values/styles.xml (right): https://chromiumcodereview.appspot.com/1603383003/diff/180001/demos/src/main/... demos/src/main/res/values/styles.xml:24: <item name="android:layout_width">fill_parent</item> Do you still need this style? https://chromiumcodereview.appspot.com/1603383003/diff/180001/shared/src/main... File shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java (right): https://chromiumcodereview.appspot.com/1603383003/diff/180001/shared/src/main... shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java:61: * @param context {@link Context} to use for accessing {@link PackageManager}. Where is the javadoc for the second parameter? https://chromiumcodereview.appspot.com/1603383003/diff/180001/shared/src/main... shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java:104: * See getPackageNameToUse(Context, List<String). use @see, which is standard way in javadoc. https://chromiumcodereview.appspot.com/1603383003/diff/180001/shared/src/main... shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java:107: return getPackageNameToUse(context, null); Why can't you directly call getAllPackages......() here? this way you don't need to do a null check in #68. https://chromiumcodereview.appspot.com/1603383003/diff/180001/shared/src/main... shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java:145: * Get all apps that support custom tabs. If it is a public method, javadocs should be written with care. If it is a private method, you can choose to ignore the documentation. https://chromiumcodereview.appspot.com/1603383003/diff/180001/shared/src/main... shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java:149: public static ArrayList<String> getAllPackagesSupportingCustomTabs(Context context) { 1. I would make it a private method. 2. I would rename it to getAppsSupportingCustomTabs.
https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/java/or... File demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java (right): https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:68: static String[] getColors() { On 2016/03/16 01:35:30, Ian Wen wrote: > These methods should be private Done. https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:80: * Convert color in string to int. On 2016/03/16 01:35:30, Ian Wen wrote: > s/Convert/Converts. Same for all the javadocs you write. Done. https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:84: static int stringToColor(String color) { On 2016/03/16 01:35:30, Ian Wen wrote: > Remove this method and use Color.parse() instead. Done. https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:96: static Bitmap uriToBitMap(Context context, Uri uri) { On 2016/03/16 01:35:30, Ian Wen wrote: > s/BitMap/Bitmap, make it private Done. https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:303: class ActionButtonParams implements Serializable{ On 2016/03/16 01:35:30, Ian Wen wrote: > 1. Why is it package protected, not private? > 2. The indent is incorrect. I think the reason why it is not private is that another activity, CustomUIActivity is also using this actionButtonParams. Why is the indent not correct? https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/java/or... File demos/src/main/java/org/chromium/customtabsdemos/Camera.java (right): https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/Camera.java:7: public class Camera extends AppCompatActivity { On 2016/03/16 01:35:30, Ian Wen wrote: > Rename it to CameraActivity. Done. https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/java/or... File demos/src/main/java/org/chromium/customtabsdemos/CustomTabActivityHelper.java (right): https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/CustomTabActivityHelper.java:59: CustomTabsIntent customTabsIntent, On 2016/03/16 01:35:30, Ian Wen wrote: > Public methods should have javadocs Done. https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/java/or... File demos/src/main/java/org/chromium/customtabsdemos/CustomUIActivity.java (right): https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/CustomUIActivity.java:49: private int advancedUISettintRequestCode; On 2016/03/16 01:35:30, Ian Wen wrote: > Member variables should start with an "m". Also this variable looks like a > constant instead of a variable. Done. https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/CustomUIActivity.java:86: mToolbarColor = (String) data.getSerializableExtra On 2016/03/16 01:35:30, Ian Wen wrote: > Why is it SerializableExtra? > > You should use getStringExtra() and putExtra(). Done. https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/res/lay... File demos/src/main/res/layout/activity_custom_ui.xml (right): https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/res/lay... demos/src/main/res/layout/activity_custom_ui.xml:45: android:layout_height="@dimen/view_height"> On 2016/03/16 01:35:30, Ian Wen wrote: > Setting height on textview is potentially dangerous, because the system font > size is beyond our control. > > I recently just fixed a bug about this: > https://chromiumcodereview.appspot.com/1757583002/ > > I would keep wrap_content. Done. https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/res/lay... File demos/src/main/res/layout/advanced_ui_setting.xml (right): https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/res/lay... demos/src/main/res/layout/advanced_ui_setting.xml:19: <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" On 2016/03/16 01:35:30, Ian Wen wrote: > This file is not properly formatted. Done. https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/res/lay... File demos/src/main/res/layout/display_action_bar_item_setting.xml (right): https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/res/lay... demos/src/main/res/layout/display_action_bar_item_setting.xml:2: xmlns:app="http://schemas.android.com/apk/res-auto" On 2016/03/16 01:35:30, Ian Wen wrote: > You miss copyright info in this file. Done. https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/res/val... File demos/src/main/res/values/strings.xml (right): https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/res/val... demos/src/main/res/values/strings.xml:29: <string name="label_advanced_ui_setting">Advanced UI Setting</string> On 2016/03/16 01:35:30, Ian Wen wrote: > Advanced UI settings. > > Android only capitalize the first letter. Also it should be settings instead of > setting. Done. https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/res/val... demos/src/main/res/values/strings.xml:31: <string name="label_select_image">Select Action Image</string> On 2016/03/16 01:35:31, Ian Wen wrote: > Same here. Done. https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/res/val... demos/src/main/res/values/strings.xml:33: <string name="label_add_action_bar_item">Add Action Bar Item</string> On 2016/03/16 01:35:31, Ian Wen wrote: > Same here. Done. https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/res/val... demos/src/main/res/values/strings.xml:35: <string name="label_toolbar_color">Toolbar color </string> On 2016/03/16 01:35:31, Ian Wen wrote: > Remove space after color Done. https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/res/val... File demos/src/main/res/values/styles.xml (right): https://codereview.chromium.org/1603383003/diff/180001/demos/src/main/res/val... demos/src/main/res/values/styles.xml:24: <item name="android:layout_width">fill_parent</item> On 2016/03/16 01:35:31, Ian Wen wrote: > Do you still need this style? Done. https://codereview.chromium.org/1603383003/diff/180001/shared/src/main/java/o... File shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java (right): https://codereview.chromium.org/1603383003/diff/180001/shared/src/main/java/o... shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java:61: * @param context {@link Context} to use for accessing {@link PackageManager}. On 2016/03/16 01:35:31, Ian Wen wrote: > Where is the javadoc for the second parameter? Done. https://codereview.chromium.org/1603383003/diff/180001/shared/src/main/java/o... shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java:104: * See getPackageNameToUse(Context, List<String). On 2016/03/16 01:35:31, Ian Wen wrote: > use @see, which is standard way in javadoc. Done. https://codereview.chromium.org/1603383003/diff/180001/shared/src/main/java/o... shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java:107: return getPackageNameToUse(context, null); On 2016/03/16 01:35:31, Ian Wen wrote: > Why can't you directly call getAllPackages......() here? this way you don't need > to do a null check in #68. Done. https://codereview.chromium.org/1603383003/diff/180001/shared/src/main/java/o... shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java:145: * Get all apps that support custom tabs. On 2016/03/16 01:35:31, Ian Wen wrote: > If it is a public method, javadocs should be written with care. If it is a > private method, you can choose to ignore the documentation. Done. https://codereview.chromium.org/1603383003/diff/180001/shared/src/main/java/o... shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java:149: public static ArrayList<String> getAllPackagesSupportingCustomTabs(Context context) { On 2016/03/16 01:35:31, Ian Wen wrote: > 1. I would make it a private method. > 2. I would rename it to getAppsSupportingCustomTabs. I think probably it can't be a private method because in the package spinner of the setting page, we need to getAllPackagesSupportingCustomTabs and show it to the user and let the user pick the one he wants.
I see some improvements. 1. Please update your CL description. 2. Please rebase. 3. Please be sure to read through our java code style guideline carefully. 4. Please apply my comments to all pieces of code so that we don't repeat ourselves. https://codereview.chromium.org/1603383003/diff/180001/shared/src/main/java/o... File shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java (right): https://codereview.chromium.org/1603383003/diff/180001/shared/src/main/java/o... shared/src/main/java/org/chromium/customtabsclient/shared/CustomTabsHelper.java:149: public static ArrayList<String> getAllPackagesSupportingCustomTabs(Context context) { On 2016/03/18 00:59:08, BigBossZhiling wrote: > On 2016/03/16 01:35:31, Ian Wen wrote: > > 1. I would make it a private method. > > 2. I would rename it to getAppsSupportingCustomTabs. > > I think probably it can't be a private method because in the package spinner of > the setting page, we need to getAllPackagesSupportingCustomTabs and show it to > the user and let the user pick the one he wants. Why not use the function in #140? I would put the code in this function back to #80, and let AdvancedUIActivity use getPackages(). https://codereview.chromium.org/1603383003/diff/200001/demos/build.gradle File demos/build.gradle (right): https://codereview.chromium.org/1603383003/diff/200001/demos/build.gradle#new... demos/build.gradle:17: proguardFiles getDefaultProguardFile('proguard-android.txt'), 'proguard-rules.pro' Also please rebase your patch. There were several conflicts when I tried to apply. https://codereview.chromium.org/1603383003/diff/200001/demos/build.gradle#new... demos/build.gradle:26: compile project(':customtabs') I think this change is no longer needed. 23.2.1 is already released. What we should do is to change #24 to 23.2.1, and reference customtabs:23.2.1 in #26. https://codereview.chromium.org/1603383003/diff/200001/demos/src/main/Android... File demos/src/main/AndroidManifest.xml (right): https://codereview.chromium.org/1603383003/diff/200001/demos/src/main/Android... demos/src/main/AndroidManifest.xml:88: <meta-data Q: what does meta-data do? https://codereview.chromium.org/1603383003/diff/200001/demos/src/main/java/or... File demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java (right): https://codereview.chromium.org/1603383003/diff/200001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:66: static Bitmap uriToBitmap(Context context, Uri uri) { 1. Why is it a package method? If this method is only used in another activity, put it there instead. 2. Rename to getBitmapFromUri() https://codereview.chromium.org/1603383003/diff/200001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:83: mPackageName = (String) getIntent().getSerializableExtra(KEY_PACKAGE); Again, like what I said in the previous patch, use Intent#getStringExtra() instead. https://codereview.chromium.org/1603383003/diff/200001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:152: fieldList.add(i, colorName.charAt(0) + colorName.substring(1).toLowerCase()); I would remove #151 and #152. Let's focus on more practical issues... https://codereview.chromium.org/1603383003/diff/200001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:178: { Code format incorrect! :( Please push #178 to #177. Again, please read through the guideline carefully https://source.android.com/source/code-style.html https://codereview.chromium.org/1603383003/diff/200001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:185: Uri.parse(image)); Push #185 to #184 https://codereview.chromium.org/1603383003/diff/200001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:265: * Select UI setting previously set by the user. s/Select/Selects Same for all javadocs you write. Javadoc for a function is used to "describe" the behavior of the function, not to "command" the function to do something. https://codereview.chromium.org/1603383003/diff/200001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/AdvancedUISettingActivity.java:294: this.image = image; Remove "this." Same for all other places. https://codereview.chromium.org/1603383003/diff/200001/demos/src/main/java/or... File demos/src/main/java/org/chromium/customtabsdemos/CustomUIActivity.java (right): https://codereview.chromium.org/1603383003/diff/200001/demos/src/main/java/or... demos/src/main/java/org/chromium/customtabsdemos/CustomUIActivity.java:50: private int ADVANCED_SETTING_REQUEST_CODE; Constants should be static final, and should be put on the very top of the file. https://codereview.chromium.org/1603383003/diff/200001/demos/src/main/res/lay... File demos/src/main/res/layout/display_action_bar_item_setting.xml (right): https://codereview.chromium.org/1603383003/diff/200001/demos/src/main/res/lay... demos/src/main/res/layout/display_action_bar_item_setting.xml:18: android:layout_width="match_parent" A general guideline for all of the xml layout files: 1. android:id should be the first element. 2. android:layout_* should be the second element and they should group together. 3. tools:* and other namespaces should be placed below android namespace. https://codereview.chromium.org/1603383003/diff/200001/demos/src/main/res/val... File demos/src/main/res/values/dimens.xml (right): https://codereview.chromium.org/1603383003/diff/200001/demos/src/main/res/val... demos/src/main/res/values/dimens.xml:23: <dimen name="button_padding_top">10dp</dimen> Please make sure all of the listed dimensions are used at lease once. https://codereview.chromium.org/1603383003/diff/200001/demos/src/main/res/val... File demos/src/main/res/values/styles.xml (left): https://codereview.chromium.org/1603383003/diff/200001/demos/src/main/res/val... demos/src/main/res/values/styles.xml:17: Revert the changes to this file? |
