Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1871343006:
[NTP Snippets] 📷 Add placeholders and start caching thumbnails (Closed)
Description[NTP Snippets] Add placeholders and start caching thumbnails
Adds placeholders that are displayed while the thumbnail is being
fetched from the network, and keeps the downloaded thumbnail while
we stay on the NTP.
BUG=601456
Committed: https://crrev.com/204cf9c4eb767c7f8cfbd85267c4b9e87b62067d
Cr-Commit-Position: refs/heads/master@{#387262}
Patch Set 1 #Patch Set 2 : move the async task out as it needs to be stopped from the view, not the snippet #
Total comments: 29
Patch Set 3 : Address comments #
Total comments: 6
Patch Set 4 : Address lint errors #Patch Set 5 : #
Total comments: 4
Patch Set 6 : Address comments #
Total comments: 8
Patch Set 7 : Address comments #
Messages
Total messages: 32 (10 generated)
Description was changed from ========== [NTP Snippets] Add placeholders and start caching thumbnails Adds placeholders that are displayed while the thumbnail is being fetched from the network, and keeps the downloaded thumbnail while we stay on the NTP. BUG=601456 ========== to ========== [NTP Snippets] 📷 Add placeholders and start caching thumbnails Adds placeholders that are displayed while the thumbnail is being fetched from the network, and keeps the downloaded thumbnail while we stay on the NTP. BUG=601456 ==========
dgn@chromium.org changed reviewers: + bauerb@chromium.org, twellington@chromium.org
PTAL Preview of the changes: https://goo.gl/photos/SmrJ5bnGfTehxqYR8
https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/res... File chrome/android/java/res/drawable/ic_snippet_thumbnail_placeholder.xml (right): https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/res... chrome/android/java/res/drawable/ic_snippet_thumbnail_placeholder.xml:3: android:width="72dp" Use @dimen/snippets_thumbnail_size https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/res... chrome/android/java/res/drawable/ic_snippet_thumbnail_placeholder.xml:9: android:pathData="M0,0 L72,0 L72,71.9997 L0,71.9997 Z" /> Can this fit w/ the line above? https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/res... chrome/android/java/res/drawable/ic_snippet_thumbnail_placeholder.xml:11: android:fillColor="#757575" define a color in colors.xml for this https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/new_tab_page_snippets_card.xml (right): https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_card.xml:59: android:contentDescription="@null" Does the content description get set elsewhere? This is important for accessibility. https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java (right): https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java:30: * we are prototyping this feature for clank. For the real production feature, we Avoid code names in public CLs. https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java:42: * @param snippetArticle the article for which we want to download the thumbnail Nit: use proper sentence capitalization/punctuation on comments. "The article... thumbnail." Also capitalize "The" below. https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java:50: mThumbnailView.setImageResource(R.drawable.ic_snippet_thumbnail_placeholder); Vector drawables are an L+ feature. Did you test this pre-L? I think this should just work pre-L (except for the lint warning... not sure how to work around that - maybe Stack Overflow has some ideas) since Chrome extends AppCompatActivity and we recently rolled the AppCompat support library to a version that includes vector support, but it would be good to test. If it doesn't work, here's a resource on AppCompat support for vector drawables: https://chris.banes.me/2016/02/25/appcompat-vector/ https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java:69: is = (InputStream) new URL(this.mSnippetArticle.mThumbnailUrl).getContent(); Can you use openStream() instead to avoid the cast? https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java:72: int targetSize = (int) res.getDimension(R.dimen.snippets_thumbnail_size); Use res.getDimensionPixelSize() - this will convert to an int for you. https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java:92: // Cross-fade between the placeholder and the thumbnail nit: add a period to the end of this sentence.
https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java (right): https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java:51: } else { Instead of doing this in the constructor (and the check for an empty URL below), could we skip creating the task completely in this case? https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java:52: mThumbnailView.setImageBitmap(this.mSnippetArticle.getThumbnailBitmap()); The |this| isn't necessary anymore now that this is a top-level class. https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java:90: this.mSnippetArticle.setThumbnailBitmap(thumbnail); Hm... Ideally I think I'd like this to be a bit more decoupled from the model and the view. For example, we could make it so that this task doesn't know about the classes that use it (mSnippetArticle and mThumbnailView), so it only does the fetching, then its clients add the callbacks that determine what happens when we get a bitmap back. See also my comment in line 51: That would also be something that the client of this class would do.
Also, as much as I like the 📷 in the CL description, Monorail currently has this issue where it cuts off comments as soon as it encounters a non-BMP character, which might mess with the bugdroid comments. This is why we can't have nice things 😢
On 2016/04/12 08:36:31, Bernhard Bauer wrote: > Also, as much as I like the 📷 in the CL description, Monorail currently has this > issue where it cuts off comments as soon as it encounters a non-BMP character (See https://bugs.chromium.org/p/monorail/issues/detail?id=705)
Description was changed from ========== [NTP Snippets] 📷 Add placeholders and start caching thumbnails Adds placeholders that are displayed while the thumbnail is being fetched from the network, and keeps the downloaded thumbnail while we stay on the NTP. BUG=601456 ========== to ========== [NTP Snippets] Add placeholders and start caching thumbnails Adds placeholders that are displayed while the thumbnail is being fetched from the network, and keeps the downloaded thumbnail while we stay on the NTP. BUG=601456 ==========
PTAL https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/res... File chrome/android/java/res/drawable/ic_snippet_thumbnail_placeholder.xml (right): https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/res... chrome/android/java/res/drawable/ic_snippet_thumbnail_placeholder.xml:3: android:width="72dp" On 2016/04/11 23:42:27, Theresa Wellington wrote: > Use @dimen/snippets_thumbnail_size Done. https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/res... chrome/android/java/res/drawable/ic_snippet_thumbnail_placeholder.xml:9: android:pathData="M0,0 L72,0 L72,71.9997 L0,71.9997 Z" /> On 2016/04/11 23:42:27, Theresa Wellington wrote: > Can this fit w/ the line above? Done. https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/res... chrome/android/java/res/drawable/ic_snippet_thumbnail_placeholder.xml:11: android:fillColor="#757575" On 2016/04/11 23:42:27, Theresa Wellington wrote: > define a color in colors.xml for this Done. https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/new_tab_page_snippets_card.xml (right): https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_card.xml:59: android:contentDescription="@null" On 2016/04/11 23:42:27, Theresa Wellington wrote: > Does the content description get set elsewhere? This is important for > accessibility. Not as far as I can see. That being said, it's content that is not actionable and can't really be described. I can't think of a description here that would be meaningful and not just end up being noise. ("thumbnail", "thumbnail", "thumbnail" ...) Any idea? https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java (right): https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java:30: * we are prototyping this feature for clank. For the real production feature, we On 2016/04/11 23:42:27, Theresa Wellington wrote: > Avoid code names in public CLs. Done. https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java:42: * @param snippetArticle the article for which we want to download the thumbnail On 2016/04/11 23:42:27, Theresa Wellington wrote: > Nit: use proper sentence capitalization/punctuation on comments. "The article... > thumbnail." > > Also capitalize "The" below. Done. https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java:50: mThumbnailView.setImageResource(R.drawable.ic_snippet_thumbnail_placeholder); On 2016/04/11 23:42:27, Theresa Wellington wrote: > Vector drawables are an L+ feature. Did you test this pre-L? I think this should > just work pre-L (except for the lint warning... not sure how to work around that > - maybe Stack Overflow has some ideas) since Chrome extends AppCompatActivity > and we recently rolled the AppCompat support library to a version that includes > vector support, but it would be good to test. > > If it doesn't work, here's a resource on AppCompat support for vector drawables: > https://chris.banes.me/2016/02/25/appcompat-vector/ Yes I looked into that, apparently there is nothing special to do. We had to change some build files when updating the support library to 23.2 (which ports the VectorDrawable support and starts using them internally), but new they worked just fine on my KitKat device. https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java:51: } else { On 2016/04/12 08:34:34, Bernhard Bauer wrote: > Instead of doing this in the constructor (and the check for an empty URL below), > could we skip creating the task completely in this case? Done. https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java:52: mThumbnailView.setImageBitmap(this.mSnippetArticle.getThumbnailBitmap()); On 2016/04/12 08:34:34, Bernhard Bauer wrote: > The |this| isn't necessary anymore now that this is a top-level class. Done. https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java:69: is = (InputStream) new URL(this.mSnippetArticle.mThumbnailUrl).getContent(); On 2016/04/11 23:42:27, Theresa Wellington wrote: > Can you use openStream() instead to avoid the cast? Done. https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java:72: int targetSize = (int) res.getDimension(R.dimen.snippets_thumbnail_size); On 2016/04/11 23:42:27, Theresa Wellington wrote: > Use res.getDimensionPixelSize() - this will convert to an int for you. Thanks! Done. https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java:90: this.mSnippetArticle.setThumbnailBitmap(thumbnail); On 2016/04/12 08:34:34, Bernhard Bauer wrote: > Hm... Ideally I think I'd like this to be a bit more decoupled from the model > and the view. For example, we could make it so that this task doesn't know about > the classes that use it (mSnippetArticle and mThumbnailView), so it only does > the fetching, then its clients add the callbacks that determine what happens > when we get a bitmap back. See also my comment in line 51: That would also be > something that the client of this class would do. Done. After that there was pretty much nothing but the actual download in this class so I just moved it to the ViewHolder. https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java:92: // Cross-fade between the placeholder and the thumbnail On 2016/04/11 23:42:27, Theresa Wellington wrote: > nit: add a period to the end of this sentence. Done.
https://codereview.chromium.org/1871343006/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1871343006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:118: mThumbnailView.setImageBitmap(snippet.getThumbnailBitmap()); Return in this case? https://codereview.chromium.org/1871343006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:127: Log.e(TAG, "Could not get image thumbnail due to empty URL"); Move this out of the task? https://codereview.chromium.org/1871343006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:142: fadeThumbnailIn(snippet, result); Skip this if the result is null?
PTAL https://codereview.chromium.org/1871343006/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1871343006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:118: mThumbnailView.setImageBitmap(snippet.getThumbnailBitmap()); On 2016/04/12 11:21:19, Bernhard Bauer wrote: > Return in this case? Done. https://codereview.chromium.org/1871343006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:127: Log.e(TAG, "Could not get image thumbnail due to empty URL"); On 2016/04/12 11:21:19, Bernhard Bauer wrote: > Move this out of the task? Done. https://codereview.chromium.org/1871343006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:142: fadeThumbnailIn(snippet, result); On 2016/04/12 11:21:19, Bernhard Bauer wrote: > Skip this if the result is null? Done. https://codereview.chromium.org/1871343006/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1871343006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:135: try { API 19 minimum for try-with-resources :(
lgtm https://codereview.chromium.org/1871343006/diff/80001/chrome/android/java/res... File chrome/android/java/res/drawable/ic_snippet_thumbnail_placeholder.xml (right): https://codereview.chromium.org/1871343006/diff/80001/chrome/android/java/res... chrome/android/java/res/drawable/ic_snippet_thumbnail_placeholder.xml:2: <!-- TODO(https://crbug.com/602627) tools:targetApi added mute a lint error.--> "tools:targetApi added _to_ mute a lint error"? https://codereview.chromium.org/1871343006/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1871343006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:135: try { On 2016/04/12 13:40:21, dgn wrote: > API 19 minimum for try-with-resources :( Yeah, I was wondering about that. (Sorry, should have mentioned it.)
dgn@chromium.org changed reviewers: + yusufo@chromium.org
yusufo@chromium.org: Please review changes in chrome/android/java/res
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1871343006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1871343006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/new_tab_page_snippets_card.xml (right): https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_card.xml:59: android:contentDescription="@null" On 2016/04/12 11:17:14, dgn wrote: > On 2016/04/11 23:42:27, Theresa Wellington wrote: > > Does the content description get set elsewhere? This is important for > > accessibility. > > Not as far as I can see. That being said, it's content that is not actionable > and can't really be described. I can't think of a description here that would be > meaningful and not just end up being noise. ("thumbnail", "thumbnail", > "thumbnail" ...) Any idea? As long as the snippet itself has a content description I think that's fine. Can you test with talk back on please to see what gets read out for the snippet? Since this has no content description, it would be good to confirm that the thumbnail can't get focus for accessibility. This might need android:importantForAccessibility="no" or android:focusableInTouchMode="false".
https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/new_tab_page_snippets_card.xml (right): https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_card.xml:59: android:contentDescription="@null" On 2016/04/12 18:11:48, Theresa Wellington wrote: > On 2016/04/12 11:17:14, dgn wrote: > > On 2016/04/11 23:42:27, Theresa Wellington wrote: > > > Does the content description get set elsewhere? This is important for > > > accessibility. > > > > Not as far as I can see. That being said, it's content that is not actionable > > and can't really be described. I can't think of a description here that would > be > > meaningful and not just end up being noise. ("thumbnail", "thumbnail", > > "thumbnail" ...) Any idea? > > As long as the snippet itself has a content description I think that's fine. Can > you test with talk back on please to see what gets read out for the snippet? > > Since this has no content description, it would be good to confirm that the > thumbnail can't get focus for accessibility. This might need > android:importantForAccessibility="no" or android:focusableInTouchMode="false". TalkBack properly skips it. Looking on the net, it looks like screen readers skipping entirely elements with the @null description is the expected behaviour. Other concern, thew full card only can be selected. individual text fields inside it are not selectable in TalkBack mode, so it just reads everything at once. That's weird but we are planning to have a complete a11y pass on the whole new UI, so it should be addressed then. https://codereview.chromium.org/1871343006/diff/80001/chrome/android/java/res... File chrome/android/java/res/drawable/ic_snippet_thumbnail_placeholder.xml (right): https://codereview.chromium.org/1871343006/diff/80001/chrome/android/java/res... chrome/android/java/res/drawable/ic_snippet_thumbnail_placeholder.xml:2: <!-- TODO(https://crbug.com/602627) tools:targetApi added mute a lint error.--> On 2016/04/12 13:46:19, Bernhard Bauer wrote: > "tools:targetApi added _to_ mute a lint error"? Done.
https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/new_tab_page_snippets_card.xml (right): https://codereview.chromium.org/1871343006/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_card.xml:59: android:contentDescription="@null" On 2016/04/13 16:52:21, dgn wrote: > On 2016/04/12 18:11:48, Theresa Wellington wrote: > > On 2016/04/12 11:17:14, dgn wrote: > > > On 2016/04/11 23:42:27, Theresa Wellington wrote: > > > > Does the content description get set elsewhere? This is important for > > > > accessibility. > > > > > > Not as far as I can see. That being said, it's content that is not > actionable > > > and can't really be described. I can't think of a description here that > would > > be > > > meaningful and not just end up being noise. ("thumbnail", "thumbnail", > > > "thumbnail" ...) Any idea? > > > > As long as the snippet itself has a content description I think that's fine. > Can > > you test with talk back on please to see what gets read out for the snippet? > > > > Since this has no content description, it would be good to confirm that the > > thumbnail can't get focus for accessibility. This might need > > android:importantForAccessibility="no" or > android:focusableInTouchMode="false". > > TalkBack properly skips it. Looking on the net, it looks like screen readers > skipping entirely elements with the @null description is the expected behaviour. > Other concern, thew full card only can be selected. individual text fields > inside it are not selectable in TalkBack mode, so it just reads everything at > once. That's weird but we are planning to have a complete a11y pass on the whole > new UI, so it should be addressed then. sg, thanks for testing :)
https://codereview.chromium.org/1871343006/diff/100001/chrome/android/java/re... File chrome/android/java/res/drawable/ic_snippet_thumbnail_placeholder.xml (right): https://codereview.chromium.org/1871343006/diff/100001/chrome/android/java/re... chrome/android/java/res/drawable/ic_snippet_thumbnail_placeholder.xml:2: <!-- TODO(https://crbug.com/602627) tools:targetApi added to mute the NewApi lint error. --> is this lint being over cautious then? https://codereview.chromium.org/1871343006/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/1871343006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:51: missing javadoc https://codereview.chromium.org/1871343006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:55: javadoc https://codereview.chromium.org/1871343006/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1871343006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:44: public TextView mHeadlineTextView; I think these can also be final, right? Actually, these all seem to be used only once in onBindViewHolder now. Should we not be caching them and just doing the findViewById's inside that call?
PTAL https://codereview.chromium.org/1871343006/diff/100001/chrome/android/java/re... File chrome/android/java/res/drawable/ic_snippet_thumbnail_placeholder.xml (right): https://codereview.chromium.org/1871343006/diff/100001/chrome/android/java/re... chrome/android/java/res/drawable/ic_snippet_thumbnail_placeholder.xml:2: <!-- TODO(https://crbug.com/602627) tools:targetApi added to mute the NewApi lint error. --> On 2016/04/13 17:02:14, Yusuf wrote: > is this lint being over cautious then? That's what it seems. Android Studio had to make changes to take into account the appCompat library: go/vector-lint-error https://codereview.chromium.org/1871343006/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/1871343006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:51: On 2016/04/13 17:02:14, Yusuf wrote: > missing javadoc Done. https://codereview.chromium.org/1871343006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:55: On 2016/04/13 17:02:14, Yusuf wrote: > javadoc Done. https://codereview.chromium.org/1871343006/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1871343006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:44: public TextView mHeadlineTextView; On 2016/04/13 17:02:14, Yusuf wrote: > I think these can also be final, right? > > Actually, these all seem to be used only once in onBindViewHolder now. Should we > not be caching them and just doing the findViewById's inside that call? That's on done on purpose. We use RecyclerView and the ViewHolder pattern to reuse the views for different elements without having to run the expensive findViewById all the time: http://developer.android.com/training/improving-layouts/smooth-scrolling.html... made the fields private final.
lgtm
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, twellington@chromium.org Link to the patchset: https://codereview.chromium.org/1871343006/#ps120001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1871343006/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1871343006/120001
On 2016/04/13 23:53:18, Yusuf wrote: > lgtm Awesome, thanks all for the review!
Message was sent while issue was closed.
Description was changed from ========== [NTP Snippets] Add placeholders and start caching thumbnails Adds placeholders that are displayed while the thumbnail is being fetched from the network, and keeps the downloaded thumbnail while we stay on the NTP. BUG=601456 ========== to ========== [NTP Snippets] Add placeholders and start caching thumbnails Adds placeholders that are displayed while the thumbnail is being fetched from the network, and keeps the downloaded thumbnail while we stay on the NTP. BUG=601456 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [NTP Snippets] Add placeholders and start caching thumbnails Adds placeholders that are displayed while the thumbnail is being fetched from the network, and keeps the downloaded thumbnail while we stay on the NTP. BUG=601456 ========== to ========== [NTP Snippets] Add placeholders and start caching thumbnails Adds placeholders that are displayed while the thumbnail is being fetched from the network, and keeps the downloaded thumbnail while we stay on the NTP. BUG=601456 Committed: https://crrev.com/204cf9c4eb767c7f8cfbd85267c4b9e87b62067d Cr-Commit-Position: refs/heads/master@{#387262} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/204cf9c4eb767c7f8cfbd85267c4b9e87b62067d Cr-Commit-Position: refs/heads/master@{#387262} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
