Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2758313002:
Implement the new Photo picker, part two. (Closed)
DescriptionImplement the new Photo picker, part two.
This replaces the "Not implemented" Toast with a fully working dialog, except
the images used are all placeholder gray tiles (instead of actual decoded
images).
BUG=656015
Review-Url: https://codereview.chromium.org/2758313002
Cr-Commit-Position: refs/heads/master@{#463133}
Committed: https://chromium.googlesource.com/chromium/src/+/67f22396e43ea69024f3f158bfcd5ec411042dee
Patch Set 1 #Patch Set 2 : RemoveDecoder #Patch Set 3 : Resolve findbugs #Patch Set 4 : Polish #
Total comments: 34
Patch Set 5 : Address feedback #Patch Set 6 : Fix scrim color #
Total comments: 89
Patch Set 7 : Address feedback #Patch Set 8 : Trim changelist #
Total comments: 48
Patch Set 9 : Address feedback #
Total comments: 43
Patch Set 10 : Address remaining feedback #Patch Set 11 : Address Theresa's feedback #
Total comments: 8
Patch Set 12 : Address Theresa's feedback #
Total comments: 10
Patch Set 13 : Address Theresa's feedback #
Total comments: 8
Patch Set 14 : Last bit of comments #Patch Set 15 : Fix build #
Total comments: 22
Patch Set 16 : Address Ted's comments (and sync to latest) #Messages
Total messages: 92 (55 generated)
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
finnur@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
Looks a bit daunting, but about two-thirds of the files are png, xml and string changes. :)
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 finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
finnur@chromium.org changed reviewers: + twellington@chromium.org - mvanouwerkerk@chromium.org
Looks like Michael is out today; was going to have him do an initial pass at it. Do you mind reviewing? And, like I said to Michael: Looks a bit daunting, but about two-thirds of the files are png, xml and string changes. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
From a very high-level, what's the reason(s) that the photo picker isn't using the SelectableListLayout class? https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... File chrome/android/java/res/drawable/file_picker_scrim.xml (right): https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/drawable/file_picker_scrim.xml:1: <?xml version="1.0" encoding="utf-8"?> This needs a copyright comment. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/drawable/file_picker_scrim.xml:4: android:startColor="#42000000" Please define this in colors.xml. We have a TON of colors already, so ideally, if this is close enough to a color that's already used somewhere in Chrome we should use that. If it's not close enough to an existing color, the alpha % should be a value that's likely to be reused (e.g. 40000000 would be 25% or 4D000000 would be 30%). https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/drawable/file_picker_scrim.xml:5: android:endColor="#00000000" nit: @android:color/black https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... File chrome/android/java/res/drawable/file_picker_selected_background.xml (right): https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/drawable/file_picker_selected_background.xml:1: <?xml version="1.0" encoding="utf-8"?> This needs a copyright comment too. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/drawable/file_picker_selected_background.xml:11: <solid android:color="#ffffff" /> @android:color/white https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/bitmap_list_row.xml (right): https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/bitmap_list_row.xml:3: Copyright 2016 The Chromium Authors. All rights reserved. s/2016/2017 https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/bitmap_list_row.xml:21: android:contentDescription="@string/clear_cookies_and_site_data_title" This string is just a placeholder, right? Will you please add a TODO to update these strings or remove this file (whatever the next step is). https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/bitmap_list_row.xml:25: android:id="@+id/selected" Is this for the selected background? If so, selected_bg or selected_background would be a good id, and the content description can be @null. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/bitmap_list_row.xml:32: <ImageView nit: blank line before this ImageView https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/photo_picker_dialog_content.xml (right): https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/photo_picker_dialog_content.xml:3: Copyright 2016 The Chromium Authors. All rights reserved. s/2016/2017 https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/photo_picker_dialog_content.xml:12: android:id="@+id/layout" nit: replace "layout" with a more meaningful id. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/photo_picker_dialog_title.xml (right): https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/photo_picker_dialog_title.xml:3: Copyright 2016 The Chromium Authors. All rights reserved. s/2016/2017 https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/photo_picker_dialog_title.xml:23: <include layout="@layout/number_roll_view" /> SelectableListToolbar inflates the number_roll_view so it shouldn't need to be included here: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/picker_bitmap_view.xml (right): https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/picker_bitmap_view.xml:3: Copyright 2016 The Chromium Authors. All rights reserved. s/2016/2017 https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/picker_bitmap_view.xml:8: <!-- Represents a single item in the DownloadHistoryAdapterView. --> This comment needs to be updated. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/picker_bitmap_view.xml:16: <!-- android:background="?android:attr/selectableItemBackground" --> Remove this left over comment https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/picker_bitmap_view.xml:17: <RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android" xmlns doesn't need to be defined twice
On 2017/03/20 17:15:57, Finnur wrote: > Looks a bit daunting, but about two-thirds of the files are png, xml and string > changes. :) Despite all of the pngs, xml and string changes, there are 1700+ lines in Java files in this CL, which is indeed daunting. Is there any way to split up the CL to make it a bit more manageable to review? E.g. is there some scaffolding that can be split off? Or some functionality that can be stubbed out and landed in a separate CL (for example, the SelectFileDialog changes may a good candidate at first glance)?
I haven't updated the code, just wanted to have a round of discussion before deciding on how to proceed. > From a very high-level, what's the reason(s) that the photo picker isn't using the SelectableListLayout class? The PhotoPicker lives in two places: Both in Chromium-land but also stand-alone in an Android Studio project, which has minimal code from Chromium. The idea is that it should be reusable so the fewer the dependencies on Chromium the easier it is to adapt the picker to other projects. I've so far only needed to port over a handful of Chromium files to get to a working picker solution, which is ideal because keeping more files in sync with Chromium is costly. Adding SelectableListToolbar pulled in all kinds of new dependencies when I tried to add it to the Android Studio project. The file count had already ballooned by about 50% (+11 files) part-way through my attempt at reaching a working solution and that's just counting the java files. I hadn't even finished porting SelectableListToolbar and it's dependencies. There's also a bunch of bitmaps and xml files (layout, colors, dimensions) that come with it and a string which relies in ICU. But at that point I started wondering... what if we flip the question around? What do we gain by using SelectableListLayout? I already have a working recycler view and a toolbar. I don't need an EmptyView since my recyclerview shows items even when there are no photos on disk. I also don't need the search functionality, which seems like a fairly decent chunk of the SelectableListLayout implementation... What's your take on this? Is it worth pursuing?
On 2017/03/23 14:56:01, Finnur wrote: > I haven't updated the code, just wanted to have a round of discussion before > deciding on how to proceed. > > > From a very high-level, what's the reason(s) that the photo picker isn't using > the SelectableListLayout class? > > The PhotoPicker lives in two places: Both in Chromium-land but also stand-alone > in an Android Studio project, which has minimal code from Chromium. The idea is > that it should be reusable so the fewer the dependencies on Chromium the easier > it is to adapt the picker to other projects. I've so far only needed to port > over a handful of Chromium files to get to a working picker solution, which is > ideal because keeping more files in sync with Chromium is costly. > > Adding SelectableListToolbar pulled in all kinds of new dependencies when I > tried to add it to the Android Studio project. The file count had already > ballooned by about 50% (+11 files) part-way through my attempt at reaching a > working solution and that's just counting the java files. I hadn't even finished > porting SelectableListToolbar and it's dependencies. There's also a bunch of > bitmaps and xml files (layout, colors, dimensions) that come with it and a > string which relies in ICU. > > But at that point I started wondering... what if we flip the question around? > What do we gain by using SelectableListLayout? I already have a working recycler > view and a toolbar. I don't need an EmptyView since my recyclerview shows items > even when there are no photos on disk. I also don't need the search > functionality, which seems like a fairly decent chunk of the > SelectableListLayout implementation... > > What's your take on this? Is it worth pursuing? From a porting perspective, using fewer Chromium dependencies makes a lot of sense. From a Chromium perspective, however, I have a very strong preference for using common widgets regardless of the widget having more functionality than is needed for a specific application. Re-using SelectableListLayout helps ensure that styling changes for selectable lists of things (e.g. toolbar colors or sizes) can be made in one place. It also allows us to update functionality in one place if needed (e.g. maybe we need to add extra support for accessibility down the road). There are other thing SelectableListLayout provides in terms of functionality: - Toolbar shadow and control over its visibility. I think the photo picker is going to want this. - Loading view: is there going to be a loading view while we determine the number of photos? - Updating display style based on configuration changes through the widget.displaystyle classes. We use this to width constrain the history UI to 600dp. Downloads and bookmarks still need a bit of refactoring but soon they will be width constrained too. This seems like something we should do for the photo picker as well; let's check with UX (and if they say no, there should be a solid reason why our UIs are diverging). I think we may be able to get re-use for consistency inside Chromium and fewer dependencies for the port by splitting the SelectableListLayout (and/or some of the other selection widgets) into a base class and a class that supports X extra feature(s).
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:160001) has been deleted
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I've taken all your comments into consideration, I believe, and adapted the SelectableListLayout. I've also trimmed down the CL, so three files are gone along with about 330+ lines of code. Hope this is more manageable. PTAL.
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Now with comments. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... File chrome/android/java/res/drawable/file_picker_scrim.xml (right): https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/drawable/file_picker_scrim.xml:1: <?xml version="1.0" encoding="utf-8"?> On 2017/03/21 15:36:59, Theresa wrote: > This needs a copyright comment. Done. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/drawable/file_picker_scrim.xml:4: android:startColor="#42000000" Done. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/drawable/file_picker_scrim.xml:5: android:endColor="#00000000" On 2017/03/21 15:36:59, Theresa wrote: > nit: @android:color/black Done. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... File chrome/android/java/res/drawable/file_picker_selected_background.xml (right): https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/drawable/file_picker_selected_background.xml:1: <?xml version="1.0" encoding="utf-8"?> On 2017/03/21 15:36:59, Theresa wrote: > This needs a copyright comment too. Done. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/drawable/file_picker_selected_background.xml:11: <solid android:color="#ffffff" /> On 2017/03/21 15:36:59, Theresa wrote: > @android:color/white Done. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/bitmap_list_row.xml (right): https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/bitmap_list_row.xml:3: Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/21 15:37:00, Theresa wrote: > s/2016/2017 Done. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/bitmap_list_row.xml:21: android:contentDescription="@string/clear_cookies_and_site_data_title" Not really a placeholder. I used another string to get around a lint error somewhere, as I didn't think of @null. Meant to get back to it, but forgot. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/bitmap_list_row.xml:25: android:id="@+id/selected" On 2017/03/21 15:36:59, Theresa wrote: > Is this for the selected background? If so, selected_bg or selected_background > would be a good id, and the content description can be @null. Done. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/bitmap_list_row.xml:32: <ImageView On 2017/03/21 15:37:00, Theresa wrote: > nit: blank line before this ImageView Done. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/photo_picker_dialog_content.xml (right): https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/photo_picker_dialog_content.xml:3: Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/21 15:37:00, Theresa wrote: > s/2016/2017 Redundant, file is gone. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/photo_picker_dialog_content.xml:12: android:id="@+id/layout" Ditto. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/photo_picker_dialog_title.xml (right): https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/photo_picker_dialog_title.xml:3: Copyright 2016 The Chromium Authors. All rights reserved. Redundant, file is gone. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/photo_picker_dialog_title.xml:23: <include layout="@layout/number_roll_view" /> Ditto. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/picker_bitmap_view.xml (right): https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/picker_bitmap_view.xml:3: Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/21 15:37:00, Theresa wrote: > s/2016/2017 Done. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/picker_bitmap_view.xml:8: <!-- Represents a single item in the DownloadHistoryAdapterView. --> On 2017/03/21 15:37:00, Theresa wrote: > This comment needs to be updated. Done. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/picker_bitmap_view.xml:16: <!-- android:background="?android:attr/selectableItemBackground" --> On 2017/03/21 15:37:00, Theresa wrote: > Remove this left over comment Done. https://codereview.chromium.org/2758313002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/picker_bitmap_view.xml:17: <RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android" On 2017/03/21 15:37:00, Theresa wrote: > xmlns doesn't need to be defined twice Done.
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...)
I got through a rough first pass. This is patch is HUGE, though, and as a result, really hard and time consuming to review. There seems to be a lot of code for fetching lists of files, setting images, etc. Could this be split up more? Maybe into something like: 1) Scaffolding - just get the base classes in place, display some dummy items 2) Populate with real items 3) Add styling for selected vs unselected Or any other slicing that allows smaller, more incremental changes. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... File chrome/android/java/res/drawable/file_picker_selected_background.xml (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/drawable/file_picker_selected_background.xml:6: <inset xmlns:android="http://schemas.android.com/apk/res/android" When this background is combined with ic_check_circle_black_24dp, it looks verify_checkmark.png, right? Can we just use that asset? https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... File chrome/android/java/res/layout/photo_picker_toolbar.xml (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/photo_picker_toolbar.xml:3: Copyright 2016 The Chromium Authors. All rights reserved. s/2016/2017 https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... File chrome/android/java/res/layout/picker_bitmap_view.xml (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/picker_bitmap_view.xml:20: android:orientation="vertical"> android:orientation is used with LinearLayout not RelativeLayout https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/picker_bitmap_view.xml:51: <ImageView nit: add a blank line above this one https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... File chrome/android/java/res/layout/picker_category_view.xml (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/picker_category_view.xml:1: <?xml version="1.0" encoding="utf-8"?> Is this file used anywhere anymore? https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/picker_category_view.xml:3: Copyright 2016 The Chromium Authors. All rights reserved. s/2016/2017 https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... File chrome/android/java/res/menu/file_picker_menu.xml (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/menu/file_picker_menu.xml:22: android:textColor="#55555555" Ideally this menu item would not have a custom colors for the toolbar text and would rather use consistent styling with our other selectable lists of things. In the spec this looks white to me. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... File chrome/android/java/res/values/colors.xml (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/values/colors.xml:32: <color name="black_alpha_00">#00000000</color> Isn't this just transparent (@android:color/transparent)? https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/values/colors.xml:33: <color name="white_alpha_30">#4d000000</color> #000000 is black, not white. progress_bar_background_white is poorly named -- it's the background color for progress_bar_foreground_white rather than actually being white itself. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/values/colors.xml:183: <color name="file_picker_tile_bg_color">#eeeeee</color> @color/google_grey_200 (defined in a different in-flight CL) https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/values/colors.xml:184: <color name="file_picker_special_tile_bg_color">#263238</color> @color/dar_action_bar_color https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/values/colors.xml:185: <color name="file_picker_special_tile_color">#fff</color> @android:color/white https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/values/colors.xml:186: <color name="file_picker_special_tile_disabled_bg_color">#e0e0e0</color> @color/google_grey_300 https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/values/colors.xml:187: <color name="file_picker_special_tile_disabled_color">#bdbdbd</color> @color/google_grey_400 https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/AttrAcceptFileFilter.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/AttrAcceptFileFilter.java:15: class AttrAcceptFileFilter implements FileFilter { Please document this class https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. s/2016/2017 https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:40: return null; nit: in line "return null" and add blank space: ... Bitmap bitmap = .. if (bitmap == null) return null; return sizeBitmap(bitmap, width); https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:57: options.inJustDecodeBounds = false; nit: Maybe there should be a helper method that creates these options? Does the BitmapFactory#decode* method need to be interleaved with setting options? https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:66: * Calculates the sub-sampling factor (inSampleSize option from the BitmapFactory) for a given nit: {@link BitmapFactory#inSampleSize}? https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:93: * @param filter true if the source should be filtered. nit: s/true/True https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. s/2016/2017 https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:48: * Enumerates (in the background) the image files on disk. Called on a non-UI thread nit: Please add an assert to the method to check that this is a non-UI thread. See ThreadUtils#runningOnUiThread(). https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:55: return null; nit: inline above if (isCancelled()) return null; https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:61: paths[0] = "/DCIM/Camera"; How confident are we that the directories will be labeled this way? Maybe we should look at the logic used in other apps (e.g. Google Photos) to make sure we're finding images in a consistent way. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:99: return file.endsWith(".jpg") || file.endsWith(".gif") || file.endsWith(".png"); This list should probably include ".webp" and ".bmp" https://developer.android.com/guide/topics/media/media-formats.html#image-for... I think relying on the extension is error prone, since files can be downloaded from online with no extension or the incorrect extension. We should probably be relying on the mime type instead if possible. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. s/2016/2017 https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerToolbar.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerToolbar.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. s/2016/2017 https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerAdapter.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerAdapter.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. s/2016/2017 https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmap.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmap.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. s/2016/2017 https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:129: if (mCategoryView == null) return; // Android Studio calls onMeasure to draw the widget. Why does it matter if Android Studio calls onMeasure? This comment should be more applicable to Chromium. I think it's probably not necessary -- the early return makes sense (to me) without it. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:147: public void onClick() { We had to introduce some special handling for bookmark items that weren't selectable (it's since been mostly removed), but maybe this indicates that having some items that are not selectable in the list is an acceptable patter than needs to be backed into the selection widget classes. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:196: animation.setDuration(100); nit: define this as a constant somewhere In the future, a good way to break up a really large CL like this is to focus on base implementation first and add things like animation in a follow-on. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:259: float pixels = 48 * ((float) metrics.densityDpi / DisplayMetrics.DENSITY_DEFAULT); Define a dimension in dimens.xml rather than programatically calculating the pixels. Is this scaling the 24dp assets to 48dp? https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:261: resources, Bitmap.createScaledBitmap(icon, (int) pixels, (int) pixels, true)); Android will automatically scale for you if the ImageView's scale type is set correctly. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:274: * @return true if no image was loaded before (e.g. not even a low-res image). nit:s/true/True https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:283: mIconView.getLayoutParams().width = mCategoryView.getImageSize() - 2 * mBorder; nit: extract this calculation into a helper method. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:301: mIconView.animate().alpha(1.0f).setDuration(200).start(); nit: define 200 as a constant somewhere. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:57: private boolean mMultiSelection; nit: mIsMultiSelectionAllowed? or mIsMultiSelectionEnabled? https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:81: // are a mix). nit: Should these be JavaDoc /** */ comments so that it's easy to get hints-on-hover in IDEs when looking at these variables elsewhere in the file? https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:143: mRecyclerView.setItemAnimator(new DefaultItemAnimator()); This shouldn't be necessary. By default, RecyclerView uses DefaultItemAnimator. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:155: mBitmapUnselected = colorBitmap(mBitmapUnselected, unselectedColor); Typically we would set the src for an ImageView in XML. You can tint using TintedImageView rather than manually tinting a bitmap. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:163: return bitmap.getByteCount() / 1024; nit: define a constant for 1024 https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:200: setVisibility(View.GONE); Can this ever be empty, since there's the camera and gallery? https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectionDelegate.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectionDelegate.java:19: // True if the SelectionDelegate should only support a single item being selected at a time. The concept of single vs multi-selection would have been good to pull into a separate change as well.
No code update, just a question. And sorry about the size of the CL. I'd like to try to break it down, but in doing so I also don't want to create more work for you. What I mean is that you seem to have already reviewed some of the files/features that you propose cutting, like animation, file enumeration, styling, etc. Are you sure you want to move those to a separate CL and review them again from scratch at a later date? :)
On 2017/03/30 16:33:04, Finnur wrote: > No code update, just a question. > > And sorry about the size of the CL. I'd like to try to break it down, but in > doing so I also don't want to create more work for you. > > What I mean is that you seem to have already reviewed some of the files/features > that you propose cutting, like animation, file enumeration, styling, etc. Are > you sure you want to move those to a separate CL and review them again from > scratch at a later date? :) Yes, I'm sure. I'd rather re-review in manageable chunks than do a fine-grained review as part of a 1700 line patch.
This is a code update, but does not yet address the size of the CL. I'm working on that next, just wanted to address the comments you made first with minor changes. Next up: Putting the CL on diet. Feel free to review just the delta since last time or you can wait until next update and do a full review. Thanks! :) https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... File chrome/android/java/res/drawable/file_picker_selected_background.xml (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/drawable/file_picker_selected_background.xml:6: <inset xmlns:android="http://schemas.android.com/apk/res/android" Good catch! Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... File chrome/android/java/res/layout/photo_picker_toolbar.xml (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/photo_picker_toolbar.xml:3: Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/28 20:40:27, Theresa wrote: > s/2016/2017 Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... File chrome/android/java/res/layout/picker_bitmap_view.xml (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/picker_bitmap_view.xml:20: android:orientation="vertical"> On 2017/03/28 20:40:27, Theresa wrote: > android:orientation is used with LinearLayout not RelativeLayout Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/picker_bitmap_view.xml:51: <ImageView On 2017/03/28 20:40:27, Theresa wrote: > nit: add a blank line above this one Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... File chrome/android/java/res/layout/picker_category_view.xml (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/picker_category_view.xml:1: <?xml version="1.0" encoding="utf-8"?> On 2017/03/28 20:40:27, Theresa wrote: > Is this file used anywhere anymore? Good point. Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/picker_category_view.xml:3: Copyright 2016 The Chromium Authors. All rights reserved. File redundant. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... File chrome/android/java/res/menu/file_picker_menu.xml (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/menu/file_picker_menu.xml:22: android:textColor="#55555555" Done. (Removed the custom color). https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... File chrome/android/java/res/values/colors.xml (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/values/colors.xml:32: <color name="black_alpha_00">#00000000</color> On 2017/03/28 20:40:27, Theresa wrote: > Isn't this just transparent (@android:color/transparent)? Indeed! Removed. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/values/colors.xml:33: <color name="white_alpha_30">#4d000000</color> Ah, right. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/values/colors.xml:183: <color name="file_picker_tile_bg_color">#eeeeee</color> On 2017/03/28 20:40:27, Theresa wrote: > @color/google_grey_200 (defined in a different in-flight CL) Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/values/colors.xml:184: <color name="file_picker_special_tile_bg_color">#263238</color> Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/values/colors.xml:185: <color name="file_picker_special_tile_color">#fff</color> On 2017/03/28 20:40:27, Theresa wrote: > @android:color/white Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/re... chrome/android/java/res/values/colors.xml:187: <color name="file_picker_special_tile_disabled_color">#bdbdbd</color> Done. Thinking about this some more; it would be handy if the presubmit script checked if someone is adding a pre-existing color. The script could standardize on upper/lowercase as well for the hex numbers. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/AttrAcceptFileFilter.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/AttrAcceptFileFilter.java:15: class AttrAcceptFileFilter implements FileFilter { On 2017/03/28 20:40:27, Theresa wrote: > Please document this class Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/28 20:40:27, Theresa wrote: > s/2016/2017 Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:40: return null; On 2017/03/28 20:40:27, Theresa wrote: > nit: in line "return null" and add blank space: > > > ... > Bitmap bitmap = .. > > if (bitmap == null) return null; > > return sizeBitmap(bitmap, width); Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:57: options.inJustDecodeBounds = false; Done (I think). https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:66: * Calculates the sub-sampling factor (inSampleSize option from the BitmapFactory) for a given On 2017/03/28 20:40:27, Theresa wrote: > nit: {@link BitmapFactory#inSampleSize}? Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:93: * @param filter true if the source should be filtered. On 2017/03/28 20:40:27, Theresa wrote: > nit: s/true/True Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/28 20:40:28, Theresa wrote: > s/2016/2017 Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:48: * Enumerates (in the background) the image files on disk. Called on a non-UI thread On 2017/03/28 20:40:28, Theresa wrote: > nit: Please add an assert to the method to check that this is a non-UI thread. > See ThreadUtils#runningOnUiThread(). Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:55: return null; On 2017/03/28 20:40:28, Theresa wrote: > nit: inline above > > if (isCancelled()) return null; Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:61: paths[0] = "/DCIM/Camera"; Acknowledged. Will change it in a future CL, since I'm removing this file shortly (from this CL). https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:99: return file.endsWith(".jpg") || file.endsWith(".gif") || file.endsWith(".png"); Acknowledged. Will change it in a future CL, since I'm removing this file shortly (from this CL). https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/28 20:40:28, Theresa wrote: > s/2016/2017 Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerToolbar.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerToolbar.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/28 20:40:28, Theresa wrote: > s/2016/2017 Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerAdapter.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerAdapter.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/28 20:40:28, Theresa wrote: > s/2016/2017 Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmap.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmap.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/28 20:40:28, Theresa wrote: > s/2016/2017 Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:129: if (mCategoryView == null) return; // Android Studio calls onMeasure to draw the widget. Excellent. I worried it would not make sense without it. :) https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:147: public void onClick() { On 2017/03/28 20:40:28, Theresa wrote: > We had to introduce some special handling for bookmark items that weren't > selectable (it's since been mostly removed), but maybe this indicates that > having some items that are not selectable in the list is an acceptable patter > than needs to be backed into the selection widget classes. Acknowledged. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:196: animation.setDuration(100); On 2017/03/28 20:40:28, Theresa wrote: > nit: define this as a constant somewhere > > > In the future, a good way to break up a really large CL like this is to focus on > base implementation first and add things like animation in a follow-on. Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:259: float pixels = 48 * ((float) metrics.densityDpi / DisplayMetrics.DENSITY_DEFAULT); On 2017/03/28 20:40:28, Theresa wrote: > Define a dimension in dimens.xml rather than programatically calculating the > pixels. > > Is this scaling the 24dp assets to 48dp? Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:261: resources, Bitmap.createScaledBitmap(icon, (int) pixels, (int) pixels, true)); It is not obvious to me how to interpret this comment... https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:274: * @return true if no image was loaded before (e.g. not even a low-res image). On 2017/03/28 20:40:28, Theresa wrote: > nit:s/true/True Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:283: mIconView.getLayoutParams().width = mCategoryView.getImageSize() - 2 * mBorder; On 2017/03/28 20:40:28, Theresa wrote: > nit: extract this calculation into a helper method. Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:301: mIconView.animate().alpha(1.0f).setDuration(200).start(); On 2017/03/28 20:40:28, Theresa wrote: > nit: define 200 as a constant somewhere. Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:57: private boolean mMultiSelection; On 2017/03/28 20:40:29, Theresa wrote: > nit: mIsMultiSelectionAllowed? or mIsMultiSelectionEnabled? Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:81: // are a mix). On 2017/03/28 20:40:29, Theresa wrote: > nit: Should these be JavaDoc /** */ comments so that it's easy to get > hints-on-hover in IDEs when looking at these variables elsewhere in the file? Done. Unless you mean all of them? https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:143: mRecyclerView.setItemAnimator(new DefaultItemAnimator()); On 2017/03/28 20:40:29, Theresa wrote: > This shouldn't be necessary. By default, RecyclerView uses DefaultItemAnimator. Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:155: mBitmapUnselected = colorBitmap(mBitmapUnselected, unselectedColor); Neat. Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:163: return bitmap.getByteCount() / 1024; On 2017/03/28 20:40:29, Theresa wrote: > nit: define a constant for 1024 Done. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:200: setVisibility(View.GONE); Removed. I believe this is from back when the Camera and Gallery tile hadn't been added. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectionDelegate.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectionDelegate.java:19: // True if the SelectionDelegate should only support a single item being selected at a time. Acknowledged. Removing this file for now.
OK, I've trimmed away a bunch of stuff. Hopefully it is more manageable now. PTAL.
On 2017/03/31 16:07:09, Finnur wrote: > OK, I've trimmed away a bunch of stuff. Hopefully it is more manageable now. > PTAL. Basically, it just shows 4 hardcoded images in a grid. No animations, selection rings, proper spacing, caching, etc. Pretty basic.
Thank you for splitting this up! Please run "tools/resources/optimize-png-files.sh -o2" on the new assets if you haven't already. Can the ic_check_circle_black_24dp.png be a shape drawable since it's so simple? ic_collections_grey.xml is a vector drawable version of the ic_collections_black_24dp. Let's remove one or the other rather than duplicating assets. Unless they need to be different sizes, then let's chat. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:147: public void onClick() { On 2017/03/31 14:26:50, Finnur wrote: > On 2017/03/28 20:40:28, Theresa wrote: > > We had to introduce some special handling for bookmark items that weren't > > selectable (it's since been mostly removed), but maybe this indicates that > > having some items that are not selectable in the list is an acceptable patter > > than needs to be backed into the selection widget classes. > > Acknowledged. I was thinking about this incorrectly -- this actually looks correct to me. The other selectable lists of things take some action other than selection on click. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:1: // Copyright 2017 The Chromium Authors. All rights reserved. Is this class being used anywhere currently? https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:35: Bitmap bitmap = BitmapFactory.decodeFileDescriptor( Why does this call decodeFileDescriptor twice? Same question for the double call to decodeFile in #decodeBitmapFromDisk below. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DefaultAcceptFileFilter.java (right): https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DefaultAcceptFileFilter.java:11: class DefaultAcceptFileFilter extends AcceptFileFilter { I'm not seeing the AcceptFileFilter base class -- is this an Android class or a Chromium class? It also looks like this file isn't used in this patch, should it be removed? https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:96: mSelectionDelegate.toggleSelectionForItem(mRequest); I think this could be simplified to calling super.onLongClick(this), which calls toggleSelectionForItem and setChecked(). A comment indicating that a regular click on an item in the photo picker selects it would be helpful. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:116: updateSelectionState(); Instead of unselecting the camera/gallery here, could you override toggleSelectionForItem() to return false if the item type isn't PICTURE and call super if the type is PICTURE? https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:123: super.toggle(); I'm confused about how the item could be !selected but checked? Is it because this isn't calling super before this check? I think that this is trying to unset the checked state if this multi-selection isn't allowed. Can this be done earlier then the onSelectionStateChange() call? https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:156: setOnClickListener(this); The superclass already calls setOnClickListener() after inflation is finished. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:163: int size = mCategoryView.getImageSize(); You shouldn't need to programatically construct a bitmap for the camera and gallery icons. Rahter than scaling the camera and collections 24dp icons up to file_picker_special_icon_size (which is 48dp?), you should just include the 48 x 48dp icons (or SVG equivalents) and use something like this: Drawable image = ApiCompatibilityUtils.getDrawable(resources, iconBitmapId); ApiCompatibilityUtils.setCompoundDrawablesRelativeWithIntrinsicBounds(mSpecialTile, null, drawable, null, null); https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:218: private static void addPaddingToParent(View view, int padding) { Is anything calling this anymore? https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:241: bgColorId = R.color.file_picker_special_tile_disabled_bg_color; If multi-select is allowed, won't this set the set the bg color for special tiles to disabled regardless of whether selection is enabled? https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:245: mBorderView.setBackgroundColor(ContextCompat.getColor(mContext, bgColorId)); Typically we use ApiCompatibilityUtils#getColor() https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:41: public void imageDecodedCallback(String filePath, Bitmap bitmap) { nit: JavaDoc https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:68: mItemView.initializeSpecialTile(); Can PickerBitmapView call initializeSpecialTile directly from #initialize() if needed? https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:114: View view = ((Activity) context).getWindow().getDecorView(); getDecorView() contains all of the window decoration (e.g. Android status bar, bottom controls). Is that desired? https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:134: public void setStartingState( The values set in this don't appear to change so #setStartinState() doesn't seem like an appropriate method name. Typically I'd recommend #initialize() but there's already an #init() method... https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:135: PhotoPickerDialog dialog, PhotoPickerListener listener, boolean multiSelection) { Please rename this parameter to match mMultiSelectionAllowed. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:161: public SelectionDelegate<PickerBitmap> getSelectionDelegate() { nit: add a blank space above this method and the others in this block as well. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:167: public boolean isMultiSelect() { Please rename this method as well. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:191: private Bitmap colorBitmap(Bitmap original, int color) { It looks like this is no longer used. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:233: * Tries to notify any listeners that one or more photos have been selected. When would it not successfully notify the listener? I'm thinking this can just be notfiyPhotosSelected()
I'll make a pass at removing the image assets. My first attempt was unsuccessful (it just wouldn't draw anything) and I ran out of time. I think that's the only thing I didn't address in this update. PTAL. https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:147: public void onClick() { On 2017/03/31 18:05:55, Theresa wrote: > On 2017/03/31 14:26:50, Finnur wrote: > > On 2017/03/28 20:40:28, Theresa wrote: > > > We had to introduce some special handling for bookmark items that weren't > > > selectable (it's since been mostly removed), but maybe this indicates that > > > having some items that are not selectable in the list is an acceptable > patter > > > than needs to be backed into the selection widget classes. > > > > Acknowledged. > > I was thinking about this incorrectly -- this actually looks correct to me. The > other selectable lists of things take some action other than selection on click. Acknowledged. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java (right): https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:1: // Copyright 2017 The Chromium Authors. All rights reserved. Huh... You are right. Removed. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapUtils.java:35: Bitmap bitmap = BitmapFactory.decodeFileDescriptor( Because that's how the sub-sampling is implemented in Android. You call decode twice, first with inJustDecodeBounds=true (which tells you the dimensions of the image on disk) and then you can use those dimensions to set inSampleSize to decode the image to the desired thumbnail size. However, I've come to realize that I no longer need decodeBitmapFromDisk, which was the impetus for creating two utility functions below it, so I've deleted it and transformed the remaining three into one again. But that's for a later review, since this file isn't needed yet. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DefaultAcceptFileFilter.java (right): https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DefaultAcceptFileFilter.java:11: class DefaultAcceptFileFilter extends AcceptFileFilter { Yes, this became redundant in a previous simplification. Didn't realize it was still in the CL. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:96: mSelectionDelegate.toggleSelectionForItem(mRequest); Yup. Seems to work fine that way. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:116: updateSelectionState(); On 2017/03/31 18:05:56, Theresa wrote: > Instead of unselecting the camera/gallery here, could you override > toggleSelectionForItem() to return false if the item type isn't PICTURE and call > super if the type is PICTURE? Done (much better). https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:123: super.toggle(); I don't remember why this is needed anymore and since this CL no longer supports multi-selection, I think it is best to remove this and revisit at a later date. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:156: setOnClickListener(this); Removed. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:163: int size = mCategoryView.getImageSize(); I don't have much experience with how the drawables work. Is this accomplished by moving the current assets from drawable-xxxhdpi to drawable-xxhdpi and xxhdpi to xhdpi (continuing all the way down to drawable-mdpi) or is it enough to simply delete the mdpi asset? Or are you referring to something completely different? :) https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:218: private static void addPaddingToParent(View view, int padding) { Nope! Removed. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:241: bgColorId = R.color.file_picker_special_tile_disabled_bg_color; Yes, I don't know why that condition is there. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:245: mBorderView.setBackgroundColor(ContextCompat.getColor(mContext, bgColorId)); On 2017/03/31 18:05:56, Theresa wrote: > Typically we use ApiCompatibilityUtils#getColor() Done. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:41: public void imageDecodedCallback(String filePath, Bitmap bitmap) { On 2017/03/31 18:05:56, Theresa wrote: > nit: JavaDoc Done. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:68: mItemView.initializeSpecialTile(); When I went to make that change I realized initializeSpecialTile is used to figure out the right (special) icon to show and calls initialize internally to set it. Which means we don't need to call initialize at all here. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:114: View view = ((Activity) context).getWindow().getDecorView(); We only care about the width here. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:134: public void setStartingState( Yeah, I struggled with this as well. All I can think of is setup()... I don't know. Is that better? https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:135: PhotoPickerDialog dialog, PhotoPickerListener listener, boolean multiSelection) { On 2017/03/31 18:05:56, Theresa wrote: > Please rename this parameter to match mMultiSelectionAllowed. Done. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:161: public SelectionDelegate<PickerBitmap> getSelectionDelegate() { On 2017/03/31 18:05:56, Theresa wrote: > nit: add a blank space above this method and the others in this block as well. Done. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:167: public boolean isMultiSelect() { On 2017/03/31 18:05:56, Theresa wrote: > Please rename this method as well. Done. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:191: private Bitmap colorBitmap(Bitmap original, int color) { Correct. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:233: * Tries to notify any listeners that one or more photos have been selected. Yes, in an old revision this could actually result in no notification, but that's no longer the case, as you point out.
https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:163: int size = mCategoryView.getImageSize(); On 2017/04/03 17:30:29, Finnur wrote: > I don't have much experience with how the drawables work. Is this accomplished > by moving the current assets from drawable-xxxhdpi to drawable-xxhdpi and xxhdpi > to xhdpi (continuing all the way down to drawable-mdpi) or is it enough to > simply delete the mdpi asset? Or are you referring to something completely > different? :) Something completely different :). Currently you're constructing an empty "Bitmap tile". I don't think that should be needed at all (I could be wrong). Then below you're calling: Bitmap icon = BitmapFactory.decodeResource(resources, iconBitmapId); BitmapDrawable drawable = new BitmapDrawable resources, Bitmap.createScaledBitmap(icon, (int) pixels, (int) pixels, true)); ApiCompatibilityUtils.setCompoundDrawable.... You should be able to avoid programatically creating bitmaps and BitmapDrawables and do something like this: Drawable image = ApiCompatibilityUtils.getDrawable(resources, iconBitmapId); ApiCompatibilityUtils.setCompoundDrawablesRelativeWithIntrinsicBounds(mSpecialTile, null, drawable, null, null); If the PNG you're using is smaller than mCategoryView.getImageSize(), then you'll probably want to get a bigger PNG. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:114: View view = ((Activity) context).getWindow().getDecorView(); On 2017/04/03 17:30:30, Finnur wrote: > We only care about the width here. Sometimes the width contains the window decorations e.g. a Nexus 6P in landscape. If you only want the app's dimensions, use: Rect appRect = new Rect(); mActivity.getWindow().getDecorView().getWindowVisibleDisplayFrame(appRect); https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:134: public void setStartingState( On 2017/04/03 17:30:30, Finnur wrote: > Yeah, I struggled with this as well. All I can think of is setup()... I don't > know. Is that better? What about something like postConstruction() for the other method and initialize() (or init()) for this method? https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/re... File chrome/android/java/res/drawable/file_picker_selected_background.xml (right): https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/re... chrome/android/java/res/drawable/file_picker_selected_background.xml:11: <shape xmlns:android="http://schemas.android.com/apk/res/android" android:shape="oval"> nit: There's no need to declare xmlns twice; it can be removed from this line. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/re... File chrome/android/java/res/layout/bitmap_list_row.xml (right): https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/re... chrome/android/java/res/layout/bitmap_list_row.xml:15: android:orientation="vertical"> nit: remove android:orientation since this is a RelativeLayout https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/re... File chrome/android/java/res/menu/file_picker_menu.xml (right): https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/re... chrome/android/java/res/menu/file_picker_menu.xml:2: <!-- Copyright 2016 The Chromium Authors. All rights reserved. s/2016/2017 https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. s/2016/2017 https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:29: * A container class for a view showing a photo in the photo picker. nit: s/photo picker/Photo Picker for consistency in other files? https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:42: private PickerBitmap mRequest; nit: mBitmap? The PickerBitmap object isn't actually requesting anything, it's just storing data. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:103: || mRequest.type() == PickerBitmap.TileTypes.CAMERA)) { nit: && mRequest.type() != PickerBitmap.TileTypes.PICTURE to match check below? Maybe this should be pulled out into an isSelectable() helper method? https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:121: boolean selected = selectedItems.contains(mRequest); It appears this variable isn't used anymore. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:130: public void preInitialize(PickerCategoryView categoryView) { nit: Maybe we can just call this setCategoryView()?: /** * Sets the {@link PickerCategoryView} for this PickerBitmapView. * @param categoryView The category view showing the images. Used to ... and retrieve the {@link SelectionDelegate}. * / https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. s/2016/2017 https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:18: * Holds onto a View that displays information about a picker bitmap. s/onto/on to s/View/{@link PickerBitmapView}? https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:28: private PickerBitmap mRequest; Same question about this mRequest. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:32: * @param itemView The PickerBitmap view for showing the image. nit: s/PickerBitmap view/{@link PickerBitmapView} https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:37: assert itemView instanceof PickerBitmapView; Can we make itemView a PickerBitmapView instead of a generic View? https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. s/2016/2017 https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:36: // The view containing the recycler view and the toolbar, etc. nit: s/recycler view/RecyclerView https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:51: // The recycler view showing the images. s/recycler view/RecyclerView https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:54: // The picker adapter for the RecyclerView. nit: s/picker adapter/PickerAdapter https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:57: // The selection delegate keeping track of which images are selected. nit: s/selection delegate/SelectionDelegate https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:142: if (item.getItemId() == R.id.close_menu_id If close_menu_id is clicked rather than selection_mode_done_menu_id, doesn't that imply that there was no selection, in which case should we be notifying that the dialog was dismissed without selection rather than calling notifyPhotosSelected()? Maybe there should be a PhotoPickerListener.Action.CANCEL or something like that. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:218: private void notfiyPhotosSelected() { nit: fix typo with "notfiy"
PTAL. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:163: int size = mCategoryView.getImageSize(); I've introduced the vector format to this, so the comments are partly addressing code that's changed. Although I think there's probably room for improvements, I've ran out of time for today and didn't want to hold the review up for this. I'll try to improve this further tomorrow. Particularly wondering if I can use the vector directly in xml... hmmm... By the way, the png will always be smaller than getImageSize (it's a centered icon on a gray background). https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:114: View view = ((Activity) context).getWindow().getDecorView(); On 2017/04/04 15:48:32, Theresa wrote: > On 2017/04/03 17:30:30, Finnur wrote: > > We only care about the width here. > > Sometimes the width contains the window decorations e.g. a Nexus 6P in > landscape. > > If you only want the app's dimensions, use: > Rect appRect = new Rect(); > mActivity.getWindow().getDecorView().getWindowVisibleDisplayFrame(appRect); Done. https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:134: public void setStartingState( Sounds reasonable. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/re... File chrome/android/java/res/drawable/file_picker_selected_background.xml (right): https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/re... chrome/android/java/res/drawable/file_picker_selected_background.xml:11: <shape xmlns:android="http://schemas.android.com/apk/res/android" android:shape="oval"> File gone. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/re... File chrome/android/java/res/layout/bitmap_list_row.xml (right): https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/re... chrome/android/java/res/layout/bitmap_list_row.xml:15: android:orientation="vertical"> Ooh! File-be-gone! (Removed it, not needed). https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/re... File chrome/android/java/res/menu/file_picker_menu.xml (right): https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/re... chrome/android/java/res/menu/file_picker_menu.xml:2: <!-- Copyright 2016 The Chromium Authors. All rights reserved. On 2017/04/04 15:48:33, Theresa wrote: > s/2016/2017 Done. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/04/04 15:48:33, Theresa wrote: > s/2016/2017 Done. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:29: * A container class for a view showing a photo in the photo picker. On 2017/04/04 15:48:33, Theresa wrote: > nit: s/photo picker/Photo Picker for consistency in other files? Done. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:42: private PickerBitmap mRequest; I didn't like calling it mBitmap/mImage or something to that effect because standalone I would see the variable and think it was an ImageView or something like that. But I've renamed it mBitmapDetails. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:103: || mRequest.type() == PickerBitmap.TileTypes.CAMERA)) { Did it slightly differently... https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:121: boolean selected = selectedItems.contains(mRequest); On 2017/04/04 15:48:33, Theresa wrote: > It appears this variable isn't used anymore. Done. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:130: public void preInitialize(PickerCategoryView categoryView) { Also very reasonable. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/04/04 15:48:33, Theresa wrote: > s/2016/2017 Done. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:18: * Holds onto a View that displays information about a picker bitmap. On 2017/04/04 15:48:33, Theresa wrote: > s/onto/on to > > s/View/{@link PickerBitmapView}? Done. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:28: private PickerBitmap mRequest; On 2017/04/04 15:48:33, Theresa wrote: > Same question about this mRequest. Done. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:32: * @param itemView The PickerBitmap view for showing the image. On 2017/04/04 15:48:33, Theresa wrote: > nit: s/PickerBitmap view/{@link PickerBitmapView} Done. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:37: assert itemView instanceof PickerBitmapView; On 2017/04/04 15:48:33, Theresa wrote: > Can we make itemView a PickerBitmapView instead of a generic View? Done. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/04/04 15:48:34, Theresa wrote: > s/2016/2017 Done. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:36: // The view containing the recycler view and the toolbar, etc. On 2017/04/04 15:48:34, Theresa wrote: > nit: s/recycler view/RecyclerView Done. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:51: // The recycler view showing the images. On 2017/04/04 15:48:34, Theresa wrote: > s/recycler view/RecyclerView Done. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:54: // The picker adapter for the RecyclerView. On 2017/04/04 15:48:34, Theresa wrote: > nit: s/picker adapter/PickerAdapter Done. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:57: // The selection delegate keeping track of which images are selected. On 2017/04/04 15:48:34, Theresa wrote: > nit: s/selection delegate/SelectionDelegate Done. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:142: if (item.getItemId() == R.id.close_menu_id On 2017/04/04 15:48:34, Theresa wrote: > If close_menu_id is clicked rather than selection_mode_done_menu_id, doesn't > that imply that there was no selection, in which case should we be notifying > that the dialog was dismissed without selection rather than calling > notifyPhotosSelected()? > > Maybe there should be a PhotoPickerListener.Action.CANCEL or something like > that. Done. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:218: private void notfiyPhotosSelected() { On 2017/04/04 15:48:34, Theresa wrote: > nit: fix typo with "notfiy" Done.
This is getting close! https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:163: int size = mCategoryView.getImageSize(); On 2017/04/04 18:05:36, Finnur wrote: > I've introduced the vector format to this, so the comments are partly addressing > code that's changed. > > Although I think there's probably room for improvements, I've ran out of time > for today and didn't want to hold the review up for this. I'll try to improve > this further tomorrow. Particularly wondering if I can use the vector directly > in xml... hmmm... > > By the way, the png will always be smaller than getImageSize (it's a centered > icon on a gray background). Vector drawables cannot be used directly in XML - it crashes on older versions of Android. https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:42: private PickerBitmap mRequest; On 2017/04/04 18:05:37, Finnur wrote: > I didn't like calling it mBitmap/mImage or something to that effect because > standalone I would see the variable and think it was an ImageView or something > like that. But I've renamed it mBitmapDetails. Acknowledged. https://codereview.chromium.org/2758313002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:141: mBitmapDetails = request; nit: should the @param be renamed to match mBitmapDetails? https://codereview.chromium.org/2758313002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:171: collections.draw(canvas); Can collections be passed directly to ApiCompatibilityUTils#setCompoundDrawables...? We set VectorDrawable's as compound drawables for the empty text view in SelectableListLayout without issue. I'm picturing something like this: public void initializeSpecialTile(PickerBitmap request) { int labelStringId; Drawable image; Resources resources = mContext.getResources(); if (isCameraTile()) { image = ApiCompatibilityUtils.getDrawable(resources, R.drawable.ic_photo_camera); labelStringId = R.string.file_picker_camera; } else { image = VectorDrawableCompat.create( resources, R.drawable.ic_collections_grey, mContext.getTheme()); labelStringId = R.string.file_picker_browse; } ApiCompatibilityUtils.setCompoundDrawablesRelativeWithIntrinsicBounds( mSpecialTile, null, image, null, null); mSpecialTile.setText(labelStringId); initialize(request, null, false); // Reset visibility, since #initialize() sets mSpecialTile visibility to GONE. mSpecialTile.setVisibility(View.VISIBLE); } With some constructs in setThumbnailBitmap() to deal with @Nullable Bitmap thumbnail. https://codereview.chromium.org/2758313002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2758313002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:77: imageDecodedCallback(mBitmapDetails.getFilePath(), createPlaceholderBitmap(size, size)); This probably needs to call #intialize() rather than just imageDecodedCallback(). #displayItem() could be getting called because a view is getting rebound to a new adapter item, so you'll want to reset the tile background, etc.
> This is getting close! Woot! :) https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:163: int size = mCategoryView.getImageSize(); Understood. I've done it the way you suggested. Thanks! https://codereview.chromium.org/2758313002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:141: mBitmapDetails = request; On 2017/04/04 18:40:46, Theresa wrote: > nit: should the @param be renamed to match mBitmapDetails? Done. https://codereview.chromium.org/2758313002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:171: collections.draw(canvas); Done. Except the last part (constructs for null bitmaps). Is it necessary to deal with a null bitmap specifically? https://codereview.chromium.org/2758313002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2758313002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:77: imageDecodedCallback(mBitmapDetails.getFilePath(), createPlaceholderBitmap(size, size)); I'd like to ignore this for now because this is call is temporary. Once the decoder is in place, the flow will be different.
These should be the last of the nits/comments :) https://codereview.chromium.org/2758313002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:171: collections.draw(canvas); On 2017/04/05 15:14:39, Finnur wrote: > Done. Except the last part (constructs for null bitmaps). Is it necessary to > deal with a null bitmap specifically? If Android is happy with a null passed in to #setImageBitmap() then there's no need add special handling. https://codereview.chromium.org/2758313002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2758313002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:77: imageDecodedCallback(mBitmapDetails.getFilePath(), createPlaceholderBitmap(size, size)); On 2017/04/05 15:14:39, Finnur wrote: > I'd like to ignore this for now because this is call is temporary. Once the > decoder is in place, the flow will be different. Acknowledged. https://codereview.chromium.org/2758313002/diff/320001/chrome/android/java/re... File chrome/android/java/res/layout/picker_bitmap_view.xml (right): https://codereview.chromium.org/2758313002/diff/320001/chrome/android/java/re... chrome/android/java/res/layout/picker_bitmap_view.xml:20: android:layout_gravity="center"> I think that this RelativeLayout probably isn't necessary since none of the relative layout specific things are used on the child views. Can you please try moving the android:layout_gravity param to PickerBitmapView and removing this RelativeLayout? https://codereview.chromium.org/2758313002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerAdapter.java (right): https://codereview.chromium.org/2758313002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerAdapter.java:49: public void onBindViewHolder(ViewHolder holder, int position, List payloads) { Since payloads isn't used, can you move the contents of this to the other 2-param version of the method? https://codereview.chromium.org/2758313002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:150: * @param bitmapDetails The request represented by this PickerBitmapView. nit: Rework the @param description here too https://codereview.chromium.org/2758313002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2758313002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:106: R.string.menu_history, null, R.id.file_picker_normal_menu_group, This should be passing something other than R.string.menu_history for the title resource ID. It's acceptable to pass 0 if the SelectableListToolbar isn't responsible for setting the title. https://codereview.chromium.org/2758313002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:173: * Notifies the caller that the user selected to launch the gallery. nit: s/caller/listener here and below
Another quick thought, it looks like file_picker_scrim.xml, file_picker_selected_background.xml and bitmap_list_row.xml aren't used in this patch. Should we wait and commit them with the next CL?
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> Should we wait and commit them with the next CL? Absolutely. All comments addressed, and some minor additional cleanup performed. PTAL. https://codereview.chromium.org/2758313002/diff/320001/chrome/android/java/re... File chrome/android/java/res/layout/picker_bitmap_view.xml (right): https://codereview.chromium.org/2758313002/diff/320001/chrome/android/java/re... chrome/android/java/res/layout/picker_bitmap_view.xml:20: android:layout_gravity="center"> Done. Moved layout_gravity to the two items that need it, instead of doing it in code. Seemed simpler that way. https://codereview.chromium.org/2758313002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerAdapter.java (right): https://codereview.chromium.org/2758313002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerAdapter.java:49: public void onBindViewHolder(ViewHolder holder, int position, List payloads) { Huh... interesting. I distinctly remember relying on this function override explicitly (think it was for selection), but now I can do without it. Weird. Anyway. Gone. https://codereview.chromium.org/2758313002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:150: * @param bitmapDetails The request represented by this PickerBitmapView. On 2017/04/05 15:48:24, Theresa wrote: > nit: Rework the @param description here too Done. https://codereview.chromium.org/2758313002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2758313002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:106: R.string.menu_history, null, R.id.file_picker_normal_menu_group, Done (and a few string removals/renames as per file picker->photo picker). https://codereview.chromium.org/2758313002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:173: * Notifies the caller that the user selected to launch the gallery. On 2017/04/05 15:48:24, Theresa wrote: > nit: s/caller/listener here and below Done.
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...)
lgtm https://codereview.chromium.org/2758313002/diff/340001/chrome/android/java/re... File chrome/android/java/res/drawable/circle_white.xml (right): https://codereview.chromium.org/2758313002/diff/340001/chrome/android/java/re... chrome/android/java/res/drawable/circle_white.xml:2: <!-- Copyright 2017 The Chromium Authors. All rights reserved. It looks like this file isn't used, remove for now? https://codereview.chromium.org/2758313002/diff/340001/chrome/android/java/re... File chrome/android/java/res/layout/picker_bitmap_view.xml (right): https://codereview.chromium.org/2758313002/diff/340001/chrome/android/java/re... chrome/android/java/res/layout/picker_bitmap_view.xml:3: Copyright 2017 The Chromium Authors. All rights reserved. nit: photo_picker_bitmap_view.xml for consistency with other file names?
https://codereview.chromium.org/2758313002/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:203: boolean checked = super.isChecked(); nit: findbugs reported a deadstore for this variable. https://codereview.chromium.org/2758313002/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2758313002/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:66: String filePath = mBitmapDetails.getFilePath(); nit: findbugs reported a deadstore for this variable
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Description was changed from ========== Implement the new Photo picker, part two. This replaces the "Not implemented" Toast with a fully working dialog, except the images used are all placeholder gray tiles (instead of actual decoded images). BUG=656015 ========== to ========== Implement the new Photo picker, part two. This replaces the "Not implemented" Toast with a fully working dialog, except the images used are all placeholder gray tiles (instead of actual decoded images). BUG=656015 ==========
finnur@chromium.org changed reviewers: + tedchoc@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, Theresa. Ted, mind providing an OWNERS check?
Forgot to publish my comments... https://codereview.chromium.org/2758313002/diff/340001/chrome/android/java/re... File chrome/android/java/res/drawable/circle_white.xml (right): https://codereview.chromium.org/2758313002/diff/340001/chrome/android/java/re... chrome/android/java/res/drawable/circle_white.xml:2: <!-- Copyright 2017 The Chromium Authors. All rights reserved. Ah, right. Done. https://codereview.chromium.org/2758313002/diff/340001/chrome/android/java/re... File chrome/android/java/res/layout/picker_bitmap_view.xml (right): https://codereview.chromium.org/2758313002/diff/340001/chrome/android/java/re... chrome/android/java/res/layout/picker_bitmap_view.xml:3: Copyright 2017 The Chromium Authors. All rights reserved. On 2017/04/06 15:13:52, Theresa wrote: > nit: photo_picker_bitmap_view.xml for consistency with other file names? Done. https://codereview.chromium.org/2758313002/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:203: boolean checked = super.isChecked(); On 2017/04/06 15:15:29, Theresa wrote: > nit: findbugs reported a deadstore for this variable. Done. https://codereview.chromium.org/2758313002/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java (right): https://codereview.chromium.org/2758313002/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java:66: String filePath = mBitmapDetails.getFilePath(); On 2017/04/06 15:15:29, Theresa wrote: > nit: findbugs reported a deadstore for this variable Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) 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 finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Fixed compile error due to file rename. (Failed on bots but worked locally because the old build artifacts were still around).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java (right): https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:63: private PhotoPickerDialog mDialog; will this ever need to be used outside of the delegate? If not, we should probably just have this within the inner class. https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java (right): https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java:29: * selected. Nit. For multi-line javadoc, we should align with the text after the boilerplate (i.e. "True if the ..."), for @return, you would align the text with what is right after the @return for example. As an even more nit of nits, I prefer to use whether for boolean statements which would read like "Whether the photo picker should...". https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmap.java (right): https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmap.java:8: * A class to keep track of the meta data associated with a an image in the wrap javadoc lines to 100 https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmap.java:13: public enum TileTypes { PICTURE, CAMERA, GALLERY } We typically try to avoid using enums in java land. Any thought on using ints and a corresponding @IntDef here? https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmap.java:59: if (mLastModified < other.mLastModified) { could this be: return ApiCompatibilityUtils.compareLong(other.mLastModified, mLastModified); ? https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmap.java:73: public boolean equals(Object other) { Hmm...this seems a bit weird to me. Why do you need it to do comparison equals vs variable equals? I've found that when I do this I'm usually special casing a behavior into an object class that should likely live in the feature code (I recently did something similar with search engines and it bit me a week later). https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:74: protected void onFinishInflate() { nit, I'd put this right under the constructor as it is really the stuff that completes the view https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:93: super.onLongClick(this); Do you specifically need super. here? That would skip any overrides you do at any point in the future. You'll want to make sure this doesn't do wonky things in accessibility (at some point in the future). Mixing click and long click might confuse it. https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:125: super.setSelectionDelegate(mSelectionDelegate); same comment where I don't think you need the super call here since you're not already inside of setSelectionDelegate https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:205: int bgColorId, fgColorId; one init per line plz :-) https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:32: private static final int KILOBYTE = 1024; unused
Patchset #16 (id:400001) has been deleted
Patchset #16 (id:420001) has been deleted
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. If it meets your bar, feel free to check the CQ box after giving the green light. Thanks! https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java (right): https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:63: private PhotoPickerDialog mDialog; On 2017/04/06 23:52:15, Ted C wrote: > will this ever need to be used outside of the delegate? If not, we should > probably just have this within the inner class. Done. https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java (right): https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java:29: * selected. On 2017/04/06 23:52:15, Ted C wrote: > Nit. > > For multi-line javadoc, we should align with the text after the boilerplate > (i.e. "True if the ..."), for @return, you would align the text with what is > right after the @return for example. > > As an even more nit of nits, I prefer to use whether for boolean statements > which would read like "Whether the photo picker should...". Done. https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmap.java (right): https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmap.java:8: * A class to keep track of the meta data associated with a an image in the On 2017/04/06 23:52:15, Ted C wrote: > wrap javadoc lines to 100 Done. https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmap.java:13: public enum TileTypes { PICTURE, CAMERA, GALLERY } On 2017/04/06 23:52:15, Ted C wrote: > We typically try to avoid using enums in java land. Any thought on using ints > and a corresponding @IntDef here? Done. https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmap.java:59: if (mLastModified < other.mLastModified) { On 2017/04/06 23:52:15, Ted C wrote: > could this be: > > return ApiCompatibilityUtils.compareLong(other.mLastModified, mLastModified); > > ? Done. https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmap.java:73: public boolean equals(Object other) { I think I don't need this equals. These objects will always be unique, and I don't have a need to ever compare them. I was just trying to suppress a FindBugs error, which is easier with a simple suppress command above. https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java (right): https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:74: protected void onFinishInflate() { On 2017/04/06 23:52:15, Ted C wrote: > nit, I'd put this right under the constructor as it is really the stuff that > completes the view Done. https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:93: super.onLongClick(this); On 2017/04/06 23:52:15, Ted C wrote: > Do you specifically need super. here? That would skip any overrides you do at > any point in the future. > > You'll want to make sure this doesn't do wonky things in accessibility (at some > point in the future). Mixing click and long click might confuse it. Done. https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:125: super.setSelectionDelegate(mSelectionDelegate); On 2017/04/06 23:52:15, Ted C wrote: > same comment where I don't think you need the super call here since you're not > already inside of setSelectionDelegate Done. https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapView.java:205: int bgColorId, fgColorId; On 2017/04/06 23:52:15, Ted C wrote: > one init per line plz :-) Done. https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2758313002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:32: private static final int KILOBYTE = 1024; On 2017/04/06 23:52:15, Ted C wrote: > unused Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Note: The try bot errors are not isolated to this patch... See: https://bugs.chromium.org/p/chromium/issues/detail?id=709459
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Woot! Bot errors have cleared up (as evident above). Would really appreciate if you find some time for this before you leave today, if at all possible... :)
baaah...I had a window open with an lgtm from hours and hours ago. Sigh. LGTM!!!
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org Link to the patchset: https://codereview.chromium.org/2758313002/#ps440001 (title: "Address Ted's comments (and sync to latest)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
No harm, no foul. Just happy to see this go in before EOD Sunday, so I can come in on Monday and hit the ground running. :) Thanks for the review.
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1491658911146960,
"parent_rev": "154a93d5c03f1a483b644c1bca4dea60dabb7dad", "commit_rev":
"67f22396e43ea69024f3f158bfcd5ec411042dee"}
Message was sent while issue was closed.
Description was changed from ========== Implement the new Photo picker, part two. This replaces the "Not implemented" Toast with a fully working dialog, except the images used are all placeholder gray tiles (instead of actual decoded images). BUG=656015 ========== to ========== Implement the new Photo picker, part two. This replaces the "Not implemented" Toast with a fully working dialog, except the images used are all placeholder gray tiles (instead of actual decoded images). BUG=656015 Review-Url: https://codereview.chromium.org/2758313002 Cr-Commit-Position: refs/heads/master@{#463133} Committed: https://chromium.googlesource.com/chromium/src/+/67f22396e43ea69024f3f158bfcd... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:440001) as https://chromium.googlesource.com/chromium/src/+/67f22396e43ea69024f3f158bfcd... |
