Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2688383005:
[NTP::Downloads] Show thumbnails for Download suggestions. (Closed)
Description[NTP::Downloads] Show thumbnails for Download suggestions.
1) Move some bits in DownloadItemView into functions and make them
public, so that NTP can reuse them;
2) Copy DownloadItemView thumbnails behaviour for Download suggestions
on the NTP:
- for images show preview;
- for other asset downloads show mimeType based icons;
- for offline pages show specific icon;
BUG=690333, 690332
Review-Url: https://codereview.chromium.org/2688383005
Cr-Commit-Position: refs/heads/master@{#450925}
Committed: https://chromium.googlesource.com/chromium/src/+/15b486bc4330fb7892946cca2ba60eaafc24cea6
Patch Set 1 #
Total comments: 16
Patch Set 2 : rebase. #
Total comments: 10
Patch Set 3 : comments. #
Total comments: 16
Patch Set 4 : comments. #Patch Set 5 : clean rebase + dfalcantara@ comment + isRecycled() check in DownloadItemView #Messages
Total messages: 39 (21 generated)
The CQ bit was checked by vitaliii@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...
vitaliii@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
Hi mvanouwerkerk@, Could you have a look at ntp_snippets java bits (as an OWNER) and Downloads ui java bits (just in case you have an advice how to share more code)?
vitaliii@chromium.org changed reviewers: + jkrcal@chromium.org
Hi jkrcal@, Could you have a look at download_suggestions_provider.cc?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
vitaliii@chromium.org changed reviewers: + dfalcantara@chromium.org
Hi dfalcantara@, Could you have a look at download/ui/ please? This is probably far from being finished yet, but at least a high level WDYT? (sorry for somewhat premature review, but there is not much time left before M58 FF and losing one iteration of communication would be a shame)
On 2017/02/13 21:00:09, vitaliii wrote: > Hi dfalcantara@, > > Could you have a look at download/ui/ please? > This is probably far from being finished yet, but at least a high level WDYT? > (sorry for somewhat premature review, but there is not much time left before M58 > FF and losing one iteration of communication would be a shame) Are there screenshots?
High level l-g-t-m https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemView.java (right): https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemView.java:132: public static int getIconResId(int fileType) { 1) Public methods need javadocs. 2) These new methods should probably be moved to DownloadUtils, at this point. https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:120: public boolean isAssetDownload() { isAssetDownload sounds like it's just an accessor when it's actually more complicated than that. Can you rename either the member field or the function name? https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:93: private final ThumbnailProvider mThumbnailProvider; private finals above regular privates https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:265: && thumbnail.getWidth() != 0 && thumbnail.getHeight() != 0) { Using > 0 for these makes more sense, I think. Anything negative should be construed as an error condition for dimensions. https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:269: mThumbnailView.setTint(null); These four lines are (sort of) repeated. Should they be pulled into a private function to guarantee that everything is set correctly at the same time? Maybe with two variations: one that takes in a bitmap, and one that takes in a resource, loads it, then passes it along to the first version? https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:274: if (mArticle.mIsAssetDownload) { Given that mIsAssetDownload != isAssetDownload(), you should use isAssetDownload() for consistency everywhere. I know you do the isDownload() check before calling this, but it's not a guarantee that future calls to this function will do the same check. https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:324: if (mArticle.isDownload()) { It's weird to breaking only part of this thumbnail setting logic out. Can you yank it all out into a function setThumbnail() (or something) instead of just the setDownloadThumbnail function?
download_suggestions_provider.cc lgtm
dgn@chromium.org changed reviewers: + dgn@chromium.org
lgtm https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:273: private void setDownloadThumbnail() { nit: member order: public methods > private methods > inner classes https://codereview.chromium.org/2688383005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2688383005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:263: public void onThumbnailRetrieved(String filePath, Bitmap thumbnail) { nit: For the sake of encapsulation and smaller public APIs, I'd prefer the ThumbnailRequest implementation to be an inner class that we instantiate only when we need, but I might be overdoing it. Michael wdyt? https://codereview.chromium.org/2688383005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:273: private void setDownloadThumbnail() { nit: private methods after public methods but before inner classes https://codereview.chromium.org/2688383005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:284: mThumbnailView.setBackground(null); why do this after setting the color/icon/tint a few lines above? I think some utility methods like setThumbnailFromFileType(int) and setThumbnailFromBitmap(Bitmap (or Drawable, whatever it is)) would bring some clarity here. Also, prefer early returns to nested ifs https://codereview.chromium.org/2688383005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:321: if (mArticle.getThumbnailBitmap() != null) { I guess the entire thumbnail handling from here can be moved to a neat method
Didn't mean to lgtm yet :/
On 2017/02/13 21:12:40, dfalcantara (load balance plz) wrote: > On 2017/02/13 21:00:09, vitaliii wrote: > > Hi dfalcantara@, > > > > Could you have a look at download/ui/ please? > > This is probably far from being finished yet, but at least a high level WDYT? > > (sorry for somewhat premature review, but there is not much time left before > M58 > > FF and losing one iteration of communication would be a shame) > > Are there screenshots? Currently it looks like this (please ignore the strings) https://bugs.chromium.org/p/chromium/issues/detail?id=631447#c34 We expect it looks like this (please ignore the strings) once https://codereview.chromium.org/2690123002 lands https://bugs.chromium.org/p/chromium/issues/detail?id=690420#c5
https://codereview.chromium.org/2688383005/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/new_tab_page_snippets_card.xml (right): https://codereview.chromium.org/2688383005/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_card.xml:94: android:scaleType="center" Does this change also affect regular article thumbnails? Are they all of exactly size snippets_thumbnail_size or did they need the scaling?
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by vitaliii@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...
[jkrcal@ please ignore this] Hi, I addressed your comments. Could you have a look? [To London folks] Also occasionally ThumbnailProviderImpl does not reply asynchronously at all. I tried to debug and it seems that this is not in my code. Can this be related to reusing of views? https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemView.java (right): https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemView.java:132: public static int getIconResId(int fileType) { On 2017/02/13 21:35:44, dfalcantara (load balance plz) wrote: > 1) Public methods need javadocs. > 2) These new methods should probably be moved to DownloadUtils, at this point. Done. https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:120: public boolean isAssetDownload() { On 2017/02/13 21:35:44, dfalcantara (load balance plz) wrote: > isAssetDownload sounds like it's just an accessor when it's actually more > complicated than that. Can you rename either the member field or the function > name? Done. I made the function an accessor. https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:93: private final ThumbnailProvider mThumbnailProvider; On 2017/02/13 21:35:44, dfalcantara (load balance plz) wrote: > private finals above regular privates Done. https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:265: && thumbnail.getWidth() != 0 && thumbnail.getHeight() != 0) { On 2017/02/13 21:35:44, dfalcantara (load balance plz) wrote: > Using > 0 for these makes more sense, I think. Anything negative should be > construed as an error condition for dimensions. Done. I also changed this in DownloadItemView.onThumbnailRetrieved. https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:269: mThumbnailView.setTint(null); On 2017/02/13 21:35:44, dfalcantara (load balance plz) wrote: > These four lines are (sort of) repeated. Should they be pulled into a private > function to guarantee that everything is set correctly at the same time? Maybe > with two variations: one that takes in a bitmap, and one that takes in a > resource, loads it, then passes it along to the first version? Somewhat done. They require different background and tint, so passing to the first version is not possible. https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:273: private void setDownloadThumbnail() { On 2017/02/14 10:54:28, dgn wrote: > nit: member order: public methods > private methods > inner classes Done. I had to move some unrelated methods. https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:274: if (mArticle.mIsAssetDownload) { On 2017/02/13 21:35:45, dfalcantara (load balance plz) wrote: > Given that mIsAssetDownload != isAssetDownload(), you should use > isAssetDownload() for consistency everywhere. I know you do the isDownload() > check before calling this, but it's not a guarantee that future calls to this > function will do the same check. Done. I added assert mArticle.isDownload() and I use an accessor |isAssetDownload| instead of getting the member directly. https://codereview.chromium.org/2688383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:324: if (mArticle.isDownload()) { On 2017/02/13 21:35:44, dfalcantara (load balance plz) wrote: > It's weird to breaking only part of this thumbnail setting logic out. Can you > yank it all out into a function setThumbnail() (or something) instead of just > the setDownloadThumbnail function? Done. I left both setThumbnail and setDownloadThumbnail. https://codereview.chromium.org/2688383005/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/new_tab_page_snippets_card.xml (right): https://codereview.chromium.org/2688383005/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_card.xml:94: android:scaleType="center" On 2017/02/14 13:35:31, Michael van Ouwerkerk wrote: > Does this change also affect regular article thumbnails? Are they all of exactly > size snippets_thumbnail_size or did they need the scaling? Done. Good observation! Now I set different scaleType for bitmaps and icons. https://codereview.chromium.org/2688383005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2688383005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:263: public void onThumbnailRetrieved(String filePath, Bitmap thumbnail) { On 2017/02/14 10:54:28, dgn wrote: > nit: For the sake of encapsulation and smaller public APIs, I'd prefer the > ThumbnailRequest implementation to be an inner class that we instantiate only > when we need, but I might be overdoing it. Michael wdyt? Done. https://codereview.chromium.org/2688383005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:273: private void setDownloadThumbnail() { On 2017/02/14 10:54:28, dgn wrote: > nit: private methods after public methods but before inner classes Done. https://codereview.chromium.org/2688383005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:284: mThumbnailView.setBackground(null); On 2017/02/14 10:54:28, dgn wrote: > why do this after setting the color/icon/tint a few lines above? Because color/icon/tint are not needed when we show bitmap (and it is done in similar fashion in DownloadItemView). > I think some utility methods like setThumbnailFromFileType(int) and > setThumbnailFromBitmap(Bitmap (or Drawable, whatever it is)) would bring some > clarity here. > > Also, prefer early returns to nested ifs Done. https://codereview.chromium.org/2688383005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:321: if (mArticle.getThumbnailBitmap() != null) { On 2017/02/14 10:54:28, dgn wrote: > I guess the entire thumbnail handling from here can be moved to a neat method Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm now :) https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:85: private final ColorStateList mWhiteTint; nit: maybe call it mIconForegroundColor (or ColorList or ColorSpecs something like that)? The DownloadUtils getter could be renamed to. "WhiteTint" in the context of this class is not meaningful. https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:290: if (thumbnail != null && !thumbnail.isRecycled()) { nit: early return. There is also a few places below (in the new code) were you could do remove nested blocks https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:330: // If the article has a thumbnail already, reuse it. Otherwise start a fetch. nit: comment is a bit out of place (only really applies to the last part of the method) and outdated
lgtm % questions https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java (right): https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:664: * @param fileType Type of the file as returned by DownloadFilter nit: Add periods for consistency with the rest of the file, then fix getAbbreviatedFileName to also have periods at the end of its comment lines. https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:171: public void onBindViewHolder(SnippetArticle article, SuggestionsCategoryInfo categoryInfo) { @Override. https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:295: } Shouldn't this do something in the else case? Seems like this will just continue showing whatever thumbnail was there before. If you don't want an else case, this should assert that the thumbnail isn't null and that it hasn't been recycled. https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:344: mThumbnailView.setBackground(null); Add a comment about what this block is for.
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I addressed your comments. No action is required. Feel free to have a look if you like. Note there is https://bugs.chromium.org/p/chromium/issues/detail?id=692609 and it is not related to this CL. I am going to submit this CL tomorrow. https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java (right): https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:664: * @param fileType Type of the file as returned by DownloadFilter On 2017/02/14 21:46:40, dfalcantara (load balance plz) wrote: > nit: Add periods for consistency with the rest of the file, then fix > getAbbreviatedFileName to also have periods at the end of its comment lines. Done. I should have looked at more methods before deciding that the style is no periods. https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:85: private final ColorStateList mWhiteTint; On 2017/02/14 16:59:35, dgn wrote: > nit: maybe call it mIconForegroundColor (or ColorList or ColorSpecs something > like that)? The DownloadUtils getter could be renamed to. "WhiteTint" in the > context of this class is not meaningful. Done. https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:171: public void onBindViewHolder(SnippetArticle article, SuggestionsCategoryInfo categoryInfo) { On 2017/02/14 21:46:40, dfalcantara (load balance plz) wrote: > @Override. Can't be done. CardViewHolder.InBindViewHolder does not take any arguments. https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:290: if (thumbnail != null && !thumbnail.isRecycled()) { On 2017/02/14 16:59:35, dgn wrote: > nit: early return. There is also a few places below (in the new code) were you > could do remove nested blocks Done. https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:295: } On 2017/02/14 21:46:40, dfalcantara (load balance plz) wrote: > Shouldn't this do something in the else case? Seems like this will just > continue showing whatever thumbnail was there before. If you don't want an else > case, this should assert that the thumbnail isn't null and that it hasn't been > recycled. Done (but differently). I replaced |if| with the assert. https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:330: // If the article has a thumbnail already, reuse it. Otherwise start a fetch. On 2017/02/14 16:59:35, dgn wrote: > nit: comment is a bit out of place (only really applies to the last part of the > method) and outdated Done. Removed. https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:344: mThumbnailView.setBackground(null); On 2017/02/14 21:46:40, dfalcantara (load balance plz) wrote: > Add a comment about what this block is for. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
(still lgtm) https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:171: public void onBindViewHolder(SnippetArticle article, SuggestionsCategoryInfo categoryInfo) { On 2017/02/15 17:26:01, vitaliii wrote: > On 2017/02/14 21:46:40, dfalcantara (load balance plz) wrote: > > @Override. > Can't be done. CardViewHolder.InBindViewHolder does not take any arguments. Add a javadoc for the non-overridden public method, s'il vous plait.
Hi, I addressed dfalcantara@'s comment. No need to look. I also added checks for isRecycled() in DownloadItemView. https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2688383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:171: public void onBindViewHolder(SnippetArticle article, SuggestionsCategoryInfo categoryInfo) { On 2017/02/15 18:55:35, dfalcantara (load balance plz) wrote: > On 2017/02/15 17:26:01, vitaliii wrote: > > On 2017/02/14 21:46:40, dfalcantara (load balance plz) wrote: > > > @Override. > > Can't be done. CardViewHolder.InBindViewHolder does not take any arguments. > > Add a javadoc for the non-overridden public method, s'il vous plait. Done.
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org, dgn@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2688383005/#ps100001 (title: "clean rebase + dfalcantara@ comment + isRecycled() check in DownloadItemView")
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": 100001, "attempt_start_ts": 1487235526290230,
"parent_rev": "0699e0240d57d5b557b59c5d4060cf86d2b46f17", "commit_rev":
"15b486bc4330fb7892946cca2ba60eaafc24cea6"}
Message was sent while issue was closed.
Description was changed from ========== [NTP::Downloads] Show thumbnails for Download suggestions. 1) Move some bits in DownloadItemView into functions and make them public, so that NTP can reuse them; 2) Copy DownloadItemView thumbnails behaviour for Download suggestions on the NTP: - for images show preview; - for other asset downloads show mimeType based icons; - for offline pages show specific icon; BUG=690333,690332 ========== to ========== [NTP::Downloads] Show thumbnails for Download suggestions. 1) Move some bits in DownloadItemView into functions and make them public, so that NTP can reuse them; 2) Copy DownloadItemView thumbnails behaviour for Download suggestions on the NTP: - for images show preview; - for other asset downloads show mimeType based icons; - for offline pages show specific icon; BUG=690333,690332 Review-Url: https://codereview.chromium.org/2688383005 Cr-Commit-Position: refs/heads/master@{#450925} Committed: https://chromium.googlesource.com/chromium/src/+/15b486bc4330fb7892946cca2ba6... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/15b486bc4330fb7892946cca2ba6... |
