Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2902503002:
[Payment] should show all accepted card icon in payment editor. (Closed)
|
Created:
3 years, 7 months ago by Hwanseung Lee Modified:
3 years, 6 months ago CC:
chromium-reviews, mahmadi+paymentswatch_chromium.org, rouslan+payments_chromium.org, sebsg+paymentswatch_chromium.org, gogerald+paymentswatch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Payment] should show all accepted card icon in payment editor.
accepted card icons are missed in portrait mode according
devices resolution. so we should replace LinearLayout
with GridView to show all accepted card icons.
if can't show all card icons in one line, GridView will
make more then two lines.
BUG=725296
Review-Url: https://codereview.chromium.org/2902503002
Cr-Commit-Position: refs/heads/master@{#477268}
Committed: https://chromium.googlesource.com/chromium/src/+/117a4a82c42bcc441a7d5f21ab76b3e37fcbe6ef
Patch Set 1 #Patch Set 2 : fix a build error. #Patch Set 3 : adjust icon size #
Total comments: 34
Patch Set 4 : fix up #
Total comments: 8
Patch Set 5 : fix a class name. #
Total comments: 4
Patch Set 6 : fix the comment #
Total comments: 7
Patch Set 7 : move #
Total comments: 2
Patch Set 8 : add comment #
Total comments: 2
Patch Set 9 : q #
Messages
Total messages: 71 (51 generated)
Description was changed from ========== [wip] icon gridview BUG= ========== to ========== should show all accepted card icon in payment editor. accepted card icons are missed in portrait mode according devices resolution. so we should replace LinearLayout with GridView to show all accepted card icons. if can't show all card icons in one line, gridview will make more then one line. BUG=725296 ==========
Description was changed from ========== should show all accepted card icon in payment editor. accepted card icons are missed in portrait mode according devices resolution. so we should replace LinearLayout with GridView to show all accepted card icons. if can't show all card icons in one line, gridview will make more then one line. BUG=725296 ========== to ========== [Payment]should show all accepted card icon in payment editor. accepted card icons are missed in portrait mode according devices resolution. so we should replace LinearLayout with GridView to show all accepted card icons. if can't show all card icons in one line, gridview will make more then one line. BUG=725296 ==========
Description was changed from ========== [Payment]should show all accepted card icon in payment editor. accepted card icons are missed in portrait mode according devices resolution. so we should replace LinearLayout with GridView to show all accepted card icons. if can't show all card icons in one line, gridview will make more then one line. BUG=725296 ========== to ========== [Payment]should show all accepted card icon in payment editor. accepted card icons are missed in portrait mode according devices resolution. so we should replace LinearLayout with GridView to show all accepted card icons. if can't show all card icons in one line, gridview will make more then one line. BUG=725296 ==========
Description was changed from ========== [Payment]should show all accepted card icon in payment editor. accepted card icons are missed in portrait mode according devices resolution. so we should replace LinearLayout with GridView to show all accepted card icons. if can't show all card icons in one line, gridview will make more then one line. BUG=725296 ========== to ========== [Payment] should show all accepted card icon in payment editor. accepted card icons are missed in portrait mode according devices resolution. so we should replace LinearLayout with GridView to show all accepted card icons. if can't show all card icons in one line, gridview will make more then one line. BUG=725296 ==========
The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Payment] should show all accepted card icon in payment editor. accepted card icons are missed in portrait mode according devices resolution. so we should replace LinearLayout with GridView to show all accepted card icons. if can't show all card icons in one line, gridview will make more then one line. BUG=725296 ========== to ========== [Payment] should show all accepted card icon in payment editor. accepted card icons are missed in portrait mode according devices resolution. so we should replace LinearLayout with GridView to show all accepted card icons. if can't show all card icons in one line, gridview will make more then two lines. BUG=725296 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Payment] should show all accepted card icon in payment editor. accepted card icons are missed in portrait mode according devices resolution. so we should replace LinearLayout with GridView to show all accepted card icons. if can't show all card icons in one line, gridview will make more then two lines. BUG=725296 ========== to ========== [Payment] should show all accepted card icon in payment editor. accepted card icons are missed in portrait mode according devices resolution. so we should replace LinearLayout with GridView to show all accepted card icons. if can't show all card icons in one line, GridView will make more then two lines. BUG=725296 ==========
hs1217.lee@samsung.com changed reviewers: + gogerald@chromium.org, rouslan@chromium.org
@rouslan, gogerald PTAL, thank you. i attached image to compare easily with before and after. (https://bugs.chromium.org/p/chromium/issues/detail?id=725296&desc=2#c10) please refer to attached image.
Ganggui, you can review this as an owner.
https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java (right): https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:41: DynamicHeightGridView container = nit: iconsContainer https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:52: private static class IconAdapter extends BaseAdapter { gridview setAdapter accepts ListAdapter https://developer.android.com/reference/android/widget/GridView.html#setAdapt... https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:54: private List<Integer> mIconResources; nit: mIconResourceIds https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:55: private List<Integer> mIconDescription; nit: mIconDescriptionIds https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:58: public IconAdapter( IconListAdapter? https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:64: R.dimen.payments_section_logo_width); nit: assert mIconResourceIds.size() == mIconDescriptionIds.size(); https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:66: public int getCount() { one space line above and @Override https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:69: public Object getItem(int position) { ditto https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:72: public long getItemId(int position) { ditto https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:75: public View getView(int position, View convertView, ViewGroup parent) { ditto https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:76: ImageView imageView; you can simplify this code like below. ImageView iconView = covertView; if(iconView == null) iconView = new ImageView(mContext); https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:82: imageView.setImageResource(mIconResources.get(position)); setImageDrawable instead of setImageResource, https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:83: imageView.setBackgroundResource(R.drawable.payments_ui_logo_bg); no need setBackgroundResource anymore, https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/DynamicHeightGridView.java (right): https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DynamicHeightGridView.java:13: * This class is customized GridView. it may be clear and simpler to say "This class is a customized GridView which draws items in multiple lines automatically" https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DynamicHeightGridView.java:17: public DynamicHeightGridView(Context context, AttributeSet attrs) { looks only this constructor is needed, comment it like here https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DynamicHeightGridView.java:26: @Override space line above, https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DynamicHeightGridView.java:28: int expandSpec = do we really need this customization since we already set height to wrap_content?
@gogerald1 could you review again? https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java (right): https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:41: DynamicHeightGridView container = On 2017/05/24 14:25:23, gogerald1 wrote: > nit: iconsContainer Done. https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:52: private static class IconAdapter extends BaseAdapter { On 2017/05/24 14:25:23, gogerald1 wrote: > gridview setAdapter accepts ListAdapter > > https://developer.android.com/reference/android/widget/GridView.html#setAdapt... right, but BaseAdapter implement ListAdapter. https://developer.android.com/reference/android/widget/BaseAdapter.html https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:54: private List<Integer> mIconResources; On 2017/05/24 14:25:23, gogerald1 wrote: > nit: mIconResourceIds Done. https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:55: private List<Integer> mIconDescription; On 2017/05/24 14:25:23, gogerald1 wrote: > nit: mIconDescriptionIds Done. https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:58: public IconAdapter( On 2017/05/24 14:25:23, gogerald1 wrote: > IconListAdapter? Done. https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:64: R.dimen.payments_section_logo_width); On 2017/05/24 14:25:23, gogerald1 wrote: > nit: assert mIconResourceIds.size() == mIconDescriptionIds.size(); Done. https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:66: public int getCount() { On 2017/05/24 14:25:23, gogerald1 wrote: > one space line above and @Override Done. https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:69: public Object getItem(int position) { On 2017/05/24 14:25:23, gogerald1 wrote: > ditto Done. https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:72: public long getItemId(int position) { On 2017/05/24 14:25:23, gogerald1 wrote: > ditto Done. https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:75: public View getView(int position, View convertView, ViewGroup parent) { On 2017/05/24 14:25:23, gogerald1 wrote: > ditto Done. https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:76: ImageView imageView; On 2017/05/24 14:25:23, gogerald1 wrote: > you can simplify this code like below. > > ImageView iconView = covertView; > if(iconView == null) iconView = new ImageView(mContext); Done. https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:82: imageView.setImageResource(mIconResources.get(position)); On 2017/05/24 14:25:23, gogerald1 wrote: > setImageDrawable instead of setImageResource, > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... Done. https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:83: imageView.setBackgroundResource(R.drawable.payments_ui_logo_bg); On 2017/05/24 14:25:23, gogerald1 wrote: > no need setBackgroundResource anymore, Done. https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/DynamicHeightGridView.java (right): https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DynamicHeightGridView.java:13: * This class is customized GridView. On 2017/05/24 14:25:23, gogerald1 wrote: > it may be clear and simpler to say "This class is a customized GridView which > draws items in multiple lines automatically" Done. https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DynamicHeightGridView.java:17: public DynamicHeightGridView(Context context, AttributeSet attrs) { On 2017/05/24 14:25:23, gogerald1 wrote: > looks only this constructor is needed, comment it like here > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... Done. https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DynamicHeightGridView.java:26: @Override On 2017/05/24 14:25:23, gogerald1 wrote: > space line above, Done. https://codereview.chromium.org/2902503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DynamicHeightGridView.java:28: int expandSpec = On 2017/05/24 14:25:24, gogerald1 wrote: > do we really need this customization since we already set height to > wrap_content? it is not working in scrollview, so this class needs. refer to below link, there is detail information. https://stackoverflow.com/questions/4523609/grid-of-images-inside-scrollview/...
Thanks, few more comments https://codereview.chromium.org/2902503002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java (right): https://codereview.chromium.org/2902503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:54: * missing comments? https://codereview.chromium.org/2902503002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/DynamicHeightGridView.java (right): https://codereview.chromium.org/2902503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DynamicHeightGridView.java:14: public class DynamicHeightGridView extends GridView { name it ExpandableGridView looks make more sense, https://codereview.chromium.org/2902503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DynamicHeightGridView.java:21: public DynamicHeightGridView(Context context) { I guess you no need this and the below constructor for now, especially this constructor is usually used to construct in code without giving AttributeSet, https://codereview.chromium.org/2902503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DynamicHeightGridView.java:34: heightSpec = MeasureSpec.makeMeasureSpec(Integer.MAX_VALUE >> 2, MeasureSpec.AT_MOST); View.MEASURED_SIZE_MASK looks make more sense here, or 'Integer.MAX_VALUE & View.MEASURED_SIZE_MASK' for easy to read,
The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
@gogerald1 PTAL, i fixed the some lines. https://codereview.chromium.org/2902503002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java (right): https://codereview.chromium.org/2902503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:54: * On 2017/05/30 19:36:48, gogerald1 wrote: > missing comments? sorry i missed this line. https://codereview.chromium.org/2902503002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/DynamicHeightGridView.java (right): https://codereview.chromium.org/2902503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DynamicHeightGridView.java:14: public class DynamicHeightGridView extends GridView { On 2017/05/30 19:36:48, gogerald1 wrote: > name it ExpandableGridView looks make more sense, Done. https://codereview.chromium.org/2902503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DynamicHeightGridView.java:21: public DynamicHeightGridView(Context context) { On 2017/05/30 19:36:48, gogerald1 wrote: > I guess you no need this and the below constructor for now, especially this > constructor is usually used to construct in code without giving AttributeSet, Done. https://codereview.chromium.org/2902503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DynamicHeightGridView.java:34: heightSpec = MeasureSpec.makeMeasureSpec(Integer.MAX_VALUE >> 2, MeasureSpec.AT_MOST); On 2017/05/30 19:36:48, gogerald1 wrote: > View.MEASURED_SIZE_MASK looks make more sense here, or 'Integer.MAX_VALUE & > View.MEASURED_SIZE_MASK' for easy to read, Done.
lgtm % comments, you need another reviewer for resource changes, https://codereview.chromium.org/2902503002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java (right): https://codereview.chromium.org/2902503002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:54: * this Adapter provides access to the icon views. "An instance of a {@link BaseAdapter} that provides a list of card icon views."? https://codereview.chromium.org/2902503002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java (right): https://codereview.chromium.org/2902503002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java:22: public ExpandableGridView(Context context, AttributeSet attrs, int defStyle) { nit: I guess this constructor is not needed for now,
The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2902503002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java (right): https://codereview.chromium.org/2902503002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorIconsField.java:54: * this Adapter provides access to the icon views. On 2017/05/31 15:12:01, gogerald1 wrote: > "An instance of a {@link BaseAdapter} that provides a list of card icon views."? Done. https://codereview.chromium.org/2902503002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java (right): https://codereview.chromium.org/2902503002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java:22: public ExpandableGridView(Context context, AttributeSet attrs, int defStyle) { On 2017/05/31 15:12:01, gogerald1 wrote: > nit: I guess this constructor is not needed for now, Done.
hs1217.lee@samsung.com changed reviewers: + dfalcantara@chromium.org
@dfalcantara PTAL, thank you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2902503002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java (right): https://codereview.chromium.org/2902503002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java:25: heightSpec = MeasureSpec.makeMeasureSpec(View.MEASURED_SIZE_MASK, MeasureSpec.AT_MOST); This is a mask; you're supposed to use it to mask against the actual measurespec, not use it as an actual width for makeMeasureSpec. You're saying that the height of this thing should be at most 16777215 pixels high. Why do you think this is actually necessary?
On 2017/05/31 17:29:42, dfalcantara wrote: > https://codereview.chromium.org/2902503002/diff/100001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java > (right): > > https://codereview.chromium.org/2902503002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java:25: > heightSpec = MeasureSpec.makeMeasureSpec(View.MEASURED_SIZE_MASK, > MeasureSpec.AT_MOST); > This is a mask; you're supposed to use it to mask against the actual > measurespec, not use it as an actual width for makeMeasureSpec. You're saying > that the height of this thing should be at most 16777215 pixels high. > > Why do you think this is actually necessary? it is not working well without this code in scrollview. refer to below link, there is detail information. https://stackoverflow.com/questions/4523609/grid-of-images-inside-scrollview/...
On 2017/05/31 23:41:17, Hwanseung Lee wrote: > On 2017/05/31 17:29:42, dfalcantara wrote: > > > https://codereview.chromium.org/2902503002/diff/100001/chrome/android/java/sr... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java > > (right): > > > > > https://codereview.chromium.org/2902503002/diff/100001/chrome/android/java/sr... > > > chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java:25: > > heightSpec = MeasureSpec.makeMeasureSpec(View.MEASURED_SIZE_MASK, > > MeasureSpec.AT_MOST); > > This is a mask; you're supposed to use it to mask against the actual > > measurespec, not use it as an actual width for makeMeasureSpec. You're saying > > that the height of this thing should be at most 16777215 pixels high. > > > > Why do you think this is actually necessary? > > it is not working well without this code in scrollview. > refer to below link, there is detail information. > https://stackoverflow.com/questions/4523609/grid-of-images-inside-scrollview/... As your link mentions (which you really should have linked to), you're copying a hack that is passing in a value just happens to match some value that is the maximum possible value that a measure spec takes in. This would be less objectionable if you explained why you're actually doing it, but right now it just looks broken.
https://codereview.chromium.org/2902503002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java (right): https://codereview.chromium.org/2902503002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java:5: package org.chromium.chrome.browser.widget; Confine this to payments/ui. The stackoverflow post describes many ways this is suboptimal and I'd hate for it to be used in situations where larger images are used and memory is critical. https://codereview.chromium.org/2902503002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java:13: * This class is a customized GridView which draws items in multiple lines automatically End this comment with a period.
The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
@dfalcantara could you review again? thank you. https://codereview.chromium.org/2902503002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java (right): https://codereview.chromium.org/2902503002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java:5: package org.chromium.chrome.browser.widget; On 2017/06/01 00:08:28, dfalcantara wrote: > Confine this to payments/ui. The stackoverflow post describes many ways this is > suboptimal and I'd hate for it to be used in situations where larger images are > used and memory is critical. i moved it to payments/ui https://codereview.chromium.org/2902503002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java:13: * This class is a customized GridView which draws items in multiple lines automatically On 2017/06/01 00:08:27, dfalcantara wrote: > End this comment with a period. Done. https://codereview.chromium.org/2902503002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java:25: heightSpec = MeasureSpec.makeMeasureSpec(View.MEASURED_SIZE_MASK, MeasureSpec.AT_MOST); On 2017/05/31 17:29:42, dfalcantara wrote: > This is a mask; you're supposed to use it to mask against the actual > measurespec, not use it as an actual width for makeMeasureSpec. You're saying > that the height of this thing should be at most 16777215 pixels high. > > Why do you think this is actually necessary? i think it is not necessary. i checked how to makeMeasureSpec work. (https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/sources/an...) actually MeasureSpec.AT_MOST is enough big number. so size(first parameter of makeMeasureSpec) is not important when use MeasureSpec.AT_MOST as mode. but i think it is symbolical mean which set big number to use View.MEASURED_SIZE_MASK. i changed it to parent's height. is it fine? or to use 'Integer.MAX_VALUE & View.MEASURED_SIZE_MASK' is more better?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2902503002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java (right): https://codereview.chromium.org/2902503002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ExpandableGridView.java:25: heightSpec = MeasureSpec.makeMeasureSpec(View.MEASURED_SIZE_MASK, MeasureSpec.AT_MOST); On 2017/06/01 14:36:38, Hwanseung Lee wrote: > On 2017/05/31 17:29:42, dfalcantara wrote: > > This is a mask; you're supposed to use it to mask against the actual > > measurespec, not use it as an actual width for makeMeasureSpec. You're saying > > that the height of this thing should be at most 16777215 pixels high. > > > > Why do you think this is actually necessary? > > i think it is not necessary. > i checked how to makeMeasureSpec work. > (https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/sources/an...) > > actually MeasureSpec.AT_MOST is enough big number. > so size(first parameter of makeMeasureSpec) is not important when use > MeasureSpec.AT_MOST as mode. > but i think it is symbolical mean which set big number to use > View.MEASURED_SIZE_MASK. > > i changed it to parent's height. is it fine? > or to use 'Integer.MAX_VALUE & View.MEASURED_SIZE_MASK' is more better? Parent's height makes more sense, but I don't understand why AT_MOST is a "big enough number". AT_MOST is a flag that indicates that the height of this View should only be AT MOST the first number you pass in. https://codereview.chromium.org/2902503002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ExpandableGridView.java (right): https://codereview.chromium.org/2902503002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ExpandableGridView.java:21: public void onMeasure(int widthMeasureSpec, int heightMeasureSpec) { Explain why you're actually doing this in a comment.
The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
@dfalcantara never mind about "enough big number". second parameter is mode type, mode type use front 2 bit of integer. so size(first parameter) have not to use front 2 bit. https://codereview.chromium.org/2902503002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ExpandableGridView.java (right): https://codereview.chromium.org/2902503002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ExpandableGridView.java:21: public void onMeasure(int widthMeasureSpec, int heightMeasureSpec) { On 2017/06/01 17:32:19, dfalcantara wrote: > Explain why you're actually doing this in a comment. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % using my comment https://codereview.chromium.org/2902503002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ExpandableGridView.java (right): https://codereview.chromium.org/2902503002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ExpandableGridView.java:23: // attributes. by setting MeasureSpec.AT_MOST, GridView can work well forcely. // GridView does not work well in a ScrollView when it uses WRAP_CONTENT. Instead, force it to use // AT_MOST. https://stackoverflow.com/questions/4523609/grid-of-images-inside-scrollview
The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #9 (id:160001) has been deleted
The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thank you. https://codereview.chromium.org/2902503002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ExpandableGridView.java (right): https://codereview.chromium.org/2902503002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ExpandableGridView.java:23: // attributes. by setting MeasureSpec.AT_MOST, GridView can work well forcely. On 2017/06/05 17:21:01, dfalcantara wrote: > // GridView does not work well in a ScrollView when it uses WRAP_CONTENT. > Instead, force it to use > // AT_MOST. > https://stackoverflow.com/questions/4523609/grid-of-images-inside-scrollview Done.
The CQ bit was checked by hs1217.lee@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from gogerald@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2902503002/#ps180001 (title: "q")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1496753321176980,
"parent_rev": "ee6a8849e41e7e43563e32b24f5d949c8c01f015", "commit_rev":
"117a4a82c42bcc441a7d5f21ab76b3e37fcbe6ef"}
Message was sent while issue was closed.
Description was changed from ========== [Payment] should show all accepted card icon in payment editor. accepted card icons are missed in portrait mode according devices resolution. so we should replace LinearLayout with GridView to show all accepted card icons. if can't show all card icons in one line, GridView will make more then two lines. BUG=725296 ========== to ========== [Payment] should show all accepted card icon in payment editor. accepted card icons are missed in portrait mode according devices resolution. so we should replace LinearLayout with GridView to show all accepted card icons. if can't show all card icons in one line, GridView will make more then two lines. BUG=725296 Review-Url: https://codereview.chromium.org/2902503002 Cr-Commit-Position: refs/heads/master@{#477268} Committed: https://chromium.googlesource.com/chromium/src/+/117a4a82c42bcc441a7d5f21ab76... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/117a4a82c42bcc441a7d5f21ab76... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
