Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2670083002:
[Download Home] Displaying offline page bundle per day (Closed)
|
Created:
3 years, 10 months ago by shaktisahu Modified:
3 years, 9 months ago CC:
chromium-reviews, asanka, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Download Home] Displaying offline page bundle per day
This CL adds suggested offline pages to the DownloadHistoryAdapter.
The suggested pages are grouped into a bundle for each day and placed
at the bottom of each day's download history. The pages can be shown or
hidden through tapping an up/down arrow on the subsection header.
Key points of this CL:
1- To keep DateDividedAdapter unaware of offline suggestions,
DateDividedAdapter.ItemGroup was subclassed into a separate
DownloadItemGroup class which contains the special sorting function for
this purpose. Based on whether the offline pages section is expanded or
not, the offline items are filtered out before adding to the adapter.
2- For displaying the section header for the suggested offline pages,
a TimedItem is added to the adapter which is passed the required
information about the group it is associated to. OfflineGroupHeaderView
and OfflineGroupHeaderViewHolder classes represent the view portion
for the header. The section header is not selectable at the moment
which would be addressed in a future CL.
BUG=689801
Review-Url: https://codereview.chromium.org/2670083002
Cr-Commit-Position: refs/heads/master@{#452361}
Committed: https://chromium.googlesource.com/chromium/src/+/1b901526d84b86839e58b340a117eef6b40a7706
Patch Set 1 #
Total comments: 35
Patch Set 2 : Dan's initial comments #Patch Set 3 : Some fix #
Total comments: 22
Patch Set 4 : Some comments from Theresa #Patch Set 5 : rebase and style changes #
Total comments: 4
Patch Set 6 : Theresa's comments #
Total comments: 47
Patch Set 7 : addressed some comments #
Total comments: 2
Patch Set 8 : Modified removeItem #
Total comments: 2
Patch Set 9 : rebase #Patch Set 10 : Filter out pages before adding to DateDividedAdapter #
Total comments: 53
Patch Set 11 : Dan's comments on new approach #
Total comments: 6
Patch Set 12 : comments #
Total comments: 6
Patch Set 13 : comments #
Total comments: 4
Patch Set 14 : nits #
Total comments: 6
Patch Set 15 : nits #Patch Set 16 : rebase #Patch Set 17 : FindBugs fix #Messages
Total messages: 67 (31 generated)
shaktisahu@chromium.org changed reviewers: + dfalcantara@chromium.org
dfalcantara@ - I haven't cleaned up this code yet. But can you give me some feedback on this design?
Haven't gone too deeply at all, but you're going to need Theresa's eyes on this, too. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:73: public boolean isOfflinePage() { This class should really have no idea what an offline page is.
https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:73: public boolean isOfflinePage() { On 2017/02/02 20:49:55, dfalcantara (load balance plz) wrote: > This class should really have no idea what an offline page is. Okay. In that case, I can move this function to DownloadItemGroup since that's the only place where it is used.
dfalcantara@chromium.org changed reviewers: + twellington@chromium.org
Still going through this. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/offline_download_header.xml (right): https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/offline_download_header.xml:56: android:layout_toStartOf="@+id/filesize_view" move this to a layout_toEndOf rule on filesize_view so that everything that comes later in the file depends on the things before it https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/offline_download_header.xml:78: <ImageView This view is just dangling here; it can end up overlapping the view that is below it. 1) It should be placed above the description textview. 2) It needs to be layout_alignBottom with the page_count_text. 3) filesize_view should be layout_below expand_icon https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/offline_download_header.xml:91: </LinearLayout> This really needs a screenshot to be reviewed properly. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryItemWrapper.java (right): https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryItemWrapper.java:561: public boolean isOfflinePage() { If you absolutely need this function, then put it in the DownloadHistoryItemWrapper, not in TimedItem. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java (right): https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:13: public class DownloadItemGroup extends DateDividedAdapter.ItemGroup { This is confusing because the user can download offline pages manually that aren't bucketed into your new group. You need to be explicitly clear that these are for automatically downloaded ones throughout the code. Consider creating an inner class that encapsulates all of your new logic, then making the DownloadItemGroup own an instance. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:14: private int mNumOfflinePages; mNumSuggestedOfflinePages or mNumAutomaticallyDownloadedOfflinePages https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:15: private boolean mOfflinePagesExpanded; mIsSuggestedOfflinePagesSectionExpanded / isSuggestedOfflinePagesSectionExpanded / setIsSuggestedOfflinePagesSectionExpanded https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:17: // The header for the group containing offline pages. 1) Don't say there's a "group" inside a "group". 2) Use single line javadoc notation instead of this for individual fields. /** The header representing the Offline Pages that are automatically downloaded. */ https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:25: mOfflineHeader = new TimedItem() { This should only be created when it's actually necessary. The likelihood of there being a bucket every single day is highly unlikely and wastes memory. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:38: mStableId = (mStableId << 32) + ~(getTimestamp() & 0x0FFFFFFFF); This should be more consistent with the stable IDs generated for everything else. Date headers get a header based on their date: (day of year << 16) + year Download items get a header based on their timestamp and their ID: (hash of download guid << 32) + (lower 32 bits of download timestamp) Given that this header is meant to be in the list of download items, maybe this would make more sense: (0xffffffff00000000) + (lower 32 bits of download timestamp) https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:114: public boolean isOfflineHeader(int index) { This is used only here. Make it private. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:32: if (!mGroup.offlinePageExpanded()) { Positive condition on top. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:50: mDescription.setText(R.string.download_manager_offline_header_description); If this text never changes, put it in the XML itself. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:55: mAdapter = adapter; This should call updateTitleText directly. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (left): https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:169: */ Why'd you remove this? https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:241: public static final int TYPE_OFFLINE_HEADER = 2; automatic offline header or something https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:513: // Somewhat inefficient if number of groups is large. Can't you recompute the size based on the item/group you just removed? Beats having to iterate through the entire data structure every time something is removed.
https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/offline_download_header.xml (right): https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/offline_download_header.xml:17: android:layout_marginStart="16dp" This should be extracted as a style in styles.xml and shared with download_item_view.xml so that it is easier to keep styles in sync. Same for the other items in this layout that are the same as download_item_view.xml https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:211: return R.layout.offline_download_header; Since this is only called in one place I think it'd be fine to inline it below in #onCreateViewHolder() https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java (right): https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:35: public int getSuggestedOfflinePageCount() { nit: public methods need JavaDocs https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:85: index += 1; A comment here explaining why we skip a position for the first suggested item would be helpful. https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:88: if (!isSuggestedOfflinePagesSectionExpanded()) continue; Should we break if the suggested offline page section isn't expanded instead of checking for each of the remaining items? https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:98: if (mNumSuggestedOfflinePages > 0) { Do we add everything to the adapter whether or not the section is expanded? Can you walk me through the logic of how suggested offline pages are hidden when the section is not expanded? https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:17: /** A header that presents users to view or hide the suggested offline pages. */ nit: ... the option to view or... https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:171: protected boolean isListHeaderOrFooter() { Where is this called? https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:239: public static final int TYPE_SUGGESTED_OFFLINE_PAGES_HEADER = 2; DateDividedAdapter shouldn't know about offline pages. Also, would it make sense to introduce a "TYPE_SUBSECTION" concept so that the work in this CL can be more easily leveraged for possible future expandable subsections? https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:434: return TYPE_DATE; If we're moving the responsibility of knowing the view types for items into ItemGroup, then this should probably move too.
https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:50: It looks like you need to sync and rebase this CL. I landed a change to DateDividedAdapter earlier this week that introduced variables for mIsFirstInGroup and mIsLastInGroup to TimedItem. The booleans are being used to select the background drawable for history items and it will soon be used for downloads as well. Since this CL changes it so that not all items in a group are visible, the booleans should be updated to something like mIsFirstVisibleInGroup and mIsLastVisibleInGroup, so that the last visible item in each group gets the background drawable with a shadow on the bottom even when there are hidden items in the group.
https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/offline_download_header.xml (right): https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/offline_download_header.xml:56: android:layout_toStartOf="@+id/filesize_view" On 2017/02/02 23:59:47, dfalcantara (load balance plz) wrote: > move this to a layout_toEndOf rule on filesize_view so that everything that > comes later in the file depends on the things before it I followed the same code as download_item_view.xml. I guess the description could be longer than the available space overlap with the filesize_view. That's why filesize_view needs to be drawn first ? https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/offline_download_header.xml:78: <ImageView On 2017/02/02 23:59:47, dfalcantara (load balance plz) wrote: > This view is just dangling here; it can end up overlapping the view that is > below it. > > 1) It should be placed above the description textview. > 2) It needs to be layout_alignBottom with the page_count_text. > 3) filesize_view should be layout_below expand_icon Done. 1- Done 2- It has alignParentTop instead. 3- Not necessarily, since filesize_view also has alignParentBottom. Moreover the icon is not directly above the file size text. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/offline_download_header.xml:91: </LinearLayout> On 2017/02/02 23:59:47, dfalcantara (load balance plz) wrote: > This really needs a screenshot to be reviewed properly. The screenshot should be same as https://docs.google.com/presentation/d/13k5aqrflvb6GohGd_xjM2JeSZtF3A665HgUn7... https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryItemWrapper.java (right): https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryItemWrapper.java:561: public boolean isOfflinePage() { On 2017/02/02 23:59:47, dfalcantara (load balance plz) wrote: > If you absolutely need this function, then put it in the > DownloadHistoryItemWrapper, not in TimedItem. Done. Removed. Added check isSuggested() to OfflinePageItemWrapper and OfflinePageItem https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java (right): https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:13: public class DownloadItemGroup extends DateDividedAdapter.ItemGroup { On 2017/02/02 23:59:48, dfalcantara (load balance plz) wrote: > This is confusing because the user can download offline pages manually that > aren't bucketed into your new group. You need to be explicitly clear that these > are for automatically downloaded ones throughout the code. Consider creating an > inner class that encapsulates all of your new logic, then making the > DownloadItemGroup own an instance. Okay. I renamed everything with *OfflinePage* here to *SuggestedOfflinePage*. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:14: private int mNumOfflinePages; On 2017/02/02 23:59:48, dfalcantara (load balance plz) wrote: > mNumSuggestedOfflinePages or mNumAutomaticallyDownloadedOfflinePages Done. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:15: private boolean mOfflinePagesExpanded; On 2017/02/02 23:59:48, dfalcantara (load balance plz) wrote: > mIsSuggestedOfflinePagesSectionExpanded / isSuggestedOfflinePagesSectionExpanded > / setIsSuggestedOfflinePagesSectionExpanded Done. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:17: // The header for the group containing offline pages. On 2017/02/02 23:59:48, dfalcantara (load balance plz) wrote: > 1) Don't say there's a "group" inside a "group". > 2) Use single line javadoc notation instead of this for individual fields. > > /** The header representing the Offline Pages that are automatically downloaded. > */ Done. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:25: mOfflineHeader = new TimedItem() { On 2017/02/02 23:59:47, dfalcantara (load balance plz) wrote: > This should only be created when it's actually necessary. The likelihood of > there being a bucket every single day is highly unlikely and wastes memory. Done. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:38: mStableId = (mStableId << 32) + ~(getTimestamp() & 0x0FFFFFFFF); On 2017/02/02 23:59:47, dfalcantara (load balance plz) wrote: > This should be more consistent with the stable IDs generated for everything > else. > > Date headers get a header based on their date: > (day of year << 16) + year > > Download items get a header based on their timestamp and their ID: > (hash of download guid << 32) + (lower 32 bits of download timestamp) > > Given that this header is meant to be in the list of download items, maybe this > would > make more sense: > (0xffffffff00000000) + (lower 32 bits of download timestamp) Done. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:114: public boolean isOfflineHeader(int index) { On 2017/02/02 23:59:47, dfalcantara (load balance plz) wrote: > This is used only here. Make it private. Done. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:32: if (!mGroup.offlinePageExpanded()) { On 2017/02/02 23:59:48, dfalcantara (load balance plz) wrote: > Positive condition on top. Done. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:50: mDescription.setText(R.string.download_manager_offline_header_description); On 2017/02/02 23:59:48, dfalcantara (load balance plz) wrote: > If this text never changes, put it in the XML itself. Done. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:55: mAdapter = adapter; On 2017/02/02 23:59:48, dfalcantara (load balance plz) wrote: > This should call updateTitleText directly. Done. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (left): https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:169: */ On 2017/02/02 23:59:48, dfalcantara (load balance plz) wrote: > Why'd you remove this? Done. Was accidentally removed. https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:241: public static final int TYPE_OFFLINE_HEADER = 2; On 2017/02/02 23:59:48, dfalcantara (load balance plz) wrote: > automatic offline header or something Done. TYPE_SUGGESTED_OFFLINE_PAGES_HEADER https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/offline_download_header.xml (right): https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/offline_download_header.xml:17: android:layout_marginStart="16dp" On 2017/02/03 19:53:06, Theresa wrote: > This should be extracted as a style in styles.xml and shared with > download_item_view.xml so that it is easier to keep styles in sync. > > Same for the other items in this layout that are the same as > download_item_view.xml Done. https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:211: return R.layout.offline_download_header; On 2017/02/03 19:53:06, Theresa wrote: > Since this is only called in one place I think it'd be fine to inline it below > in #onCreateViewHolder() Done. https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java (right): https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:35: public int getSuggestedOfflinePageCount() { On 2017/02/03 19:53:06, Theresa wrote: > nit: public methods need JavaDocs Done. https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:85: index += 1; On 2017/02/03 19:53:06, Theresa wrote: > A comment here explaining why we skip a position for the first suggested item > would be helpful. Done. https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:88: if (!isSuggestedOfflinePagesSectionExpanded()) continue; On 2017/02/03 19:53:06, Theresa wrote: > Should we break if the suggested offline page section isn't expanded instead of > checking for each of the remaining items? Done. Yes. https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:98: if (mNumSuggestedOfflinePages > 0) { On 2017/02/03 19:53:06, Theresa wrote: > Do we add everything to the adapter whether or not the section is expanded? Can > you walk me through the logic of how suggested offline pages are hidden when the > section is not expanded? We only add the visible items to the adapter. The adapter invokes getItemAt(position) and we supply that item by finding the appropriate item from mItems. https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:17: /** A header that presents users to view or hide the suggested offline pages. */ On 2017/02/03 19:53:06, Theresa wrote: > nit: ... the option to view or... Done. https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:50: On 2017/02/03 22:36:45, Theresa wrote: > It looks like you need to sync and rebase this CL. I landed a change to > DateDividedAdapter earlier this week that introduced variables for > mIsFirstInGroup and mIsLastInGroup to TimedItem. The booleans are being used to > select the background drawable for history items and it will soon be used for > downloads as well. > > Since this CL changes it so that not all items in a group are visible, the > booleans should be updated to something like mIsFirstVisibleInGroup and > mIsLastVisibleInGroup, so that the last visible item in each group gets the > background drawable with a shadow on the bottom even when there are hidden items > in the group. Done. https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:171: protected boolean isListHeaderOrFooter() { On 2017/02/03 19:53:06, Theresa wrote: > Where is this called? Done. Removed. Thanks! https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:239: public static final int TYPE_SUGGESTED_OFFLINE_PAGES_HEADER = 2; On 2017/02/03 19:53:06, Theresa wrote: > DateDividedAdapter shouldn't know about offline pages. > > > Also, would it make sense to introduce a "TYPE_SUBSECTION" concept so that the > work in this CL can be more easily leveraged for possible future expandable > subsections? Yes, I changed this to TYPE_SUBSECTION which can be used for the subclassed adapters. DownloadHistoryAdapter will use this for offline page header. https://codereview.chromium.org/2670083002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:434: return TYPE_DATE; On 2017/02/03 19:53:06, Theresa wrote: > If we're moving the responsibility of knowing the view types for items into > ItemGroup, then this should probably move too. Done.
Thanks for all of the updates so far. I'll do another pass tomorrow morning. https://codereview.chromium.org/2670083002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java (right): https://codereview.chromium.org/2670083002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:157: if (index == 0) return null; The history view is going to have a list header view soon (when the space display moves out of the drawer and in-line, I can send you mocks if you're interested), so it would be ideal if this method kept more of the logic from its super class version (specifically the parts that check if the group is a list header or list footer). https://codereview.chromium.org/2670083002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:166: && index == mItems.size() - mNumSuggestedOfflinePages + 1) { Could this be extracted to something like getOfflinePagesSectionHeaderPosition() and shared with isSuggestedOfflinePagesSectionHeader()?
https://codereview.chromium.org/2670083002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java (right): https://codereview.chromium.org/2670083002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:157: if (index == 0) return null; On 2017/02/07 00:09:05, Theresa wrote: > The history view is going to have a list header view soon (when the space > display moves out of the drawer and in-line, I can send you mocks if you're > interested), so it would be ideal if this method kept more of the logic from its > super class version (specifically the parts that check if the group is a list > header or list footer). Done. https://codereview.chromium.org/2670083002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:166: && index == mItems.size() - mNumSuggestedOfflinePages + 1) { On 2017/02/07 00:09:05, Theresa wrote: > Could this be extracted to something like getOfflinePagesSectionHeaderPosition() > and shared with isSuggestedOfflinePagesSectionHeader()? Done.
Description was changed from ========== [Download Home] Displaying offline page bundle per day This is an exploratory CL to bundle the offline pages per date. The end UI looks like https://docs.google.com/presentation/d/13k5aqrflvb6GohGd_xjM2JeSZtF3A665HgUn7... The offline page bundle is placed at the bottom of each day's section Key points of this CL: 1- To keep DateDividedAdapter unaware of offline pages, I have subclassed DateDividedAdapter.ItemGroup into a separate DownloadItemGroup class. All the logic related to offline pages is contained in DownloadItemGroup and DownloadHistoryAdapter (subclass of DateDividedAdapter) 2- For displaying the group header for offline pages, I have added OfflineGroupHeaderView and OfflineGroupHeaderViewHolder classes. OfflineGroupHeader is similar to DownloadItemView except that it need not know about download progress, pause/resume/cancel, thumbnail, filter type etc. Since this view doesn't extend SelectableItemView, the group cannot be selected/highlighted. Not sure if user needs to be able to select and delete the whole group. If user feedback is needed, we can explore more. BUG= ========== to ========== [Download Home] Displaying offline page bundle per day This CL adds suggested offline pages to the DownloadHistoryAdapter. The suggested pages are grouped into a bundle for each day and placed at the bottom of each day's download history. The pages can be shown or hidden through tapping an up/down arrow on the subsection header. UX design: https://docs.google.com/presentation/d/13k5aqrflvb6GohGd_xjM2JeSZtF3A665HgUn7... Key points of this CL: 1- To keep DateDividedAdapter unaware of offline suggestions, DateDividedAdapter.ItemGroup was subclassed into a separate DownloadItemGroup class. All the logic related to offline pages is contained in DownloadItemGroup and DownloadHistoryAdapter. 2- The section header for the suggested offline pages is displayed with the help of OfflineGroupHeaderView, OfflineGroupHeaderViewHolder classes. OfflineGroupHeader is similar to DownloadItemView except that it is unaware of download related functionalities like download progress, pause/resume/cancel, thumbnail, filter type etc. The section header is not selectable at the moment which would be addressed in a future CL. BUG=689801 ==========
https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/re... File chrome/android/java/res/layout/offline_download_header.xml (right): https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/offline_download_header.xml:30: style="@style/DownloadFileNameStyle" /> nit: maybe this style should be DownloadTitleStyle or something like that since it's used for the page count text and file name. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/offline_download_header.xml:45: style="@style/DownloadHostNameStyle" nit: maybe this style should be called DownloadDescriptionStyle or something like that since it's used for host name and this description line. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:226: return new OfflineGroupHeaderViewHolder( For consistency with how we create headers, DateDividedAdapter#onCreateViewHolder() should include something liek this: if (viewType == TYPE_SUBSECTION_HEADER) { return createSubsectionHeader(); } and this inflate should be moved to an overridden createSubsectionHeader(). https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:241: offlineHeader.bindGroupAndAdapter(group, this); The adapter can be set when the OfflineGroupHeaderViewHolder is inflated since that won't change. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java (right): https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:165: return mItems.get(index - 1); nit: would it make sense for this to be return super.getItemInternal(); ? https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:37: } The expand icon should have a content description for TalkBack to read out e.g. "Tap to expand" or "Tap to collapse" (may be good to check with UX/accessibility folks on this). https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:218: return mDate; For consistency, we should either have a getter that returns mItems and leave that as private or make mDate protected and remove this getter. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:239: protected TimedItem getItemAtInner(int index) { nit: getItemInternal() is more consistent with naming for protected/private methods associated with a public method. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:370: protected ItemGroup createGroup(long timeStamp) { nit: s/timeStamp/timestamp https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:490: return group.getItemViewType(pair.second); nit: This could be condensed to one line: pair.first.getItemViewType(pair.second); https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:561: computeItemCount(); If there are a lot of item groups, this is less efficient than decrementing mSize.
https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:264: public void expandSuggestedPagesHeader(DownloadItemGroup group) { 1) javadocs for public methods 2) This is basically the same as collapse, with a true instead of a false. Can you consolidate the two? public void setSuggestedPagesHeaderState(DownloadItemGroup group, boolean state) https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java (right): https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:24: mNumSuggestedOfflinePages = 0; ints are automatically initialized to 0. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:25: mIsSuggestedOfflinePagesSectionExpanded = false; booleans are automatically initialized to false. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:42: public int getSuggestedOfflinePageCount() { Make this accessor match the variable name. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:60: mStableId = 0xFFFFFFFF00000000L + getTimestamp() & 0x0FFFFFFFF; Add parentheses around the & to make the order of ops more obvious https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:93: boolean firstSuggestedItem = true; use "isFirstSuggestedItem"; "firstSuggestedItem" makes it sound like it's the actual object https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:101: if (!isSuggestedOfflinePagesSectionExpanded()) break; This check only needs to be done for the first suggested item, doesn't it? You should be able to combine this as if (isSuggestedOfflinePage(item) && isFirstSuggestedItem) https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:126: // on whether the suggested items section is expanded or collapsed. Have you considered just not adding the suggested offline pages to the DateDividedAdapter until the group is expanded? That would likely remove a bunch of this special casing. We do the same thing when we filter items, where DownloadHistoryAdapter#filter calls DateDividedAdapter#loadItems. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:197: protected void setPositionForFirstAndLastInGroup() { This isn't setting positions; call it "identifyFirstAndLastItems" or something. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:372: return group; return new ItemGroup(timestamp); https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:561: computeItemCount(); On 2017/02/10 00:55:12, Theresa wrote: > If there are a lot of item groups, this is less efficient than decrementing > mSize. +1 again. I asked about this in Patch Set 1.
PTAL https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/re... File chrome/android/java/res/layout/offline_download_header.xml (right): https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/offline_download_header.xml:30: style="@style/DownloadFileNameStyle" /> On 2017/02/10 00:55:12, Theresa wrote: > nit: maybe this style should be DownloadTitleStyle or something like that since > it's used for the page count text and file name. Done. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/offline_download_header.xml:45: style="@style/DownloadHostNameStyle" On 2017/02/10 00:55:12, Theresa wrote: > nit: maybe this style should be called DownloadDescriptionStyle or something > like that since it's used for host name and this description line. Done. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:226: return new OfflineGroupHeaderViewHolder( On 2017/02/10 00:55:12, Theresa wrote: > For consistency with how we create headers, > DateDividedAdapter#onCreateViewHolder() should include something liek this: > > if (viewType == TYPE_SUBSECTION_HEADER) { > return createSubsectionHeader(); > } > > and this inflate should be moved to an overridden createSubsectionHeader(). Good idea! Thanks. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:241: offlineHeader.bindGroupAndAdapter(group, this); On 2017/02/10 00:55:12, Theresa wrote: > The adapter can be set when the OfflineGroupHeaderViewHolder is inflated since > that won't change. Done. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:264: public void expandSuggestedPagesHeader(DownloadItemGroup group) { On 2017/02/10 02:31:44, dfalcantara (load balance plz) wrote: > 1) javadocs for public methods > 2) This is basically the same as collapse, with a true instead of a false. Can > you consolidate the two? > > public void setSuggestedPagesHeaderState(DownloadItemGroup group, boolean state) Done. Thanks! https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java (right): https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:24: mNumSuggestedOfflinePages = 0; On 2017/02/10 02:31:44, dfalcantara (load balance plz) wrote: > ints are automatically initialized to 0. Done. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:25: mIsSuggestedOfflinePagesSectionExpanded = false; On 2017/02/10 02:31:45, dfalcantara (load balance plz) wrote: > booleans are automatically initialized to false. Done. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:42: public int getSuggestedOfflinePageCount() { On 2017/02/10 02:31:44, dfalcantara (load balance plz) wrote: > Make this accessor match the variable name. Done. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:60: mStableId = 0xFFFFFFFF00000000L + getTimestamp() & 0x0FFFFFFFF; On 2017/02/10 02:31:44, dfalcantara (load balance plz) wrote: > Add parentheses around the & to make the order of ops more obvious Done. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:93: boolean firstSuggestedItem = true; On 2017/02/10 02:31:44, dfalcantara (load balance plz) wrote: > use "isFirstSuggestedItem"; "firstSuggestedItem" makes it sound like it's the > actual object Done. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:101: if (!isSuggestedOfflinePagesSectionExpanded()) break; On 2017/02/10 02:31:44, dfalcantara (load balance plz) wrote: > This check only needs to be done for the first suggested item, doesn't it? You > should be able to combine this as > > if (isSuggestedOfflinePage(item) && isFirstSuggestedItem) Done. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:126: // on whether the suggested items section is expanded or collapsed. On 2017/02/10 02:31:44, dfalcantara (load balance plz) wrote: > Have you considered just not adding the suggested offline pages to the > DateDividedAdapter until the group is expanded? That would likely remove a > bunch of this special casing. We do the same thing when we filter items, where > DownloadHistoryAdapter#filter calls DateDividedAdapter#loadItems. That would still require adding the suggested page section header to the adapter. The header would need to be treated in a different way from the other items as it is a different view and it needs to keep the page count too. I am not sure if it will make it less complex. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:165: return mItems.get(index - 1); On 2017/02/10 00:55:12, Theresa wrote: > nit: would it make sense for this to be return super.getItemInternal(); ? hmm... I think, it seems more readable to keep this here with the above comments. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:197: protected void setPositionForFirstAndLastInGroup() { On 2017/02/10 02:31:45, dfalcantara (load balance plz) wrote: > This isn't setting positions; call it "identifyFirstAndLastItems" or something. Done. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:218: return mDate; On 2017/02/10 00:55:12, Theresa wrote: > For consistency, we should either have a getter that returns mItems and leave > that as private or make mDate protected and remove this getter. Done. I would make mDate as protected and remove this getter. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:239: protected TimedItem getItemAtInner(int index) { On 2017/02/10 00:55:12, Theresa wrote: > nit: getItemInternal() is more consistent with naming for protected/private > methods associated with a public method. Done. Renamed it to getItemAtInternal https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:370: protected ItemGroup createGroup(long timeStamp) { On 2017/02/10 00:55:12, Theresa wrote: > nit: s/timeStamp/timestamp Done. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:372: return group; On 2017/02/10 02:31:45, dfalcantara (load balance plz) wrote: > return new ItemGroup(timestamp); Done. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:490: return group.getItemViewType(pair.second); On 2017/02/10 00:55:12, Theresa wrote: > nit: This could be condensed to one line: > pair.first.getItemViewType(pair.second); hmm.. I think this is more readable. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:561: computeItemCount(); On 2017/02/10 02:31:45, dfalcantara (load balance plz) wrote: > On 2017/02/10 00:55:12, Theresa wrote: > > If there are a lot of item groups, this is less efficient than decrementing > > mSize. > > +1 again. I asked about this in Patch Set 1. I thought this makes it cleaner (also used when adding items in loadItems). Actually, on the next line when we do setGroupPositions, we iterate through the group anyway and set each element to its correct position after the delete, which is a bigger operation than this one (O(itemcount)). So I think this should be okay since this is O(groups) operation. Otherwise, I would have to find out how many rows need to be deleted (it can be 1 or 2 or 3). Something like : int rowsToBeDeleted = group.getNumRowsToBeDeletedForItem(item); group.removeItem(item); mSize -= rowsToBeDeleted; And then implement getNumRowsToBeDeletedForItem for ItemGroup and DownloadItemGroup subclass. But I feel the current code is cleaner than that.
You've checked that selecting a non-offline page filter (e.g. Images) on the sidebar hides all the automatically suggested items, right? https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java (right): https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:126: // on whether the suggested items section is expanded or collapsed. On 2017/02/10 21:30:24, shaktisahu wrote: > On 2017/02/10 02:31:44, dfalcantara (load balance plz) wrote: > > Have you considered just not adding the suggested offline pages to the > > DateDividedAdapter until the group is expanded? That would likely remove a > > bunch of this special casing. We do the same thing when we filter items, > where > > DownloadHistoryAdapter#filter calls DateDividedAdapter#loadItems. > > That would still require adding the suggested page section header to the > adapter. > The header would need to be treated in a different way from the other items as > it is a different view and it needs to keep the page count too. > I am not sure if it will make it less complex. It'd be consistent with how we handle filters, though. The adapter already stores a bunch of items that are not displayed on the list until they're needed. If you add items only when the groups are expanded or collapsed, then the size() value will always be accurate, you wouldn't have to special case setPositionForItems, you wouldn't have to track how many suggested offline pages are in this group here, and getItemAtInternal wouldn't have to exist because the list would be correctly numbered. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:561: computeItemCount(); On 2017/02/10 21:30:25, shaktisahu wrote: > On 2017/02/10 02:31:45, dfalcantara (load balance plz) wrote: > > On 2017/02/10 00:55:12, Theresa wrote: > > > If there are a lot of item groups, this is less efficient than decrementing > > > mSize. > > > > +1 again. I asked about this in Patch Set 1. > > I thought this makes it cleaner (also used when adding items in loadItems). > > Actually, on the next line when we do setGroupPositions, we iterate through the > group anyway and set each element to its correct position after the delete, > which is a bigger operation than this one (O(itemcount)). So I think this should > be okay since this is O(groups) operation. > > Otherwise, I would have to find out how many rows need to be deleted (it can be > 1 or 2 or 3). > > Something like : > > int rowsToBeDeleted = group.getNumRowsToBeDeletedForItem(item); > group.removeItem(item); > mSize -= rowsToBeDeleted; > > And then implement getNumRowsToBeDeletedForItem for ItemGroup and > DownloadItemGroup subclass. But I feel the current code is cleaner than that. > Cleanliness < efficiency sometimes, and depending on how many downloads a user has, that could be a big difference on the UI thread–additional lag contributes to ANRs. How complicated is the logic for determining how many rows need to be deleted? https://codereview.chromium.org/2670083002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2670083002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:34: currentState ? R.drawable.ic_expanded : R.drawable.ic_collapsed); Did you missed Theresa's comment about content descriptions?
PTAL https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:37: } On 2017/02/10 00:55:12, Theresa wrote: > The expand icon should have a content description for TalkBack to read out e.g. > "Tap to expand" or "Tap to collapse" (may be good to check with UX/accessibility > folks on this). Done. https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:561: computeItemCount(); On 2017/02/10 22:49:52, dfalcantara (load balance plz) wrote: > On 2017/02/10 21:30:25, shaktisahu wrote: > > On 2017/02/10 02:31:45, dfalcantara (load balance plz) wrote: > > > On 2017/02/10 00:55:12, Theresa wrote: > > > > If there are a lot of item groups, this is less efficient than > decrementing > > > > mSize. > > > > > > +1 again. I asked about this in Patch Set 1. > > > > I thought this makes it cleaner (also used when adding items in loadItems). > > > > Actually, on the next line when we do setGroupPositions, we iterate through > the > > group anyway and set each element to its correct position after the delete, > > which is a bigger operation than this one (O(itemcount)). So I think this > should > > be okay since this is O(groups) operation. > > > > Otherwise, I would have to find out how many rows need to be deleted (it can > be > > 1 or 2 or 3). > > > > Something like : > > > > int rowsToBeDeleted = group.getNumRowsToBeDeletedForItem(item); > > group.removeItem(item); > > mSize -= rowsToBeDeleted; > > > > And then implement getNumRowsToBeDeletedForItem for ItemGroup and > > DownloadItemGroup subclass. But I feel the current code is cleaner than that. > > > > Cleanliness < efficiency sometimes, and depending on how many downloads a user > has, that could be a big difference on the UI thread–additional lag contributes > to ANRs. How complicated is the logic for determining how many rows need to be > deleted? Done. https://codereview.chromium.org/2670083002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2670083002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:34: currentState ? R.drawable.ic_expanded : R.drawable.ic_collapsed); On 2017/02/10 22:49:52, dfalcantara (load balance plz) wrote: > Did you missed Theresa's comment about content descriptions? Done. Thanks.
https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java (right): https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:126: // on whether the suggested items section is expanded or collapsed. On 2017/02/10 22:49:52, dfalcantara (load balance plz) wrote: > On 2017/02/10 21:30:24, shaktisahu wrote: > > On 2017/02/10 02:31:44, dfalcantara (load balance plz) wrote: > > > Have you considered just not adding the suggested offline pages to the > > > DateDividedAdapter until the group is expanded? That would likely remove a > > > bunch of this special casing. We do the same thing when we filter items, > > where > > > DownloadHistoryAdapter#filter calls DateDividedAdapter#loadItems. > > > > That would still require adding the suggested page section header to the > > adapter. > > The header would need to be treated in a different way from the other items as > > it is a different view and it needs to keep the page count too. > > I am not sure if it will make it less complex. > > It'd be consistent with how we handle filters, though. The adapter already > stores a bunch of items that are not displayed on the list until they're needed. > If you add items only when the groups are expanded or collapsed, then the > size() value will always be accurate, you wouldn't have to special case > setPositionForItems, you wouldn't have to track how many suggested offline pages > are in this group here, and getItemAtInternal wouldn't have to exist because the > list would be correctly numbered. +1 -- I'd also be interested in seeing if this can be done with filtering. We may be able to hook into DownloadHistoryItemWrapper#isVisibleToUser() and || in whether the offline page header associated with the suggested offline page is expanded or not. There may be some pitfalls here I'm not considering, so it'd probably be good to start with a light-weight (~1 hour) investigation of the feasibility. https://codereview.chromium.org/2670083002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java (right): https://codereview.chromium.org/2670083002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:80: mNumSuggestedOfflinePages--; This method should be responsible for removing the OfflineGroupHeaderView if there are no more suggested offline pages. https://codereview.chromium.org/2670083002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:279: public int getNumberOfRowsToBeRemovedForItem(TimedItem item) { Instead of introducing this method, can ItemGroup#removeItem() return the number of items that were removed?
On 2017/02/13 19:15:01, Theresa wrote: > https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java > (right): > > https://codereview.chromium.org/2670083002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:126: > // on whether the suggested items section is expanded or collapsed. > On 2017/02/10 22:49:52, dfalcantara (load balance plz) wrote: > > On 2017/02/10 21:30:24, shaktisahu wrote: > > > On 2017/02/10 02:31:44, dfalcantara (load balance plz) wrote: > > > > Have you considered just not adding the suggested offline pages to the > > > > DateDividedAdapter until the group is expanded? That would likely remove > a > > > > bunch of this special casing. We do the same thing when we filter items, > > > where > > > > DownloadHistoryAdapter#filter calls DateDividedAdapter#loadItems. > > > > > > That would still require adding the suggested page section header to the > > > adapter. > > > The header would need to be treated in a different way from the other items > as > > > it is a different view and it needs to keep the page count too. > > > I am not sure if it will make it less complex. > > > > It'd be consistent with how we handle filters, though. The adapter already > > stores a bunch of items that are not displayed on the list until they're > needed. > > If you add items only when the groups are expanded or collapsed, then the > > size() value will always be accurate, you wouldn't have to special case > > setPositionForItems, you wouldn't have to track how many suggested offline > pages > > are in this group here, and getItemAtInternal wouldn't have to exist because > the > > list would be correctly numbered. > > +1 -- I'd also be interested in seeing if this can be done with filtering. We > may be able to hook into DownloadHistoryItemWrapper#isVisibleToUser() and || in > whether the offline page header associated with the suggested offline page is > expanded or not. There may be some pitfalls here I'm not considering, so it'd > probably be good to start with a light-weight (~1 hour) investigation of the > feasibility. > > https://codereview.chromium.org/2670083002/diff/140001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java > (right): > > https://codereview.chromium.org/2670083002/diff/140001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:80: > mNumSuggestedOfflinePages--; > This method should be responsible for removing the OfflineGroupHeaderView if > there are no more suggested offline pages. > > https://codereview.chromium.org/2670083002/diff/140001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java > (right): > > https://codereview.chromium.org/2670083002/diff/140001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:279: > public int getNumberOfRowsToBeRemovedForItem(TimedItem item) { > Instead of introducing this method, can ItemGroup#removeItem() return the number > of items that were removed? I took an attempt into filtering out the suggested pages before adding it to the adapter. It actually looks simpler now. DateDividedAdapter is unchanged. DownloadItemGroup is only concerned with the sorting function. DownloadHistoryAdapter keeps track of which dates are expanded/collapsed. PTAL.
https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/offline_download_header.xml (right): https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/offline_download_header.xml:6: <!-- Represents the offline pages section header in the DownloadHistoryAdapterView. --> "suggested offline pages section header". be consistent about how you refer to this thing https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:64: /** Represents the subsection header of the suggested pages for a given date */ end with a period https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:69: private long mTotalFileSize; Everything here should be final. Stable ID can be calculated in the constructor instead of on the fly. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:112: private final Map<Date, Boolean> mSuggestedSectionExpanded = new HashMap<>(); Don't arbitrarily separate the BackendItemsImpls(). Move it above mParentComponent. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:256: .inflate(R.layout.offline_download_header, parent, false); Can you clarify what you're expecting here? inflate() returns the root of the view hierarchy, which is |parent| because you're passing it in. Is |parent| actually a OfflineGroupHeaderView? If it is, can this be written more clearly? https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:465: Map<Date, Integer> suggestedPageCount = new HashMap<>(); 1) Pull this whole new block out to some private function. Putting this inline makes the basic flow of the filter() function painful to follow. 2) "Count" makes it sound like this is a single integer. Call these maps "suggestedPageCountMaps" e.g. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:468: BackendItems offlinePageItems = new BackendItemsImpl(); filteredOfflinePageItems https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:471: // Add the suggested pages to the adapter only if the section is expanded for that date. This comment only applies to the inner if statement. What you're really doing is filtering all the offline pages with the for loop. Update the comments. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:485: // Add a header suggested pages for each date // Add a TimedItem for each subsection. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:501: if (count == null) count = 0; Avoid Integers if you don't need to use them. They're another annoying vector for null pointer exceptions. Try: int count = suggestedPageCount.containsKey(date) ? suggestedPageCount.get(date) : 0; https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:503: suggestedPageCount.put(date, count); suggestedPageCount.put(date, count + 1); https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:508: suggestedPageTotalSize.put(date, fileSize); Same comments as above. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:523: public void setSubsectionExpanded(Date date, boolean expanded) { Javadoc. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:524: mSuggestedSectionExpanded.put(date, expanded); Be consistent about what you're calling this thing. It flips between "suggested section" and "subsection" between fields and comments. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:612: private Date getDateWithoutTime(long timestamp) { /** Calculates the {@link Date} for midnight of the date represented by the timestamp. */ https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java (right): https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:41: private int order(TimedItem timedItem) { 1) This really needs some documentation about what the return value is and why it's set that way. 2) This really should be an if / else if / else block instead. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:33: boolean currentState = mAdapter.isSubsectionExpanded(mDate); Instead of negating it twice, just do it once: boolean newState = !mAdapter.isSubsectionExpanded(mDate); https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:56: * Finish this comment. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:62: public void updateView(Date date, boolean expanded, int pageCount, long fileSize) { Call this update()? https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:168: public static class ItemGroup { Make this protected again. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:306: protected SubsectionHeaderViewHolder createSubsectionHeader(ViewGroup parent) { @Nullable? https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:356: } Group your subsection header-related functions together instead of arbitrarily separating grouped TimedItem related functions. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:388: protected ItemGroup createGroup(long timestamp) { Javadoc for protected & public methods https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:516: public RecyclerView.ViewHolder onCreateViewHolder(ViewGroup parent, int viewType) { Make this function final again https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:533: public void onBindViewHolder(RecyclerView.ViewHolder holder, int position) { Make this function final again
(Definitely looks a lot cleaner, overall)
Description was changed from ========== [Download Home] Displaying offline page bundle per day This CL adds suggested offline pages to the DownloadHistoryAdapter. The suggested pages are grouped into a bundle for each day and placed at the bottom of each day's download history. The pages can be shown or hidden through tapping an up/down arrow on the subsection header. UX design: https://docs.google.com/presentation/d/13k5aqrflvb6GohGd_xjM2JeSZtF3A665HgUn7... Key points of this CL: 1- To keep DateDividedAdapter unaware of offline suggestions, DateDividedAdapter.ItemGroup was subclassed into a separate DownloadItemGroup class. All the logic related to offline pages is contained in DownloadItemGroup and DownloadHistoryAdapter. 2- The section header for the suggested offline pages is displayed with the help of OfflineGroupHeaderView, OfflineGroupHeaderViewHolder classes. OfflineGroupHeader is similar to DownloadItemView except that it is unaware of download related functionalities like download progress, pause/resume/cancel, thumbnail, filter type etc. The section header is not selectable at the moment which would be addressed in a future CL. BUG=689801 ========== to ========== [Download Home] Displaying offline page bundle per day This CL adds suggested offline pages to the DownloadHistoryAdapter. The suggested pages are grouped into a bundle for each day and placed at the bottom of each day's download history. The pages can be shown or hidden through tapping an up/down arrow on the subsection header. Key points of this CL: 1- To keep DateDividedAdapter unaware of offline suggestions, DateDividedAdapter.ItemGroup was subclassed into a separate DownloadItemGroup class which contains the special sorting function for this purpose. Based on whether the offline pages section is expanded or not, the offline items are filtered out before adding to the adapter. 2- For displaying the section header for the suggested offline pages, a TimedItem is added to the adapter which is passed the required information about the group it is associated to. OfflineGroupHeaderView and OfflineGroupHeaderViewHolder classes represent the view portion for the header. The section header is not selectable at the moment which would be addressed in a future CL. BUG=689801 ==========
https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/offline_download_header.xml (right): https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/offline_download_header.xml:6: <!-- Represents the offline pages section header in the DownloadHistoryAdapterView. --> On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > "suggested offline pages section header". be consistent about how you refer to > this thing Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:64: /** Represents the subsection header of the suggested pages for a given date */ On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > end with a period Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:69: private long mTotalFileSize; On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > Everything here should be final. Stable ID can be calculated in the constructor > instead of on the fly. Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:112: private final Map<Date, Boolean> mSuggestedSectionExpanded = new HashMap<>(); On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > Don't arbitrarily separate the BackendItemsImpls(). Move it above > mParentComponent. Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:256: .inflate(R.layout.offline_download_header, parent, false); On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > Can you clarify what you're expecting here? inflate() returns the root of the > view hierarchy, which is |parent| because you're passing it in. Is |parent| > actually a OfflineGroupHeaderView? If it is, can this be written more clearly? The third param attachToRoot is false here. The return value of inflate() is the root of the inflated XML file. So it wouldn't matter if we pass |parent| or null as the second param to inflate(). But as per documentation, if provided, it provides a set of LayoutParams for the root of the returned hierarchy. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:465: Map<Date, Integer> suggestedPageCount = new HashMap<>(); On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > 1) Pull this whole new block out to some private function. Putting this inline > makes the basic flow of the filter() function painful to follow. > > 2) "Count" makes it sound like this is a single integer. Call these maps > "suggestedPageCountMaps" e.g. Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:468: BackendItems offlinePageItems = new BackendItemsImpl(); On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > filteredOfflinePageItems Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:471: // Add the suggested pages to the adapter only if the section is expanded for that date. On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > This comment only applies to the inner if statement. What you're really doing > is filtering all the offline pages with the for loop. Update the comments. Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:485: // Add a header suggested pages for each date On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > // Add a TimedItem for each subsection. Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:501: if (count == null) count = 0; On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > Avoid Integers if you don't need to use them. They're another annoying vector > for null pointer exceptions. > > Try: > int count = suggestedPageCount.containsKey(date) ? suggestedPageCount.get(date) > : 0; Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:503: suggestedPageCount.put(date, count); On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > suggestedPageCount.put(date, count + 1); Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:508: suggestedPageTotalSize.put(date, fileSize); On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > Same comments as above. Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:523: public void setSubsectionExpanded(Date date, boolean expanded) { On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > Javadoc. Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:524: mSuggestedSectionExpanded.put(date, expanded); On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > Be consistent about what you're calling this thing. It flips between "suggested > section" and "subsection" between fields and comments. Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:612: private Date getDateWithoutTime(long timestamp) { On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > /** Calculates the {@link Date} for midnight of the date represented by the > timestamp. */ Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java (right): https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:41: private int order(TimedItem timedItem) { On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > 1) This really needs some documentation about what the return value is and why > it's set that way. > 2) This really should be an if / else if / else block instead. Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:33: boolean currentState = mAdapter.isSubsectionExpanded(mDate); On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > Instead of negating it twice, just do it once: > > boolean newState = !mAdapter.isSubsectionExpanded(mDate); Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:56: * On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > Finish this comment. Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:62: public void updateView(Date date, boolean expanded, int pageCount, long fileSize) { On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > Call this update()? Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:168: public static class ItemGroup { On 2017/02/15 23:58:10, dfalcantara (load balance plz) wrote: > Make this protected again. I am subclassing this from DownloadItemGroup which is outside this package. So it can't be protected. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:306: protected SubsectionHeaderViewHolder createSubsectionHeader(ViewGroup parent) { On 2017/02/15 23:58:10, dfalcantara (load balance plz) wrote: > @Nullable? Not sure if it adds any value. Each of the following functions should be @Nullable too in that case. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:356: } On 2017/02/15 23:58:10, dfalcantara (load balance plz) wrote: > Group your subsection header-related functions together instead of arbitrarily > separating grouped TimedItem related functions. Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:388: protected ItemGroup createGroup(long timestamp) { On 2017/02/15 23:58:10, dfalcantara (load balance plz) wrote: > Javadoc for protected & public methods Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:516: public RecyclerView.ViewHolder onCreateViewHolder(ViewGroup parent, int viewType) { On 2017/02/15 23:58:10, dfalcantara (load balance plz) wrote: > Make this function final again Done. https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:533: public void onBindViewHolder(RecyclerView.ViewHolder holder, int position) { On 2017/02/15 23:58:10, dfalcantara (load balance plz) wrote: > Make this function final again Done.
I like the new approach overall, thanks! https://codereview.chromium.org/2670083002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:499: filteredTimedItems.add(new SubsectionHeader( Instead of constructing the SubsectionHeader each time we filter, would it make sense to keep a map of Date -> SubsectionHeader? Right now every time a download item or offline page item is modified or the user selects a different filter or searches for anything, we'll end up reconstructing all of the SubsectionHeaders. It would be more efficient to update them when a suggested offline page is added/updated/removed. If we store the Date -> SubsectionHeader map as an instance variable, we could also use that in place of the mSubSectionExpanded map. https://codereview.chromium.org/2670083002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2670083002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:20: public class OfflineGroupHeaderView extends FrameLayout { Have you asked UX whether this should be selectable or not? I'm okay with a TODO to make it selectable and handle removing all contained items. Or if it shouldn't be selectable, a TODO to handle setting the background resource correctly (this is dependent on https://codereview.chromium.org/2703463002/).
https://codereview.chromium.org/2670083002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:499: filteredTimedItems.add(new SubsectionHeader( On 2017/02/16 17:48:17, Theresa wrote: > Instead of constructing the SubsectionHeader each time we filter, would it make > sense to keep a map of Date -> SubsectionHeader? > > Right now every time a download item or offline page item is modified or the > user selects a different filter or searches for anything, we'll end up > reconstructing all of the SubsectionHeaders. It would be more efficient to > update them when a suggested offline page is added/updated/removed. > > If we store the Date -> SubsectionHeader map as an instance variable, we could > also use that in place of the mSubSectionExpanded map. I think this is okay, since they aren't that many (a few days probably), and they really don't contain anything heavy. I wanted to keep a map of Boolean and didn't want to maintain SubSectionHeaders to be less error prone when updating states (add, remove, filter etc). But we could maintain these things either way I guess. https://codereview.chromium.org/2670083002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java (right): https://codereview.chromium.org/2670083002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:20: public class OfflineGroupHeaderView extends FrameLayout { On 2017/02/16 17:48:17, Theresa wrote: > Have you asked UX whether this should be selectable or not? > > I'm okay with a TODO to make it selectable and handle removing all contained > items. > > Or if it shouldn't be selectable, a TODO to handle setting the background > resource correctly (this is dependent on > https://codereview.chromium.org/2703463002/). Haven't got detailed feedback from UX yet, but yes, this should be selectable. I will add a TODO to make it selectable.
https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:256: .inflate(R.layout.offline_download_header, parent, false); On 2017/02/16 06:07:06, shaktisahu wrote: > On 2017/02/15 23:58:09, dfalcantara (load balance plz) wrote: > > Can you clarify what you're expecting here? inflate() returns the root of the > > view hierarchy, which is |parent| because you're passing it in. Is |parent| > > actually a OfflineGroupHeaderView? If it is, can this be written more > clearly? > > The third param attachToRoot is false here. The return value of inflate() is the > root of the inflated XML file. So it wouldn't matter if we pass |parent| or null > as the second param to inflate(). But as per documentation, if provided, it > provides a set of LayoutParams for the root of the returned hierarchy. Acknowledged. Misread the params list :/ https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:306: protected SubsectionHeaderViewHolder createSubsectionHeader(ViewGroup parent) { On 2017/02/16 06:07:07, shaktisahu wrote: > On 2017/02/15 23:58:10, dfalcantara (load balance plz) wrote: > > @Nullable? > > Not sure if it adds any value. Each of the following functions should be > @Nullable too in that case. Yeah, they really should be. https://codereview.chromium.org/2670083002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:465: List<TimedItem> filteredTimedItems = new ArrayList<>(); You're creating a ton of one-off Lists every time you filter, which will cause a bunch of garbage collection. filteredDownloadItems() was meant to be appended to as needed, not subsumed by another list. Can you change BackendItems#filter to take in a List<TimedItem> instead of a BackendItems, then pass the filteredDownloadItems into your filterOfflinePageItems() function? https://codereview.chromium.org/2670083002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:559: mSubSectionExpanded.put(date, expanded); Also be consistent about whether something is a "subSection" or a "subsection". Everything should be "subsection". https://codereview.chromium.org/2670083002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:395: * @param timestamp A time stamp from which the date is determined. timestamp.
https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:306: protected SubsectionHeaderViewHolder createSubsectionHeader(ViewGroup parent) { On 2017/02/17 19:21:16, dfalcantara (load balance plz) wrote: > On 2017/02/16 06:07:07, shaktisahu wrote: > > On 2017/02/15 23:58:10, dfalcantara (load balance plz) wrote: > > > @Nullable? > > > > Not sure if it adds any value. Each of the following functions should be > > @Nullable too in that case. > > Yeah, they really should be. Done. https://codereview.chromium.org/2670083002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:465: List<TimedItem> filteredTimedItems = new ArrayList<>(); On 2017/02/17 19:21:16, dfalcantara (load balance plz) wrote: > You're creating a ton of one-off Lists every time you filter, which will cause a > bunch of garbage collection. filteredDownloadItems() was meant to be appended > to as needed, not subsumed by another list. Can you change BackendItems#filter > to take in a List<TimedItem> instead of a BackendItems, then pass the > filteredDownloadItems into your filterOfflinePageItems() function? Done. https://codereview.chromium.org/2670083002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:559: mSubSectionExpanded.put(date, expanded); On 2017/02/17 19:21:16, dfalcantara (load balance plz) wrote: > Also be consistent about whether something is a "subSection" or a "subsection". > Everything should be "subsection". Done. https://codereview.chromium.org/2670083002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:395: * @param timestamp A time stamp from which the date is determined. On 2017/02/17 19:21:16, dfalcantara (load balance plz) wrote: > timestamp. Done.
This is getting really close https://codereview.chromium.org/2670083002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:499: filteredTimedItems.add(new SubsectionHeader( On 2017/02/16 21:15:11, shaktisahu wrote: > On 2017/02/16 17:48:17, Theresa wrote: > > Instead of constructing the SubsectionHeader each time we filter, would it > make > > sense to keep a map of Date -> SubsectionHeader? > > > > Right now every time a download item or offline page item is modified or the > > user selects a different filter or searches for anything, we'll end up > > reconstructing all of the SubsectionHeaders. It would be more efficient to > > update them when a suggested offline page is added/updated/removed. > > > > If we store the Date -> SubsectionHeader map as an instance variable, we could > > also use that in place of the mSubSectionExpanded map. > > I think this is okay, since they aren't that many (a few days probably), and > they really don't contain anything heavy. I wanted to keep a map of Boolean and > didn't want to maintain SubSectionHeaders to be less error prone when updating > states (add, remove, filter etc). But we could maintain these things either way > I guess. The backend is going to automatically remove suggested offline content after a few days? https://codereview.chromium.org/2670083002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:484: If the user is currently searching, should we show the suggested offline page header? If so, should we reset the expanded state (when is it reset currently)? https://codereview.chromium.org/2670083002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:341: * @param timedItem The item nit: add a period at the end of this line
https://codereview.chromium.org/2670083002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:499: filteredTimedItems.add(new SubsectionHeader( On 2017/02/17 23:56:34, Theresa wrote: > On 2017/02/16 21:15:11, shaktisahu wrote: > > On 2017/02/16 17:48:17, Theresa wrote: > > > Instead of constructing the SubsectionHeader each time we filter, would it > > make > > > sense to keep a map of Date -> SubsectionHeader? > > > > > > Right now every time a download item or offline page item is modified or the > > > user selects a different filter or searches for anything, we'll end up > > > reconstructing all of the SubsectionHeaders. It would be more efficient to > > > update them when a suggested offline page is added/updated/removed. > > > > > > If we store the Date -> SubsectionHeader map as an instance variable, we > could > > > also use that in place of the mSubSectionExpanded map. > > > > I think this is okay, since they aren't that many (a few days probably), and > > they really don't contain anything heavy. I wanted to keep a map of Boolean > and > > didn't want to maintain SubSectionHeaders to be less error prone when updating > > states (add, remove, filter etc). But we could maintain these things either > way > > I guess. > > The backend is going to automatically remove suggested offline content after a > few days? I would think so. I will check this with the UX if it can go for months :) https://codereview.chromium.org/2670083002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:484: On 2017/02/17 23:56:35, Theresa wrote: > If the user is currently searching, should we show the suggested offline page > header? If so, should we reset the expanded state (when is it reset currently)? Good point. I would think we would have to reset the expanded state and may have to update the page count on the header (if it is to be displayed). I would think it makes more sense not to show the header, which would also make it simpler. Anyway, this is a UX question. I will add a TODO for this. https://codereview.chromium.org/2670083002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java (right): https://codereview.chromium.org/2670083002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:341: * @param timedItem The item On 2017/02/17 23:56:35, Theresa wrote: > nit: add a period at the end of this line Done.
lgtm https://codereview.chromium.org/2670083002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:525: while (iter.hasNext()) { nit: can this be: for (Entry<Date, Booleean> entry : mSubsectionExpanded.entrySet()) { if (!pageCountMap....) } https://codereview.chromium.org/2670083002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java (right): https://codereview.chromium.org/2670083002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:43: * the top in the following order : download items, suggested pages header, suggested pages. nit: s/order :/order:
lgtm https://codereview.chromium.org/2670083002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:464: nit: remove newline
Yay! Thanks for all the feedback. https://codereview.chromium.org/2670083002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java (right): https://codereview.chromium.org/2670083002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:464: On 2017/02/21 18:32:27, dfalcantara (load balance plz) wrote: > nit: remove newline Done. https://codereview.chromium.org/2670083002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:525: while (iter.hasNext()) { On 2017/02/21 17:07:26, Theresa wrote: > nit: can this be: > > for (Entry<Date, Booleean> entry : mSubsectionExpanded.entrySet()) { > if (!pageCountMap....) > } I can't call remove on |mSubsectionExpanded| while using for loop. https://codereview.chromium.org/2670083002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java (right): https://codereview.chromium.org/2670083002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemGroup.java:43: * the top in the following order : download items, suggested pages header, suggested pages. On 2017/02/21 17:07:26, Theresa wrote: > nit: s/order :/order: Done.
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
The CQ bit was unchecked by shaktisahu@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by shaktisahu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2670083002/#ps280001 (title: "nits")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) 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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by shaktisahu@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by shaktisahu@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by shaktisahu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2670083002/#ps320001 (title: "FindBugs fix")
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
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by shaktisahu@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 shaktisahu@chromium.org
The CQ bit was checked by shaktisahu@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shaktisahu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1487809290618810,
"parent_rev": "3cd4d8506ad29a689491f46e90f177560de9e2d2", "commit_rev":
"1b901526d84b86839e58b340a117eef6b40a7706"}
Message was sent while issue was closed.
Description was changed from ========== [Download Home] Displaying offline page bundle per day This CL adds suggested offline pages to the DownloadHistoryAdapter. The suggested pages are grouped into a bundle for each day and placed at the bottom of each day's download history. The pages can be shown or hidden through tapping an up/down arrow on the subsection header. Key points of this CL: 1- To keep DateDividedAdapter unaware of offline suggestions, DateDividedAdapter.ItemGroup was subclassed into a separate DownloadItemGroup class which contains the special sorting function for this purpose. Based on whether the offline pages section is expanded or not, the offline items are filtered out before adding to the adapter. 2- For displaying the section header for the suggested offline pages, a TimedItem is added to the adapter which is passed the required information about the group it is associated to. OfflineGroupHeaderView and OfflineGroupHeaderViewHolder classes represent the view portion for the header. The section header is not selectable at the moment which would be addressed in a future CL. BUG=689801 ========== to ========== [Download Home] Displaying offline page bundle per day This CL adds suggested offline pages to the DownloadHistoryAdapter. The suggested pages are grouped into a bundle for each day and placed at the bottom of each day's download history. The pages can be shown or hidden through tapping an up/down arrow on the subsection header. Key points of this CL: 1- To keep DateDividedAdapter unaware of offline suggestions, DateDividedAdapter.ItemGroup was subclassed into a separate DownloadItemGroup class which contains the special sorting function for this purpose. Based on whether the offline pages section is expanded or not, the offline items are filtered out before adding to the adapter. 2- For displaying the section header for the suggested offline pages, a TimedItem is added to the adapter which is passed the required information about the group it is associated to. OfflineGroupHeaderView and OfflineGroupHeaderViewHolder classes represent the view portion for the header. The section header is not selectable at the moment which would be addressed in a future CL. BUG=689801 Review-Url: https://codereview.chromium.org/2670083002 Cr-Commit-Position: refs/heads/master@{#452361} Committed: https://chromium.googlesource.com/chromium/src/+/1b901526d84b86839e58b340a117... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/1b901526d84b86839e58b340a117...
Message was sent while issue was closed.
On 2017/02/23 02:58:46, commit-bot: I haz the power wrote: > Committed patchset #17 (id:320001) as > https://chromium.googlesource.com/chromium/src/+/1b901526d84b86839e58b340a117... FYI, I'm seeing lint errors on current branch-heads/3029 that look related: FAILED: gen/chrome/android/chrome_java__lint/result.xml gen/chrome/android/chrome_java__lint/config.xml python ../../build/android/gyp/lint.py --lint-path=../../third_party/android_tools/sdk/tools/lint --cache-dir android_lint_cache --platform-xml-path ../../third_party/android_tools_internal/sdk/platform-tools/api/api-versions.xml --android-sdk-version=24 --depfile gen/chrome/android/chrome_java__lint.d --config-path ../../build/android/lint/suppressions.xml --manifest-path gen/chrome/android/chrome_public_apk/AndroidManifest.xml --product-dir=. --processed-config-path gen/chrome/android/chrome_java__lint/config.xml --result-path gen/chrome/android/chrome_java__lint/result.xml --java-sources-file=gen/chrome/android/chrome_java.sources --jar-path lib.java/chrome/android/chrome_java.jar --classpath=@FileArg\(gen/chrome/android/chrome_java.build_config:javac:interface_classpath\) --resource-sources=@FileArg\(gen/chrome/android/chrome_java.build_config:deps_info:owned_resources_dirs\) --resource-sources=@FileArg\(gen/chrome/android/chrome_java.build_config:deps_info:owned_resources_zips\) --can-fail-build /tmp/tmpaL6LwC/SRC_ROOT1/chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:67 Format string '`download_manager_offline_header_title`' is not a valid format string so it should not be passed to `String.format`: StringFormatInvalid [warning] mPageCountHeader.setText(getResources().getString( ^ I'm not sure why this would be popping up a month later though, any idea what's going on?
Message was sent while issue was closed.
Description was changed from ========== [Download Home] Displaying offline page bundle per day This CL adds suggested offline pages to the DownloadHistoryAdapter. The suggested pages are grouped into a bundle for each day and placed at the bottom of each day's download history. The pages can be shown or hidden through tapping an up/down arrow on the subsection header. Key points of this CL: 1- To keep DateDividedAdapter unaware of offline suggestions, DateDividedAdapter.ItemGroup was subclassed into a separate DownloadItemGroup class which contains the special sorting function for this purpose. Based on whether the offline pages section is expanded or not, the offline items are filtered out before adding to the adapter. 2- For displaying the section header for the suggested offline pages, a TimedItem is added to the adapter which is passed the required information about the group it is associated to. OfflineGroupHeaderView and OfflineGroupHeaderViewHolder classes represent the view portion for the header. The section header is not selectable at the moment which would be addressed in a future CL. BUG=689801 Review-Url: https://codereview.chromium.org/2670083002 Cr-Commit-Position: refs/heads/master@{#452361} Committed: https://chromium.googlesource.com/chromium/src/+/1b901526d84b86839e58b340a117... ========== to ========== [Download Home] Displaying offline page bundle per day This CL adds suggested offline pages to the DownloadHistoryAdapter. The suggested pages are grouped into a bundle for each day and placed at the bottom of each day's download history. The pages can be shown or hidden through tapping an up/down arrow on the subsection header. Key points of this CL: 1- To keep DateDividedAdapter unaware of offline suggestions, DateDividedAdapter.ItemGroup was subclassed into a separate DownloadItemGroup class which contains the special sorting function for this purpose. Based on whether the offline pages section is expanded or not, the offline items are filtered out before adding to the adapter. 2- For displaying the section header for the suggested offline pages, a TimedItem is added to the adapter which is passed the required information about the group it is associated to. OfflineGroupHeaderView and OfflineGroupHeaderViewHolder classes represent the view portion for the header. The section header is not selectable at the moment which would be addressed in a future CL. BUG=689801 Review-Url: https://codereview.chromium.org/2670083002 Cr-Commit-Position: refs/heads/master@{#452361} Committed: https://chromium.googlesource.com/chromium/src/+/1b901526d84b86839e58b340a117... ==========
Message was sent while issue was closed.
On 2017/03/23 21:06:18, klausw wrote: > On 2017/02/23 02:58:46, commit-bot: I haz the power wrote: > > Committed patchset #17 (id:320001) as > > > https://chromium.googlesource.com/chromium/src/+/1b901526d84b86839e58b340a117... > > FYI, I'm seeing lint errors on current branch-heads/3029 that look related: > > FAILED: gen/chrome/android/chrome_java__lint/result.xml > gen/chrome/android/chrome_java__lint/config.xml > python ../../build/android/gyp/lint.py > --lint-path=../../third_party/android_tools/sdk/tools/lint --cache-dir > android_lint_cache --platform-xml-path > ../../third_party/android_tools_internal/sdk/platform-tools/api/api-versions.xml > --android-sdk-version=24 --depfile gen/chrome/android/chrome_java__lint.d > --config-path ../../build/android/lint/suppressions.xml --manifest-path > gen/chrome/android/chrome_public_apk/AndroidManifest.xml --product-dir=. > --processed-config-path gen/chrome/android/chrome_java__lint/config.xml > --result-path gen/chrome/android/chrome_java__lint/result.xml > --java-sources-file=gen/chrome/android/chrome_java.sources --jar-path > lib.java/chrome/android/chrome_java.jar > --classpath=@FileArg\(gen/chrome/android/chrome_java.build_config:javac:interface_classpath\) > --resource-sources=@FileArg\(gen/chrome/android/chrome_java.build_config:deps_info:owned_resources_dirs\) > --resource-sources=@FileArg\(gen/chrome/android/chrome_java.build_config:deps_info:owned_resources_zips\) > --can-fail-build > > /tmp/tmpaL6LwC/SRC_ROOT1/chrome/android/java/src/org/chromium/chrome/browser/download/ui/OfflineGroupHeaderView.java:67 > Format string '`download_manager_offline_header_title`' is not a valid format > string so it should not be passed to `String.format`: StringFormatInvalid > [warning] > mPageCountHeader.setText(getResources().getString( > ^ > > I'm not sure why this would be popping up a month later though, any idea what's > going on? Looks like this is inconsistent translated resources as per https://bugs.chromium.org/p/chromium/issues/detail?id=703486 , being fixed separately. |
