Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2571933002:
[Android History] Add favicons to history item views (Closed)
Description[Android History] Add favicons to history item views
BUG=654071
Committed: https://crrev.com/22dc0ce8e461feeead5b37e71e0c6ea592fed37c
Cr-Commit-Position: refs/heads/master@{#438939}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Changes from dfalcantara@ review #
Total comments: 4
Patch Set 3 : [Android History] Add favicons to history item views #
Messages
Total messages: 25 (14 generated)
The CQ bit was checked by twellington@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...
twellington@chromium.org changed reviewers: + dfalcantara@chromium.org
ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2571933002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java (right): https://codereview.chromium.org/2571933002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java:34: private final RoundedIconGenerator mIconGenerator; Separate out the dimensions into another group? https://codereview.chromium.org/2571933002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java:43: mMinIconSize = (int) getResources().getDimension(R.dimen.default_favicon_min_size); getDimensionPixelSize(), or do you want 0 to be a valid value? https://codereview.chromium.org/2571933002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java:105: if (icon == null) { Does this code exist elsewhere? Seems like it's something that we'd accounted for elsewhere. https://codereview.chromium.org/2571933002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java (right): https://codereview.chromium.org/2571933002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java:90: int maxSize = Math.min(activityManager.getMemoryClass() / FOUR_MEGABYTES, Looks like getMemoryClass() returns an int of the number of MB, not B, so dividing by 4 million would always result in 0. Can you check what kinds of values you're getting for this comparison?
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
https://codereview.chromium.org/2571933002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java (right): https://codereview.chromium.org/2571933002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java:34: private final RoundedIconGenerator mIconGenerator; On 2016/12/14 00:16:25, dfalcantara (check my queue) wrote: > Separate out the dimensions into another group? Done. https://codereview.chromium.org/2571933002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java:43: mMinIconSize = (int) getResources().getDimension(R.dimen.default_favicon_min_size); On 2016/12/14 00:16:25, dfalcantara (check my queue) wrote: > getDimensionPixelSize(), or do you want 0 to be a valid value? Done. https://codereview.chromium.org/2571933002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java:105: if (icon == null) { On 2016/12/14 00:16:25, dfalcantara (check my queue) wrote: > Does this code exist elsewhere? Seems like it's something that we'd accounted > for elsewhere. Yes, it does. I added a TODO note to move this somewhere bookmarks can share. Is that okay or do you want the refactoring to happen now? https://codereview.chromium.org/2571933002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java (right): https://codereview.chromium.org/2571933002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java:90: int maxSize = Math.min(activityManager.getMemoryClass() / FOUR_MEGABYTES, On 2016/12/14 00:16:25, dfalcantara (check my queue) wrote: > Looks like getMemoryClass() returns an int of the number of MB, not B, so > dividing by 4 million would always result in 0. Can you check what kinds of > values you're getting for this comparison? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2571933002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java (right): https://codereview.chromium.org/2571933002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java:105: if (icon == null) { On 2016/12/15 20:32:13, Theresa Wellington wrote: > On 2016/12/14 00:16:25, dfalcantara (check my queue) wrote: > > Does this code exist elsewhere? Seems like it's something that we'd accounted > > for elsewhere. > > Yes, it does. I added a TODO note to move this somewhere bookmarks can share. Is > that okay or do you want the refactoring to happen now? Acknowledged. https://codereview.chromium.org/2571933002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java (right): https://codereview.chromium.org/2571933002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java:44: mMinIconSize = (int) getResources().getDimensionPixelSize(R.dimen.default_favicon_min_size); nit: don't need to cast to an int https://codereview.chromium.org/2571933002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java (right): https://codereview.chromium.org/2571933002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java:92: int maxSize = Math.min(activityManager.getMemoryClass() / 4 * MEGABYTES_TO_BYTES, nit: add some parentheses to show the order of operations more obvious
lgtm
https://codereview.chromium.org/2571933002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java (right): https://codereview.chromium.org/2571933002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java:44: mMinIconSize = (int) getResources().getDimensionPixelSize(R.dimen.default_favicon_min_size); On 2016/12/15 21:14:53, dfalcantara (check my queue) wrote: > nit: don't need to cast to an int Done. https://codereview.chromium.org/2571933002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java (right): https://codereview.chromium.org/2571933002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java:92: int maxSize = Math.min(activityManager.getMemoryClass() / 4 * MEGABYTES_TO_BYTES, On 2016/12/15 21:14:53, dfalcantara (check my queue) wrote: > nit: add some parentheses to show the order of operations more obvious Done.
The CQ bit was checked by twellington@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2571933002/#ps40001 (title: "[Android History] Add favicons to history item views")
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": 40001, "attempt_start_ts": 1481836658004650,
"parent_rev": "3ee7cbe1aa17bb8b6fad7ccda54b778c850b3839", "commit_rev":
"f8eee4dbd50d44f5629bd6e1d69636c46d1dce87"}
Message was sent while issue was closed.
Description was changed from ========== [Android History] Add favicons to history item views BUG=654071 ========== to ========== [Android History] Add favicons to history item views BUG=654071 Review-Url: https://codereview.chromium.org/2571933002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Android History] Add favicons to history item views BUG=654071 Review-Url: https://codereview.chromium.org/2571933002 ========== to ========== [Android History] Add favicons to history item views BUG=654071 Committed: https://crrev.com/22dc0ce8e461feeead5b37e71e0c6ea592fed37c Cr-Commit-Position: refs/heads/master@{#438939} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/22dc0ce8e461feeead5b37e71e0c6ea592fed37c Cr-Commit-Position: refs/heads/master@{#438939} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
