Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
997223003:
New fast NTP that uses a single iframe (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. |
DescriptionNew fast NTP that uses a single iframe
BUG=461032
Committed: https://crrev.com/a8cd0cdb5970a9aa14a5c407e05760d07f677226
Cr-Commit-Position: refs/heads/master@{#320849}
Patch Set 1 #Patch Set 2 : #
Total comments: 20
Patch Set 3 : #Patch Set 4 : #
Total comments: 42
Patch Set 5 : #
Messages
Total messages: 18 (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.
Could you set upstream of local_ntp_fast.css to local_ntp.css (e.g., change "git cl" setting) to make it easier to see diffs? Thanks! https://chromiumcodereview.appspot.com/997223003/diff/20001/chrome/browser/re... File chrome/browser/resources/local_ntp/local_ntp_fast.css (right): https://chromiumcodereview.appspot.com/997223003/diff/20001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.css:173: #mv-tiles { Remove tile-related stuff that got moved? https://chromiumcodereview.appspot.com/997223003/diff/20001/chrome/browser/re... File chrome/browser/resources/local_ntp/local_ntp_fast.html (right): https://chromiumcodereview.appspot.com/997223003/diff/20001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.html:10: Extra line? https://chromiumcodereview.appspot.com/997223003/diff/20001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.html:14: <body > Space issue. https://chromiumcodereview.appspot.com/997223003/diff/20001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.html:17: <div id="mv-tiles"> Space issue. https://chromiumcodereview.appspot.com/997223003/diff/20001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.html:23: <span id="mv-notice-links"> Indent. https://chromiumcodereview.appspot.com/997223003/diff/20001/chrome/browser/re... File chrome/browser/resources/local_ntp/local_ntp_fast.js (right): https://chromiumcodereview.appspot.com/997223003/diff/20001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:75: UNDO_LINK: 'mv-undo' Keep TILES so you can do $(IDS.TILES) instead of hard-coded? https://chromiumcodereview.appspot.com/997223003/diff/20001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:400: /** Extra /** https://chromiumcodereview.appspot.com/997223003/diff/20001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:647: // if (event.origin !== 'chrome-search://most-visited') Clean up? https://chromiumcodereview.appspot.com/997223003/diff/20001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:661: console.log('Full page render: ' + m[0].duration); Clean up? https://chromiumcodereview.appspot.com/997223003/diff/20001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:791: var args = ''; Maybe have var args = [] ... args.push('rtl=1'); ... + args.join('&');
done! :) https://codereview.chromium.org/997223003/diff/20001/chrome/browser/resources... File chrome/browser/resources/local_ntp/local_ntp_fast.css (right): https://codereview.chromium.org/997223003/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.css:173: #mv-tiles { On 2015/03/12 06:29:19, huangs wrote: > Remove tile-related stuff that got moved? Done. https://codereview.chromium.org/997223003/diff/20001/chrome/browser/resources... File chrome/browser/resources/local_ntp/local_ntp_fast.html (right): https://codereview.chromium.org/997223003/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.html:10: On 2015/03/12 06:29:19, huangs wrote: > Extra line? Done. https://codereview.chromium.org/997223003/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.html:14: <body > On 2015/03/12 06:29:19, huangs wrote: > Space issue. Done. https://codereview.chromium.org/997223003/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.html:17: <div id="mv-tiles"> On 2015/03/12 06:29:19, huangs wrote: > Space issue. Done. https://codereview.chromium.org/997223003/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.html:23: <span id="mv-notice-links"> On 2015/03/12 06:29:19, huangs wrote: > Indent. Done. https://codereview.chromium.org/997223003/diff/20001/chrome/browser/resources... File chrome/browser/resources/local_ntp/local_ntp_fast.js (right): https://codereview.chromium.org/997223003/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:75: UNDO_LINK: 'mv-undo' On 2015/03/12 06:29:19, huangs wrote: > Keep TILES so you can do > > $(IDS.TILES) > > instead of hard-coded? Done. https://codereview.chromium.org/997223003/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:400: /** On 2015/03/12 06:29:19, huangs wrote: > Extra /** Done. https://codereview.chromium.org/997223003/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:647: // if (event.origin !== 'chrome-search://most-visited') On 2015/03/12 06:29:19, huangs wrote: > Clean up? Done. https://codereview.chromium.org/997223003/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:661: console.log('Full page render: ' + m[0].duration); On 2015/03/12 06:29:19, huangs wrote: > Clean up? Is it ok if I leave this here for now until I add a proper log to capture this? I'll do it before enabling the experiment. https://codereview.chromium.org/997223003/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:791: var args = ''; On 2015/03/12 06:29:19, huangs wrote: > Maybe have > var args = [] > ... > args.push('rtl=1'); > ... > + args.join('&'); Done.
https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... File chrome/browser/resources/local_ntp/local_ntp_fast.js (right): https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:30: console.log('Full page render: ' + m[0].duration); Do you want to ship this? https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:658: console.log('Full page render: ' + m[0].duration); I don't think we should be logging to console? https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:784: extra newline
Please also test on a 2x device, e.g., Pixel. https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... File chrome/browser/resources/local_ntp/local_ntp_fast.css (right): https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.css:71: width: 298px; Can replace with box-sizing: border-box; ... width: 300px; https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.css:196: background-color: rgba(90,90,90, 1.0); NIT: inconsistent space before 1.0 . https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.css:291: border: 0; border: none; https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... File chrome/browser/resources/local_ntp/local_ntp_fast.js (right): https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:18: 'use strict'; https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:22: var timedLoadCount = 0; Can unindent by 2 space. https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:41: DARK: 'dark', DARK and DEFAULT_THEME should be injected into the single iframe (using message?). Need to watch out for possible timing-sensitive flicker on load for dark themes, potentially fix by injecting via query params. https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:44: DOT: 'dot', DOT no longer used. https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:53: RTL: 'rtl', // Right-to-left language text. Remove trailing comma (bad style for JS in general, probably because IE used to crash on this or something silly). https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:75: TILES: 'mv-tiles' Put "," back. https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:102: /** MIDDLE_MOUSE_BUTTON seems unused. https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:212: var titleColor = null; This is only needed by the single iframe. https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:237: var fakeboxText = $(IDS.FAKEBOX_TEXT); Send message to single iframe, telling it to update. Some theme-related code may need to be shared in a separate file (alternative: compute everything here and inject result to iframe, e.g., isThemeDark, which is used in both) (alternative: duplicate code). Might be a bit too much complex for this CL, so I'm also okay with TODO here. https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:249: if (!info) { titleColor is only useful in the single iframe, can move all this logic to the page when you handle themes/ https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:294: var themeStyle = This code needs to be distributed with single iframe as well. Okay with TODO. https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:320: customStyleElement = document.createElement('style'); A bit fishy that we're not searching for IDS.CUSTOM_THEME_STYLE and removing it. https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:374: function convertToRRGGBBAAColor(color) { This routine was added to talk to title.html. Can probably remove (note that it was only used for titleColor). https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:409: for (var i = 0; i < Math.min(8, pages.length); ++i) { Replace 8 with MAX_NUM_TILES_TO_SHOW. https://chromiumcodereview.appspot.com/997223003/diff/60001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_fast.js:410: iframe.postMessage({cmd: 'tile', rid: pages[i].rid}, '*'); Would it be better for performance if you combine the 8 'tile' messages into a single one, and pass array of rid?
Thanks for the reviews. I left all Theme-related comments with an ack, and I'll revisit them for the next CL. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... File chrome/browser/resources/local_ntp/local_ntp_fast.css (right): https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.css:71: width: 298px; On 2015/03/14 01:54:12, huangs wrote: > Can replace with > > box-sizing: border-box; > ... > width: 300px; Done. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.css:196: background-color: rgba(90,90,90, 1.0); On 2015/03/14 01:54:12, huangs wrote: > NIT: inconsistent space before 1.0 . Done. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.css:291: border: 0; On 2015/03/14 01:54:12, huangs wrote: > border: none; Done. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... File chrome/browser/resources/local_ntp/local_ntp_fast.js (right): https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:18: On 2015/03/14 01:54:12, huangs wrote: > 'use strict'; Done. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:22: var timedLoadCount = 0; On 2015/03/14 01:54:12, huangs wrote: > Can unindent by 2 space. Done. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:30: console.log('Full page render: ' + m[0].duration); On 2015/03/12 17:52:46, Mathieu Perreault wrote: > Do you want to ship this? removed. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:41: DARK: 'dark', On 2015/03/14 01:54:12, huangs wrote: > DARK and DEFAULT_THEME should be injected into the single iframe (using > message?). Need to watch out for possible timing-sensitive flicker on load for > dark themes, potentially fix by injecting via query params. Acknowledged. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:44: DOT: 'dot', On 2015/03/14 01:54:13, huangs wrote: > DOT no longer used. Done. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:53: RTL: 'rtl', // Right-to-left language text. On 2015/03/14 01:54:12, huangs wrote: > Remove trailing comma (bad style for JS in general, probably because IE used to > crash on this or something silly). Done. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:75: TILES: 'mv-tiles' On 2015/03/14 01:54:12, huangs wrote: > Put "," back. Done. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:102: /** On 2015/03/14 01:54:12, huangs wrote: > MIDDLE_MOUSE_BUTTON seems unused. Done. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:212: var titleColor = null; On 2015/03/14 01:54:13, huangs wrote: > This is only needed by the single iframe. Done. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:237: var fakeboxText = $(IDS.FAKEBOX_TEXT); On 2015/03/14 01:54:13, huangs wrote: > Send message to single iframe, telling it to update. > > Some theme-related code may need to be shared in a separate file (alternative: > compute everything here and inject result to iframe, e.g., isThemeDark, which > is used in both) (alternative: duplicate code). > > Might be a bit too much complex for this CL, so I'm also okay with TODO here. Acknowledged. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:249: if (!info) { On 2015/03/14 01:54:12, huangs wrote: > titleColor is only useful in the single iframe, can move all this logic to the > page when you handle themes/ Acknowledged. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:294: var themeStyle = On 2015/03/14 01:54:13, huangs wrote: > This code needs to be distributed with single iframe as well. Okay with TODO. Acknowledged. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:320: customStyleElement = document.createElement('style'); On 2015/03/14 01:54:12, huangs wrote: > A bit fishy that we're not searching for IDS.CUSTOM_THEME_STYLE and removing it. Acknowledged. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:374: function convertToRRGGBBAAColor(color) { On 2015/03/14 01:54:12, huangs wrote: > This routine was added to talk to title.html. Can probably remove (note that it > was only used for titleColor). Acknowledged. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:409: for (var i = 0; i < Math.min(8, pages.length); ++i) { On 2015/03/14 01:54:12, huangs wrote: > Replace 8 with MAX_NUM_TILES_TO_SHOW. Done. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:410: iframe.postMessage({cmd: 'tile', rid: pages[i].rid}, '*'); On 2015/03/14 01:54:12, huangs wrote: > Would it be better for performance if you combine the 8 'tile' messages into a > single one, and pass array of rid? could be, I'll try after this CL lands. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:658: console.log('Full page render: ' + m[0].duration); On 2015/03/12 17:52:46, Mathieu Perreault wrote: > I don't think we should be logging to console? Done. https://codereview.chromium.org/997223003/diff/60001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_fast.js:784: On 2015/03/12 17:52:46, Mathieu Perreault wrote: > extra newline Done.
lgtm
lgtm
fserb@chromium.org changed reviewers: + thestig@chromium.org
fserb@chromium.org changed required reviewers: + thestig@chromium.org
+thestig for chrome/browser/browser_resources.grd (last one, I promise ;) )
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/997223003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a8cd0cdb5970a9aa14a5c407e05760d07f677226 Cr-Commit-Position: refs/heads/master@{#320849} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
