Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
997203003:
Adds a new NTP endpoint for the single frame of the fast NTP (Closed)
|
Created:
5 years, 9 months ago by fserb Modified:
5 years, 9 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 a new NTP endpoint for the single frame of the fast NTP
BUG=461032
Committed: https://crrev.com/766af628b557101e9b464d4be17c1d5326708bc6
Cr-Commit-Position: refs/heads/master@{#320813}
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total comments: 69
Patch Set 4 : #
Total comments: 16
Patch Set 5 : #
Total comments: 26
Patch Set 6 : #
Messages
Total messages: 26 (5 generated)
fserb@chromium.org changed reviewers: + huangs@chromium.org, mathp@chromium.org
fserb@chromium.org changed required reviewers: + mathp@chromium.org
Issue 999153002 contains the "full version" of this change.
https://codereview.chromium.org/997203003/diff/1/chrome/browser/resources/loc... File chrome/browser/resources/local_ntp/most_visited_single.html (right): https://codereview.chromium.org/997203003/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/most_visited_single.html:10: <div id="most-visited"> NIT: indentation. https://codereview.chromium.org/997203003/diff/1/chrome/browser/resources/loc... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/997203003/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/most_visited_single.js:119: var parent = $('most-visited'); $
Done. Thanks for the review. https://codereview.chromium.org/997203003/diff/1/chrome/browser/resources/loc... File chrome/browser/resources/local_ntp/most_visited_single.html (right): https://codereview.chromium.org/997203003/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/most_visited_single.html:10: <div id="most-visited"> On 2015/03/11 18:45:32, huangs wrote: > NIT: indentation. Done. https://codereview.chromium.org/997203003/diff/1/chrome/browser/resources/loc... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/997203003/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/most_visited_single.js:119: var parent = $('most-visited'); On 2015/03/11 18:45:32, huangs wrote: > $ Done.
More comments. https://codereview.chromium.org/997203003/diff/20001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_single.css (right): https://codereview.chromium.org/997203003/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.css:40: text-align: left; Use text-align: -webkit-auto ? It should work for both LTR and RTL? https://codereview.chromium.org/997203003/diff/20001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/997203003/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:80: if (loadedCounter <= 0.0) { Why not just 0? https://codereview.chromium.org/997203003/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:126: if (ev.target != cur) return; if (ev.target === cur) { parent.removeChild(old); } https://codereview.chromium.org/997203003/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:169: tile.className += ' blacklisted'; Can use tile.classList.add('blacklisted'). Same below.
thanks :) https://codereview.chromium.org/997203003/diff/20001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_single.css (right): https://codereview.chromium.org/997203003/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.css:40: text-align: left; On 2015/03/11 19:14:57, huangs wrote: > Use text-align: -webkit-auto ? It should work for both LTR and RTL? Done. https://codereview.chromium.org/997203003/diff/20001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/997203003/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:80: if (loadedCounter <= 0.0) { On 2015/03/11 19:14:58, huangs wrote: > Why not just 0? Done. https://codereview.chromium.org/997203003/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:126: if (ev.target != cur) return; On 2015/03/11 19:14:58, huangs wrote: > if (ev.target === cur) { > parent.removeChild(old); > } Done. https://codereview.chromium.org/997203003/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:169: tile.className += ' blacklisted'; On 2015/03/11 19:14:57, huangs wrote: > Can use tile.classList.add('blacklisted'). Same below. Done.
Lots of NITs! But key points: - Please check if there are no event logs. - Verify "mouseover" vs. "mouseenter". - I don't see Window resizing stuff? https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... File chrome/browser/resources/local_ntp/most_visited_single.css (right): https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.css:48: border-color: transparent; border: 2px solid transparent; to replace 3 lines. https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.css:60: opacity: 1.0; NIT: opacity 1.0 here but 1 elsewhere. https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.css:76: .mv-tile:focus { Assume initial brightness is 100%, then you can do .mv-tile:focus:not(:hover) { -webkit-filter: brightness(75%); } then delete the stuff below. https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.css:81: -webkit-filter: brightness(100%) !important; (If you're keeping this) I don't think you need this !important. https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.css:109: linear-gradient(to right, black, black, 100px, transparent); black or #000 (your choice). https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.css:123: .mv-title { NIT: indent. https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.css:166: background-color: #FFF; NIT: #fff https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.css:229: -webkit-transition-delay: 500ms; NIT: indent. https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.css:244: html[dir=rtl] .mv-favicon { left: auto; https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:19: var LOGTYPE = { NIT: LOG_TYPE? https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:21: NTP_SERVER_SIDE_SUGGESTION: 0, The following are unused: NTP_SERVER_SIDE_SUGGESTION NTP_GRAY_TILE NTP_EXTERNAL_TILE NTP_GRAY_TILE_FALLBACK NTP_EXTERNAL_TILE_FALLBACK Please verify if they're somehow missed? Add "// Unused here." if intended. https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:35: // (if it was provided), resulting in a grey tile. NIT: grey https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:60: * @type {DOM} Note that {DOMNode} appears below, which is inconsistent. https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:65: * Log an event on the NTP NIT: "." at end. https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:66: * @param {number} eventName Event from NTP_LOGGING_EVENT_TYPE. NIT: |eventType| instead of |eventName|? Also NTP_LOGGING_EVENT_TYPE got renamed? https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:81: window.parent.postMessage({cmd: 'loaded' }, DOMAIN_ORIGIN); NIT: extra space before "}"? https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:94: var args = event.data; |args| is used only once, and in the "else" case you used |event.data| directly. https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:111: // store the tiles on the current closure. Nit: Capitalize "Store". https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:115: while (cur.childNodes.length < 8) { Magic number 8. https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:119: var parent = $('most-visited'); Why not document.querySelector('#most-visited')? Do you use anything other than '$' from utils.js? https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:176: rid: Number(tile.getAttribute('rid'))}, HTML custom attributes should have '-', so 'data-rid'? https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:198: tile.setAttribute('rid', data.rid); HTML custom attributes should have '-', so 'data-rid'? https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:205: tile.onkeypress = function(ev) { Use tile.addEventListener('keypress', function() {...} ); instead? https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:213: logEvent(LOGTYPE.NTP_MOUSEOVER); I'm a bit concerned with using mouseover; could you check to see if this fires when mouse enter/leaves child elements? If so then mouseenter would be more appropriate. https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:216: var title = tile.querySelectorAll('.mv-title')[0]; title.querySelector('.mv-title') is cleaner (takes the first). Not sure if JS would be so smart enough to NOT build the list of everything, then only extract the first element -- but we can avoid this. 3 more instances below. https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:217: title.innerHTML = data.title; Is data.title already escape entities? Otherwise prefer title.innerText = data.title; I think the code to generate data is searchbox_extension.cc: GenerateMostVisitedItem() https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:222: var img = document.createElement('img'); Move these 2 lines inside "if" since it's unused outside? Also improves symmetry compared to favicon code. https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:226: loadedCounter += 1; Nit: Inconsistent with "loadedCounter--" you used earlier. Another instance below. https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:244: fi.title = ''; Wouldn't |ti.title| be empty already? Please comment if there's some functional thing. https://chromiumcodereview.appspot.com/997203003/diff/40001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:257: mvx.onclick = function(ev) { mvx.addEventListener('click', ...) ?
https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_single.html (right): https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.html:4: <base target="_top"> curious, what does this do in this context? https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:13: * used to transfer information from the NTP javascript to the renderer and is nit: JavaScript https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:71: nit: either choose 2 lines between functions or 1 line, but be consistent in the file. I would pick 1 line. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:274: // Parse query arguments. are we only doing this for RTL? consider early break, or perhaps moving the parseQueryParameters logic into a private function.
thanks for all the comments. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_single.css (right): https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.css:48: border-color: transparent; On 2015/03/12 06:16:55, huangs wrote: > border: 2px solid transparent; > > to replace 3 lines. Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.css:60: opacity: 1.0; On 2015/03/12 06:16:55, huangs wrote: > NIT: opacity 1.0 here but 1 elsewhere. Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.css:76: .mv-tile:focus { On 2015/03/12 06:16:55, huangs wrote: > Assume initial brightness is 100%, then you can do > > .mv-tile:focus:not(:hover) { > -webkit-filter: brightness(75%); > } > > then delete the stuff below. Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.css:81: -webkit-filter: brightness(100%) !important; On 2015/03/12 06:16:55, huangs wrote: > (If you're keeping this) I don't think you need this !important. Acknowledged. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.css:109: linear-gradient(to right, black, black, 100px, transparent); On 2015/03/12 06:16:55, huangs wrote: > black or #000 (your choice). Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.css:123: .mv-title { On 2015/03/12 06:16:55, huangs wrote: > NIT: indent. Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.css:166: background-color: #FFF; On 2015/03/12 06:16:55, huangs wrote: > NIT: #fff Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.css:229: -webkit-transition-delay: 500ms; On 2015/03/12 06:16:55, huangs wrote: > NIT: indent. Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.css:244: html[dir=rtl] .mv-favicon { On 2015/03/12 06:16:55, huangs wrote: > left: auto; Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:13: * used to transfer information from the NTP javascript to the renderer and is On 2015/03/12 15:53:26, Mathieu Perreault wrote: > nit: JavaScript OMG https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:19: var LOGTYPE = { On 2015/03/12 06:16:56, huangs wrote: > NIT: LOG_TYPE? Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:21: NTP_SERVER_SIDE_SUGGESTION: 0, On 2015/03/12 06:16:56, huangs wrote: > The following are unused: > NTP_SERVER_SIDE_SUGGESTION > NTP_GRAY_TILE > NTP_EXTERNAL_TILE > NTP_GRAY_TILE_FALLBACK > NTP_EXTERNAL_TILE_FALLBACK > > Please verify if they're somehow missed? Add "// Unused here." if intended. Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:35: // (if it was provided), resulting in a grey tile. On 2015/03/12 06:16:56, huangs wrote: > NIT: grey Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:60: * @type {DOM} On 2015/03/12 06:16:56, huangs wrote: > Note that {DOMNode} appears below, which is inconsistent. Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:65: * Log an event on the NTP On 2015/03/12 06:16:56, huangs wrote: > NIT: "." at end. Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:66: * @param {number} eventName Event from NTP_LOGGING_EVENT_TYPE. On 2015/03/12 06:16:56, huangs wrote: > NIT: |eventType| instead of |eventName|? > > Also NTP_LOGGING_EVENT_TYPE got renamed? Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:71: On 2015/03/12 15:53:26, Mathieu Perreault wrote: > nit: either choose 2 lines between functions or 1 line, but be consistent in the > file. I would pick 1 line. I decided on two lines. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:81: window.parent.postMessage({cmd: 'loaded' }, DOMAIN_ORIGIN); On 2015/03/12 06:16:56, huangs wrote: > NIT: extra space before "}"? Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:94: var args = event.data; On 2015/03/12 06:16:56, huangs wrote: > |args| is used only once, and in the "else" case you used |event.data| directly. Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:111: // store the tiles on the current closure. On 2015/03/12 06:16:56, huangs wrote: > Nit: Capitalize "Store". Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:115: while (cur.childNodes.length < 8) { On 2015/03/12 06:16:56, huangs wrote: > Magic number 8. Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:119: var parent = $('most-visited'); On 2015/03/12 06:16:56, huangs wrote: > Why not > document.querySelector('#most-visited')? > > Do you use anything other than '$' from utils.js? Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:176: rid: Number(tile.getAttribute('rid'))}, On 2015/03/12 06:16:56, huangs wrote: > HTML custom attributes should have '-', so 'data-rid'? Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:198: tile.setAttribute('rid', data.rid); On 2015/03/12 06:16:56, huangs wrote: > HTML custom attributes should have '-', so 'data-rid'? Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:205: tile.onkeypress = function(ev) { On 2015/03/12 06:16:56, huangs wrote: > Use > tile.addEventListener('keypress', function() {...} ); > instead? Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:213: logEvent(LOGTYPE.NTP_MOUSEOVER); On 2015/03/12 06:16:56, huangs wrote: > I'm a bit concerned with using mouseover; could you check to see if this fires > when mouse enter/leaves child elements? If so then mouseenter would be more > appropriate. Acknowledged. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:216: var title = tile.querySelectorAll('.mv-title')[0]; On 2015/03/12 06:16:55, huangs wrote: > title.querySelector('.mv-title') is cleaner (takes the first). Not sure if JS > would be so smart enough to NOT build the list of everything, then only extract > the first element -- but we can avoid this. > > 3 more instances below. Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:217: title.innerHTML = data.title; On 2015/03/12 06:16:55, huangs wrote: > Is data.title already escape entities? Otherwise prefer > > title.innerText = data.title; > > I think the code to generate data is searchbox_extension.cc: > GenerateMostVisitedItem() Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:222: var img = document.createElement('img'); On 2015/03/12 06:16:55, huangs wrote: > Move these 2 lines inside "if" since it's unused outside? Also improves > symmetry compared to favicon code. Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:226: loadedCounter += 1; On 2015/03/12 06:16:55, huangs wrote: > Nit: Inconsistent with "loadedCounter--" you used earlier. Another instance > below. Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:244: fi.title = ''; On 2015/03/12 06:16:56, huangs wrote: > Wouldn't |ti.title| be empty already? Please comment if there's some functional > thing. Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:257: mvx.onclick = function(ev) { On 2015/03/12 06:16:55, huangs wrote: > mvx.addEventListener('click', ...) ? Done. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:274: // Parse query arguments. On 2015/03/12 15:53:26, Mathieu Perreault wrote: > are we only doing this for RTL? consider early break, or perhaps moving the > parseQueryParameters logic into a private function. We are only using it for RTL for now, but I expect to use it more for ML.
Some nits of my own! https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:244: fi.title = ''; On 2015/03/12 16:05:33, fserb wrote: > On 2015/03/12 06:16:56, huangs wrote: > > Wouldn't |ti.title| be empty already? Please comment if there's some > functional > > thing. > > Done. I don't see the results of this comment. https://codereview.chromium.org/997203003/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:274: // Parse query arguments. On 2015/03/12 16:05:34, fserb wrote: > On 2015/03/12 15:53:26, Mathieu Perreault wrote: > > are we only doing this for RTL? consider early break, or perhaps moving the > > parseQueryParameters logic into a private function. > > We are only using it for RTL for now, but I expect to use it more for ML. Then consider moving to a private function, it cleans up this function. https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:189: rid: Number(tile.getAttribute('data-rid'))}, how does this work when there is no 'rid', such as with server suggestions? Sorry if I'm not current on the design. https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:198: * data is null if you want to construct an empty tile. nit: it's indent by 4 only nit: |data| is null if... https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:202: tile.className = 'mv-tile'; nit: would move this line below the if statement. https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:214: '<div title="Don\'t show on this page" class="mv-x"></div>'; this message should be translated https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:225: tile.addEventListener('mouseover', function() { Add a todo to remove this https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:241: img.addEventListener('error', countLoad); can you combine these 'error' lines, same below https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:256: fi.src = '../' + data.faviconUrl; can you explain via comment why this is here?
thank you! https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:189: rid: Number(tile.getAttribute('data-rid'))}, On 2015/03/12 17:39:19, Mathieu Perreault wrote: > how does this work when there is no 'rid', such as with server suggestions? > Sorry if I'm not current on the design. Currently it doesn't. For server suggestions we do (and will) send an id with data that gets sent instead of the rid. In the end is the same code: rid makes sense for chrome CC code, and this other id will make sense for the server host page. https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:198: * data is null if you want to construct an empty tile. On 2015/03/12 17:39:19, Mathieu Perreault wrote: > nit: it's indent by 4 only > nit: |data| is null if... Done. https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:202: tile.className = 'mv-tile'; On 2015/03/12 17:39:19, Mathieu Perreault wrote: > nit: would move this line below the if statement. Done. https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:214: '<div title="Don\'t show on this page" class="mv-x"></div>'; On 2015/03/12 17:39:19, Mathieu Perreault wrote: > this message should be translated good catch! Done. https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:225: tile.addEventListener('mouseover', function() { On 2015/03/12 17:39:19, Mathieu Perreault wrote: > Add a todo to remove this Done. https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:225: tile.addEventListener('mouseover', function() { On 2015/03/12 17:39:19, Mathieu Perreault wrote: > Add a todo to remove this Done. https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:241: img.addEventListener('error', countLoad); On 2015/03/12 17:39:19, Mathieu Perreault wrote: > can you combine these 'error' lines, same below I could, but I'd like to keep it separate. The performance test and the actual error handling.
Almost done! https://chromiumcodereview.appspot.com/997203003/diff/80001/chrome/browser/re... File chrome/browser/resources/local_ntp/most_visited_single.css (right): https://chromiumcodereview.appspot.com/997203003/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.css:56: maring: 0 8px; TYPO: maring https://chromiumcodereview.appspot.com/997203003/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.css:167: content: ''; Short comment on why content is here (your cool trick :). https://chromiumcodereview.appspot.com/997203003/diff/80001/chrome/browser/re... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://chromiumcodereview.appspot.com/997203003/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:42: // The user moused over an NTP tile or title. Can remove "or title". https://chromiumcodereview.appspot.com/997203003/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:79: * @param {number} eventType Event from NTP_LOGGING_EVENT_TYPE. NTP_LOGGING_EVENT_TYPE => LOG_TYPE https://chromiumcodereview.appspot.com/997203003/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:87: * Down count the DOM elements that we are waiting for the page to load. NIT: "Down counts" -- if a function comment starts with verb, there's an implicit "This function..". https://chromiumcodereview.appspot.com/997203003/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:101: * Handle postMessages coming from the host page to the iframe. NIT: Handles https://chromiumcodereview.appspot.com/997203003/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:150: setTimeout(function() { Would requestAnimationFrame() do the same thing? https://chromiumcodereview.appspot.com/997203003/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:171: tiles.appendChild(renderTile(null)); Would you need to log NTP_SERVER_SIDE_SUGGESTION ? https://chromiumcodereview.appspot.com/997203003/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:235: var thumb = tile.querySelectorAll('.mv-thumb')[0]; querySelector https://chromiumcodereview.appspot.com/997203003/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:236: NIT: Don't need this new line. https://chromiumcodereview.appspot.com/997203003/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:255: var favicon = tile.querySelectorAll('.mv-favicon')[0]; querySelector https://chromiumcodereview.appspot.com/997203003/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:259: // We set the title to empty, so it doesn't say the image name on chromevox. NIT: // Set title to empty so screen readers won't say image name. https://chromiumcodereview.appspot.com/997203003/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/most_visited_single.js:272: var mvx = tile.querySelectorAll('.mv-x')[0]; querySelector
https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:256: fi.src = '../' + data.faviconUrl; On 2015/03/12 17:39:19, Mathieu Perreault wrote: > can you explain via comment why this is here? Done. https://codereview.chromium.org/997203003/diff/80001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_single.css (right): https://codereview.chromium.org/997203003/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.css:56: maring: 0 8px; On 2015/03/13 03:49:52, huangs wrote: > TYPO: maring ops. Done. https://codereview.chromium.org/997203003/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.css:167: content: ''; On 2015/03/13 03:49:52, huangs wrote: > Short comment on why content is here (your cool trick :). Done. https://codereview.chromium.org/997203003/diff/80001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/997203003/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:42: // The user moused over an NTP tile or title. On 2015/03/13 03:49:52, huangs wrote: > Can remove "or title". Done. https://codereview.chromium.org/997203003/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:79: * @param {number} eventType Event from NTP_LOGGING_EVENT_TYPE. On 2015/03/13 03:49:52, huangs wrote: > NTP_LOGGING_EVENT_TYPE => LOG_TYPE Done. https://codereview.chromium.org/997203003/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:87: * Down count the DOM elements that we are waiting for the page to load. On 2015/03/13 03:49:52, huangs wrote: > NIT: "Down counts" -- if a function comment starts with verb, there's an > implicit "This function..". Done. https://codereview.chromium.org/997203003/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:101: * Handle postMessages coming from the host page to the iframe. On 2015/03/13 03:49:52, huangs wrote: > NIT: Handles Done. https://codereview.chromium.org/997203003/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:150: setTimeout(function() { On 2015/03/13 03:49:52, huangs wrote: > Would requestAnimationFrame() do the same thing? not the same thing. https://codereview.chromium.org/997203003/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:171: tiles.appendChild(renderTile(null)); On 2015/03/13 03:49:52, huangs wrote: > Would you need to log NTP_SERVER_SIDE_SUGGESTION ? When I handle it, yes. https://codereview.chromium.org/997203003/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:235: var thumb = tile.querySelectorAll('.mv-thumb')[0]; On 2015/03/13 03:49:52, huangs wrote: > querySelector Done. https://codereview.chromium.org/997203003/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:236: On 2015/03/13 03:49:52, huangs wrote: > NIT: Don't need this new line. Done. https://codereview.chromium.org/997203003/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:255: var favicon = tile.querySelectorAll('.mv-favicon')[0]; On 2015/03/13 03:49:52, huangs wrote: > querySelector Done. https://codereview.chromium.org/997203003/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:259: // We set the title to empty, so it doesn't say the image name on chromevox. On 2015/03/13 03:49:52, huangs wrote: > NIT: // Set title to empty so screen readers won't say image name. Done. https://codereview.chromium.org/997203003/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:272: var mvx = tile.querySelectorAll('.mv-x')[0]; On 2015/03/13 03:49:52, huangs wrote: > querySelector Done.
Should be good to go, unless you want to handle theme issues as well (need to coordinate with the fast_ntp change). I'm okay with deferring that.
lgtm, please wait on sam's approval as well. I've seen so many subtle NTP bugs, let's hope we don't regress on a few of them. Perhaps a good strategy would be that once the whole single iframe goes to Canary, we and/or the test team have a session where we try all sorts of corner cases, as well as look at earlier bugs that were fixed in the past few months. Overall though I'm very excited for this. https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_single.js:256: fi.src = '../' + data.faviconUrl; On 2015/03/13 17:22:44, fserb wrote: > On 2015/03/12 17:39:19, Mathieu Perreault wrote: > > can you explain via comment why this is here? > > Done. I meant the src line... It's not immediately clear why the '../'
On 2015/03/14 13:15:58, Mathieu Perreault wrote: > lgtm, please wait on sam's approval as well. > > I've seen so many subtle NTP bugs, let's hope we don't regress on a few of them. > > > Perhaps a good strategy would be that once the whole single iframe goes to > Canary, we and/or the test team have a session where we try all sorts of corner > cases, as well as look at earlier bugs that were fixed in the past few months. > Overall though I'm very excited for this. > > https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... > File chrome/browser/resources/local_ntp/most_visited_single.js (right): > > https://codereview.chromium.org/997203003/diff/60001/chrome/browser/resources... > chrome/browser/resources/local_ntp/most_visited_single.js:256: fi.src = '../' + > data.faviconUrl; > On 2015/03/13 17:22:44, fserb wrote: > > On 2015/03/12 17:39:19, Mathieu Perreault wrote: > > > can you explain via comment why this is here? > > > > Done. > > I meant the src line... It's not immediately clear why the '../' I thought they were specified from the mainpage, fixed.
Cool stuff! LGTM.
fserb@chromium.org changed reviewers: + thestig@chromium.org
fserb@chromium.org changed required reviewers: + thestig@chromium.org
Add thestig@ for chrome/browser/browser_resources.grd
lgtm
The CQ bit was checked by fserb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/997203003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/766af628b557101e9b464d4be17c1d5326708bc6 Cr-Commit-Position: refs/heads/master@{#320813} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
