Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1129383003:
Adds support for multiple thumbnail files on Single iframe NTP, fallback to the best one that opens. (Closed)
|
Created:
5 years, 7 months ago by fserb Modified:
5 years, 7 months ago CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, jfweitz+watch_chromium.org, David Black, arv+watch_chromium.org, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds support for multiple thumbnail files on Single iframe NTP, fallback to the best one that opens.
BUG=486879
Committed: https://crrev.com/bbdef2a03b74719929213b97f032077374806eca
Cr-Commit-Position: refs/heads/master@{#329261}
Patch Set 1 #
Total comments: 20
Patch Set 2 : #
Total comments: 12
Messages
Total messages: 15 (4 generated)
fserb@chromium.org changed reviewers: + huangs@chromium.org, mathp@chromium.org
fserb@chromium.org changed reviewers: + beaudoin@chromium.org
lgtm, mostly comments about lack of comments :) https://codereview.chromium.org/1129383003/diff/1/chrome/browser/resources/lo... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/1129383003/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:385: nit: extra newline https://codereview.chromium.org/1129383003/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:388: var results = []; Add a comment like: // Used to keep track of loading outcomes for possible thumbnails for this tile. Decision is taken in loadBestImage. https://codereview.chromium.org/1129383003/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:419: var images = []; remove https://codereview.chromium.org/1129383003/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:421: for (var t = 0; t < data.thumbnailUrl.length; ++t) { could you add a comment above saying that the best thumbnail should be at the beginning of the list?
https://codereview.chromium.org/1129383003/diff/1/chrome/browser/resources/lo... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/1129383003/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:424: i.src = data.thumbnailUrl[t]; does it completely poop out if data.thumbnailUrl is null/undefined?
Comments on variable names. https://chromiumcodereview.appspot.com/1129383003/diff/1/chrome/browser/resou... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://chromiumcodereview.appspot.com/1129383003/diff/1/chrome/browser/resou... chrome/browser/resources/local_ntp/most_visited_single.js:281: data.thumbnailUrl = [data.thumbnailUrl]; This is hacky, please have: data.thumbnailUrlList = [data.thumbnailUrl]; and maybe data.thumbnailUrl = null to prevent usage. https://chromiumcodereview.appspot.com/1129383003/diff/1/chrome/browser/resou... chrome/browser/resources/local_ntp/most_visited_single.js:391: if (loaded) return; Move return to next line. https://chromiumcodereview.appspot.com/1129383003/diff/1/chrome/browser/resou... chrome/browser/resources/local_ntp/most_visited_single.js:393: if (results[i] === null) return; Move return to next line. https://chromiumcodereview.appspot.com/1129383003/diff/1/chrome/browser/resou... chrome/browser/resources/local_ntp/most_visited_single.js:412: var rejectImage = function(i) { This |i| is a number. https://chromiumcodereview.appspot.com/1129383003/diff/1/chrome/browser/resou... chrome/browser/resources/local_ntp/most_visited_single.js:423: var i = new Image(); This |i| is an Image, and |t| is now the index. Please make naming consistent.
https://codereview.chromium.org/1129383003/diff/1/chrome/browser/resources/lo... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/1129383003/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:281: data.thumbnailUrl = [data.thumbnailUrl]; On 2015/05/11 20:31:16, huangs wrote: > This is hacky, please have: > data.thumbnailUrlList = [data.thumbnailUrl]; > and maybe > data.thumbnailUrl = null > to prevent usage. Done. https://codereview.chromium.org/1129383003/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:385: On 2015/05/11 20:27:24, Mathieu Perreault wrote: > nit: extra newline Done. https://codereview.chromium.org/1129383003/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:388: var results = []; On 2015/05/11 20:27:24, Mathieu Perreault wrote: > Add a comment like: // Used to keep track of loading outcomes for possible > thumbnails for this tile. Decision is taken in loadBestImage. Done. https://codereview.chromium.org/1129383003/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:391: if (loaded) return; On 2015/05/11 20:31:16, huangs wrote: > Move return to next line. Done. https://codereview.chromium.org/1129383003/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:393: if (results[i] === null) return; On 2015/05/11 20:31:16, huangs wrote: > Move return to next line. Done. https://codereview.chromium.org/1129383003/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:412: var rejectImage = function(i) { On 2015/05/11 20:31:16, huangs wrote: > This |i| is a number. Done. https://codereview.chromium.org/1129383003/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:419: var images = []; On 2015/05/11 20:27:24, Mathieu Perreault wrote: > remove Done. https://codereview.chromium.org/1129383003/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:421: for (var t = 0; t < data.thumbnailUrl.length; ++t) { On 2015/05/11 20:27:24, Mathieu Perreault wrote: > could you add a comment above saying that the best thumbnail should be at the > beginning of the list? Done. https://codereview.chromium.org/1129383003/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:423: var i = new Image(); On 2015/05/11 20:31:16, huangs wrote: > This |i| is an Image, and |t| is now the index. Please make naming consistent. Done. https://codereview.chromium.org/1129383003/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:424: i.src = data.thumbnailUrl[t]; On 2015/05/11 20:28:58, Mathieu Perreault wrote: > does it completely poop out if data.thumbnailUrl is null/undefined? it does get an onerror for undefined. But I added extra checks.
Questions about the flow. https://chromiumcodereview.appspot.com/1129383003/diff/20001/chrome/browser/r... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://chromiumcodereview.appspot.com/1129383003/diff/20001/chrome/browser/r... chrome/browser/resources/local_ntp/most_visited_single.js:372: img.addEventListener('error', countLoad); Remove this, and just call countLoad() from within the handler? https://chromiumcodereview.appspot.com/1129383003/diff/20001/chrome/browser/r... chrome/browser/resources/local_ntp/most_visited_single.js:397: var loadBestImage = function() { It seems the first image loaded will just dominate, so if there are multiple successes then we'd have a race condition? https://chromiumcodereview.appspot.com/1129383003/diff/20001/chrome/browser/r... chrome/browser/resources/local_ntp/most_visited_single.js:450: img.addEventListener('error', countLoad); Remove this, and just call countLoad() from within the handler?
More comments. https://chromiumcodereview.appspot.com/1129383003/diff/20001/chrome/browser/r... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://chromiumcodereview.appspot.com/1129383003/diff/20001/chrome/browser/r... chrome/browser/resources/local_ntp/most_visited_single.js:278: if (args.rid) { Should there be some code to allow server NTP to populate more items into data.thumbnailUrls? https://chromiumcodereview.appspot.com/1129383003/diff/20001/chrome/browser/r... chrome/browser/resources/local_ntp/most_visited_single.js:397: var loadBestImage = function() { Ah NVM; the "null" ensures first best image gets loaded. https://chromiumcodereview.appspot.com/1129383003/diff/20001/chrome/browser/r... chrome/browser/resources/local_ntp/most_visited_single.js:416: var acceptImage = function(idx, url) { I'd suggest removing acceptImage() and rejectImage(), but have var loadBestImage = function(idx, value) { results[idx] = value; ... }; then later, have image.onload = loadBestImage.bind(loadBestImage, i, data.thumbnailUrls[i]); image.onerror = loadBestImage.bind(loadBestImage, i, null); and for the "else" case have loadBestImage(i, null);
LGTM.
The CQ bit was checked by fserb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org Link to the patchset: https://codereview.chromium.org/1129383003/#ps20001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129383003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/bbdef2a03b74719929213b97f032077374806eca Cr-Commit-Position: refs/heads/master@{#329261}
Message was sent while issue was closed.
https://codereview.chromium.org/1129383003/diff/20001/chrome/browser/resource... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/1129383003/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_single.js:278: if (args.rid) { On 2015/05/11 21:27:11, huangs wrote: > Should there be some code to allow server NTP to populate more items into > data.thumbnailUrls? Acknowledged. https://codereview.chromium.org/1129383003/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_single.js:372: img.addEventListener('error', countLoad); On 2015/05/11 21:07:30, huangs wrote: > Remove this, and just call countLoad() from within the handler? Acknowledged. https://codereview.chromium.org/1129383003/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_single.js:397: var loadBestImage = function() { On 2015/05/11 21:07:30, huangs wrote: > It seems the first image loaded will just dominate, so if there are multiple > successes then we'd have a race condition? Acknowledged. https://codereview.chromium.org/1129383003/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_single.js:397: var loadBestImage = function() { On 2015/05/11 21:27:11, huangs wrote: > Ah NVM; the "null" ensures first best image gets loaded. Acknowledged. https://codereview.chromium.org/1129383003/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_single.js:416: var acceptImage = function(idx, url) { On 2015/05/11 21:27:11, huangs wrote: > I'd suggest removing acceptImage() and rejectImage(), but have > > var loadBestImage = function(idx, value) { > results[idx] = value; > ... > }; > > then later, have > > image.onload = loadBestImage.bind(loadBestImage, i, data.thumbnailUrls[i]); > image.onerror = loadBestImage.bind(loadBestImage, i, null); > > and for the "else" case have > loadBestImage(i, null); Done. https://codereview.chromium.org/1129383003/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_single.js:450: img.addEventListener('error', countLoad); On 2015/05/11 21:07:30, huangs wrote: > Remove this, and just call countLoad() from within the handler? Acknowledged. |
