Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
412073002:
Local NTP: prevent tiles from reloading on resize by moving them into single <div>. (Closed)
|
Created:
6 years, 5 months ago by huangs Modified:
6 years, 4 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, dominich, jfweitz+watch_chromium.org, David Black, arv+watch_chromium.org, samarth+watch_chromium.org, kmadhusu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionLocal NTP: prevent tiles from reloading on resize by moving them into single <div>.
Tiles in chrome-search://local-ntp/local-ntp.html used to be arranged in
2 rows, and get reshuffled when screen resize causes number of columns
to change. However, detaching and reattaching elements with <iframe>s
cause the <iframe>s to reload, resulting in flickering.
This CL places all tiles in a single <div>, so no reshuffling is needed.
Instead, if number of columns change we resize the container width (and
also hide tiles beyond row 2), and let HTML layout place handle the
proper wrapping.
On blacklisting, we cannot compare identities between old tiles and new,
so a reload is necessary, which leads to some flickering.
Also tested for <body dir="RTL">
Additional cleanups:
- To show all tiles only after everything is loaded: using a Barrier
counter instead of looping on every load, which was O(n^2).
- Broke apart onMostVisitedChange() and added more comments.
- Removed unused CSS for fakebox and tiles resizing (JS does resizing now).
- Using CSS visibility instead of hidden to show #mv-tiles, so it take
up space and prevent content beneath tiles from jumping up briefly.
- Refactoring the logic to hide tiles during load and show all at once when
everything loads (or timeout occurs). Using new class Barrier to do this.
BUG=399388
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287120
Patch Set 1 #
Total comments: 24
Patch Set 2 : Using Barrier; cleaned up onResize(); comments. #
Total comments: 2
Patch Set 3 : Switching back to userInitiatedMostVisitedChange; hiding tiles using visibility. #
Total comments: 16
Patch Set 4 : Adding local_ntp_utils.js and moving Barrier there. #
Total comments: 29
Patch Set 5 : Fixing comments; improving renderTiles(). #
Total comments: 8
Patch Set 6 : Fixing comments. #
Total comments: 10
Patch Set 7 : Moving tile visibility logic to renderAndShowTiles(), using per-session Barrier. #
Total comments: 4
Patch Set 8 : Comment fixes. #
Messages
Total messages: 33 (0 generated)
Starting my single-<div> cleanup on local NTP first. PTAL.
+mathp@
First pass https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:449: remove extra new line https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:506: * Changes the visibility counter for most visited tiles by |delta|, and *Most Visited https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:508: * otherwise. fix comment https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:511: function deltaMostVisitedVisibility(delta) { As discussed offline, reconsider if this function is needed. https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:737: // TODO(jeremycho): Delete once the root cause of crbug/240510 is resolved. this bug as been marked as fixed, I wonder if this is still needed?
https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... File chrome/browser/resources/local_ntp/local_ntp.css (right): https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.css:148: width: 478px; Did you really want to change the size of the fakebox? https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.css:157: width: 638px; Ditto. https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:210: var tileVisibilityCounter = 0; I'd prefer numHiddenTiles with a matching comment. https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:471: deltaMostVisitedVisibility(1); I'd use just a ++ here and call the other decrementNumberOfVisibleTiles(); https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:590: deltaMostVisitedVisibility(1); ++ https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:593: deltaMostVisitedVisibility(-1); decrementNumberOfVisibleTiles(); https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:607: deltaMostVisitedVisibility(1); Same.
Did more cleanup: - Removed "@media only" CSS, which are not used. - Added Barrier class to handle counter and showing tiles on refresh. - Updated onResize() to compute based on constants instead of hard-coded values. - Added more comments. PTAL. https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... File chrome/browser/resources/local_ntp/local_ntp.css (right): https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.css:148: width: 478px; This whole section is redundant, since we use JS to resize. Removed. https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.css:157: width: 638px; On 2014/07/24 15:24:36, beaudoin wrote: > Ditto. Acknowledged. https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:210: var tileVisibilityCounter = 0; Per discussion, numHiddenTiles will be off by one because we need to increment before creating tiles to guard against race condition. Using Barrier to do this. https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:449: On 2014/07/24 15:11:25, Mathieu Perreault wrote: > remove extra new line Done. https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:471: deltaMostVisitedVisibility(1); Changed to Barrier. https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:506: * Changes the visibility counter for most visited tiles by |delta|, and On 2014/07/24 15:11:25, Mathieu Perreault wrote: > *Most Visited Done. https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:508: * otherwise. On 2014/07/24 15:11:25, Mathieu Perreault wrote: > fix comment Done. https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:511: function deltaMostVisitedVisibility(delta) { This function is needed: in onMostVisitedChange(), the container gets hidden for 0.5s before being shown. This flow preempts the wait when all the tiles have been loaded. Updating comment. https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:590: deltaMostVisitedVisibility(1); On 2014/07/24 15:24:36, beaudoin wrote: > ++ Acknowledged. https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:593: deltaMostVisitedVisibility(-1); On 2014/07/24 15:24:36, beaudoin wrote: > decrementNumberOfVisibleTiles(); Acknowledged. https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:607: deltaMostVisitedVisibility(1); On 2014/07/24 15:24:37, beaudoin wrote: > Same. Acknowledged. https://codereview.chromium.org/412073002/diff/1/chrome/browser/resources/loc... chrome/browser/resources/local_ntp/local_ntp.js:737: // TODO(jeremycho): Delete once the root cause of crbug/240510 is resolved. Updated the routine and added comments.
LGTM with one comment to fix. https://codereview.chromium.org/412073002/diff/20001/chrome/browser/resources... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/412073002/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:524: userInitiatedCounter = Math.max(0, userInitiatedCounter - 1); The guarantee that this is called one for every userInitiatedCounter++ is a bit weak. I would stick with true/false here.
Updated. Ready for OWNER review from jered@. https://codereview.chromium.org/412073002/diff/20001/chrome/browser/resources... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/412073002/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:524: userInitiatedCounter = Math.max(0, userInitiatedCounter - 1); On 2014/07/24 22:09:15, beaudoin wrote: > The guarantee that this is called one for every userInitiatedCounter++ is a bit > weak. I would stick with true/false here. Done.
Some comments https://codereview.chromium.org/412073002/diff/40001/chrome/browser/resources... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/412073002/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:171: * A flag to indicate Most Visit changed cuased by user action. If true, then *caused https://codereview.chromium.org/412073002/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:171: * A flag to indicate Most Visit changed cuased by user action. If true, then nit: extra space, and correct to "Most Visited" https://codereview.chromium.org/412073002/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:328: function Barrier(callback) { Could be in a util file included at the top of this file. Keyword: "could" https://codereview.chromium.org/412073002/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:620: tileVisibilityBarrier.lock(); I don't really like "lock". Wouldn't add, acquire or retain be better? https://codereview.chromium.org/412073002/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:742: * response resizing. Also shows or hides extra tiles beyond the bottommost row. response *to resizing https://codereview.chromium.org/412073002/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:742: * response resizing. Also shows or hides extra tiles beyond the bottommost row. instead of "beyond the bottommost row", how about "Also shows or hides tiles according to the new width of the page". https://codereview.chromium.org/412073002/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:747: // Each tile is has left and right margins that sum to TILE_MARGIN. Each tile *has https://codereview.chromium.org/412073002/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:762: // Not using .hidden becomes it does not work for inline-block elements. *because
Updated https://codereview.chromium.org/412073002/diff/40001/chrome/browser/resources... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/412073002/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:171: * A flag to indicate Most Visit changed cuased by user action. If true, then On 2014/07/25 00:19:17, Mathieu Perreault wrote: > nit: extra space, and correct to "Most Visited" Done. https://codereview.chromium.org/412073002/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:171: * A flag to indicate Most Visit changed cuased by user action. If true, then On 2014/07/25 00:19:17, Mathieu Perreault wrote: > *caused Done. https://codereview.chromium.org/412073002/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:328: function Barrier(callback) { I think the change is short enough to not require including a new file. Also, I think we should use Promise instead eventually. If we really have to do this, maybe in a refactoring CL? https://codereview.chromium.org/412073002/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:620: tileVisibilityBarrier.lock(); Renamed to add() and remove(). https://codereview.chromium.org/412073002/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:742: * response resizing. Also shows or hides extra tiles beyond the bottommost row. On 2014/07/25 00:19:17, Mathieu Perreault wrote: > instead of "beyond the bottommost row", how about "Also shows or hides tiles > according to the new width of the page". Done. https://codereview.chromium.org/412073002/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:742: * response resizing. Also shows or hides extra tiles beyond the bottommost row. On 2014/07/25 00:19:17, Mathieu Perreault wrote: > response *to resizing Done. https://codereview.chromium.org/412073002/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:747: // Each tile is has left and right margins that sum to TILE_MARGIN. On 2014/07/25 00:19:17, Mathieu Perreault wrote: > Each tile *has Done. https://codereview.chromium.org/412073002/diff/40001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:762: // Not using .hidden becomes it does not work for inline-block elements. On 2014/07/25 00:19:17, Mathieu Perreault wrote: > *because Done.
OWNER review to jhawkins@. Thanks!
On 2014/07/25 20:15:12, huangs1 wrote: > OWNER review to jhawkins@. Thanks! Which files?
All files if possible (jered@ haven't replied yet), but especially chrome/browser/browser_resources.grd Thanks!
On 2014/07/25 23:40:39, huangs1 wrote: > All files if possible (jered@ haven't replied yet), but especially > chrome/browser/browser_resources.grd > Thanks! I'm very busy but intend to review this CL. It might need to wait until Monday. Sorry for the delay.
Please take your time, as I'm taking Monday off. Thanks!
My concerns are addressed :) lgtm
In the future, please break up CLs into independent chunks (e.g., all of the little clean-ups should be separate CLs). https://codereview.chromium.org/412073002/diff/80001/chrome/browser/resources... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/412073002/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:173: * in onMostVisitedChange() tiles remain visible, to avoid flickering. Optional nit: I believe the comma on this line is not necessary (and makes the sentence confusing). https://codereview.chromium.org/412073002/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:483: tilesContainer.style.visibility = 'hidden'; Why are you not using tilesContainer.hidden? https://codereview.chromium.org/412073002/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:582: // Keep this id here. See comment above. s/id/ID/ https://codereview.chromium.org/412073002/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:599: // Keep this id here. See comment above. s/id/ID/ https://codereview.chromium.org/412073002/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp.js:729: for (var i = 0; i < renderedList.length; ++i) { nit: No braces for single line blocks. https://codereview.chromium.org/412073002/diff/80001/chrome/browser/resources... File chrome/browser/resources/local_ntp/local_ntp_util.js (right): https://codereview.chromium.org/412073002/diff/80001/chrome/browser/resources... chrome/browser/resources/local_ntp/local_ntp_util.js:12: * A counter with a callback that gets executed on the 1-to-0 transition. Please add a test for this.
https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... File chrome/browser/resources/local_ntp/local_ntp.css (right): https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.css:54: #fakebox { Do we now need an extra 10px of margins here, too? https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.js:493: // Increment barrier to guard against race conditions. What is the race here? I think this add() and its matching remove() share the same stack in a single thread of control. https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.js:507: removeChildren(tilesContainer); nit: remove this function and just inline tilesContainer.innerHTML = ''; https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.js:509: var size = Math.min(tiles.length, numColumnsShown * NUM_ROWS); Define a function to compute this expression and reuse it where it's needed. https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.js:510: for (var i = 0; i < size; ++i) { nit: omit braces https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.js:715: var availableWidth = innerWidth + TILE_MARGIN - MIN_TOTAL_HORIZONTAL_PADDING; Why do we have an extra TILE_MARGIN px available? I think we just have the window width minus the min padding. https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.js:717: newNumColumns = nit: I think this expression would be clearer if written if (newNumColumns < MIN_NUM_COLUMNS) newNumColumns = MIN_NUM_COLUMNS; if (newNumColumns > MAX_NUM_COLUMNS) newNumColumns = MAX_NUM_COLUMNS. https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... File chrome/browser/resources/local_ntp/local_ntp_util.js (right): https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_util.js:14: * @param {Function} callback The callback to be executed. nit: !Function or function() here and below
Found and fixed bug where if you start from small window, on resize no new tiles get rendered. The fix involved more canges to renderTiles(). PTAL. https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... File chrome/browser/resources/local_ntp/local_ntp.css (right): https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.css:54: #fakebox { The extra 10px is needed in #mv-tiles to accommodate the extra margin of each tile. It is not needed by #fakebox https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.js:173: * in onMostVisitedChange() tiles remain visible, to avoid flickering. On 2014/07/28 17:43:35, James Hawkins wrote: > Optional nit: I believe the comma on this line is not necessary (and makes the > sentence confusing). Done, with slight rephrasing. https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.js:483: tilesContainer.style.visibility = 'hidden'; visibility='hidden' makes #mv-tiles disappear but still taking space. This prevents any content after the element (e.g., blacklist undo) from jumping up when #mv-tile is hidden. Updated comment. https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.js:493: // Increment barrier to guard against race conditions. Indeed this is not needed. I'm being pedantic w.r.t. how Barrier should be implemented if we have a multi-threaded environment, where multiple 1-0 transitions can occur while we loop over stuff. An alternative is to do all the add() beforehand. Problem is that in createTile() a tile may be skipped. Alternatives to address this are: (1) doing a better count outside: but this duplicate logic and break encapsulation; (2) remove() if tile is not generated, but this would makes code fragile. Therefore I'd like to keep add()-remove() pairs close to each other. Meanwhile, removed the useless initial increment and added comments. https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.js:507: removeChildren(tilesContainer); On 2014/07/28 18:32:50, Jered wrote: > nit: remove this function and just inline tilesContainer.innerHTML = ''; Done. https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.js:509: var size = Math.min(tiles.length, numColumnsShown * NUM_ROWS); The other usage was in onResize(). My previous code had the bug where if you start from few tiles and resize to get more, no new tiles appear. Therefore I'm making renderTiles() do more -- not just add tile to DOM, but also controlling their visibility. This then makes it option to delete children, so added parameters. https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.js:510: for (var i = 0; i < size; ++i) { On 2014/07/28 18:32:50, Jered wrote: > nit: omit braces Done. https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.js:582: // Keep this id here. See comment above. On 2014/07/28 17:43:34, James Hawkins wrote: > s/id/ID/ Done. https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.js:599: // Keep this id here. See comment above. On 2014/07/28 17:43:34, James Hawkins wrote: > s/id/ID/ Done. https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.js:715: var availableWidth = innerWidth + TILE_MARGIN - MIN_TOTAL_HORIZONTAL_PADDING; We want to add a bit more width to account for extra 10px + 10px margin of #mv-tiles This also preserves old behavior: we have MIN_TOTAL_HORIZONTAL_PADDING = 200 TILE_MARGIN = 20 tileRequiredWidth = 140 + 20 + 160 Now suppose innerWidth = 820, then 820 + 20 - 200 = 640 640 / 160 = 4 We also get 660 --> 3. https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.js:717: newNumColumns = Done, using "else if" for the ">" check. https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp.js:729: for (var i = 0; i < renderedList.length; ++i) { Done, though functionality moved to renderTiles(). https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... File chrome/browser/resources/local_ntp/local_ntp_util.js (right): https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_util.js:12: * A counter with a callback that gets executed on the 1-to-0 transition. Maybe in a different CL? LocalNTPJavascriptTest in \chrome\browser\ui\search\local_ntp_browsertest.cc has been disabled for nearly a year, according to crbug.com/267117 . https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_util.js:14: * @param {Function} callback The callback to be executed. Done, using {function()}
nits https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... File chrome/browser/resources/local_ntp/local_ntp_util.js (right): https://chromiumcodereview.appspot.com/412073002/diff/80001/chrome/browser/re... chrome/browser/resources/local_ntp/local_ntp_util.js:12: * A counter with a callback that gets executed on the 1-to-0 transition. On 2014/07/29 19:43:45, huangs1 wrote: > Maybe in a different CL? LocalNTPJavascriptTest in > \chrome\browser\ui\search\local_ntp_browsertest.cc has been disabled for nearly > a year, according to crbug.com/267117 . Assigned the crbug to you. Thanks! :) https://chromiumcodereview.appspot.com/412073002/diff/100001/chrome/browser/r... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://chromiumcodereview.appspot.com/412073002/diff/100001/chrome/browser/r... chrome/browser/resources/local_ntp/local_ntp.js:481: // Temporarily titleContainer invisible, but still taking up space. We make *Temporarily mark titleContainer as invisible, but it is still taking up space. https://chromiumcodereview.appspot.com/412073002/diff/100001/chrome/browser/r... chrome/browser/resources/local_ntp/local_ntp.js:514: // that sends bogus impression stats. However, if a tile becomes invisible *that create meaningless metrics. https://chromiumcodereview.appspot.com/412073002/diff/100001/chrome/browser/r... chrome/browser/resources/local_ntp/local_ntp.js:525: for (; i < numExisting; ++i) Add a comment above this: "If |numDesired| is smaller than |numExisting|, this means extra tiles are to be hidden (window is downsizing)". https://chromiumcodereview.appspot.com/412073002/diff/100001/chrome/browser/r... chrome/browser/resources/local_ntp/local_ntp.js:734: newNumColumns = MAX_NUM_COLUMNS; nit: newline below this for readability
Oops forgot to mail out. Ping jered@ for more feedback. https://chromiumcodereview.appspot.com/412073002/diff/100001/chrome/browser/r... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://chromiumcodereview.appspot.com/412073002/diff/100001/chrome/browser/r... chrome/browser/resources/local_ntp/local_ntp.js:481: // Temporarily titleContainer invisible, but still taking up space. We make On 2014/07/29 21:36:08, Mathieu Perreault wrote: > *Temporarily mark titleContainer as invisible, but it is still taking up space. Done. https://chromiumcodereview.appspot.com/412073002/diff/100001/chrome/browser/r... chrome/browser/resources/local_ntp/local_ntp.js:514: // that sends bogus impression stats. However, if a tile becomes invisible On 2014/07/29 21:36:08, Mathieu Perreault wrote: > *that create meaningless metrics. Done. https://chromiumcodereview.appspot.com/412073002/diff/100001/chrome/browser/r... chrome/browser/resources/local_ntp/local_ntp.js:525: for (; i < numExisting; ++i) On 2014/07/29 21:36:08, Mathieu Perreault wrote: > Add a comment above this: "If |numDesired| is smaller than |numExisting|, this > means extra tiles are to be hidden (window is downsizing)". Done, with rephrasing. https://chromiumcodereview.appspot.com/412073002/diff/100001/chrome/browser/r... chrome/browser/resources/local_ntp/local_ntp.js:734: newNumColumns = MAX_NUM_COLUMNS; On 2014/07/29 21:36:08, Mathieu Perreault wrote: > nit: newline below this for readability Done.
https://codereview.chromium.org/412073002/diff/120001/chrome/browser/resource... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/412073002/diff/120001/chrome/browser/resource... chrome/browser/resources/local_ntp/local_ntp.js:487: tilesContainer.style.visibility = 'visible'; You should also reset the barrier here. Otherwise, if you time out reload 1, then reload again (tearing down the onload listeners), the barrier will be stuck above 0 and we'll time out every subsequent reload. Alternatively, create a new barrier for each reload. https://codereview.chromium.org/412073002/diff/120001/chrome/browser/resource... chrome/browser/resources/local_ntp/local_ntp.js:494: // Javascript is single-threaded, so don't need to wrap the loop with nit: omit this comment. Readers should already know this fact. https://codereview.chromium.org/412073002/diff/120001/chrome/browser/resource... chrome/browser/resources/local_ntp/local_ntp.js:507: function renderTiles(clearAll) { Instead of adding this boolean parameter, either clear or don't clear the tiles container before calling this function. I think the logic is the same. https://codereview.chromium.org/412073002/diff/120001/chrome/browser/resource... chrome/browser/resources/local_ntp/local_ntp.js:517: var i; nit: you can say var i in the loop initializers below. javascript has function scope, not block scope. https://codereview.chromium.org/412073002/diff/120001/chrome/browser/resource... chrome/browser/resources/local_ntp/local_ntp.js:731: var availableWidth = innerWidth + TILE_MARGIN - MIN_TOTAL_HORIZONTAL_PADDING; I still don't understand this math. Imagine MIN_TOTAL_HORIZONTAL_PADDING were 0. Would it then be ok to position tiles such that the window has a horizontal scrollbar to show the extra margin?
I realized that if there are 3 or 2 columns, then on load the tiles appear after timeout. This is because <iframe> is loaded only when they're added to the DOM, *not* when the <iframe> Element is created. Therefore it is more appropriate for <iframe> onload binding to take place when adding them to DOM, not when creating the Elements. Extracted the binding to a separate function. Current caveat: if we start with 2 or 3 columns, then resize to 4, then in the first time screen flickers because the #mv-tiles gets hidden as the <iframe> for the newly-added-to-DOM tiles are loaded. I think this should be addressed in another CL. PTAL. Thanks! https://chromiumcodereview.appspot.com/412073002/diff/120001/chrome/browser/r... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://chromiumcodereview.appspot.com/412073002/diff/120001/chrome/browser/r... chrome/browser/resources/local_ntp/local_ntp.js:487: tilesContainer.style.visibility = 'visible'; Doing both: (1) making Barrier cancellable and doing so on tomeout, (2) instantiating new Barrier, and injecting it into createTile(). The variable is retained via closure in the handler functions. Moving the code to renderAndShowTiles(). https://chromiumcodereview.appspot.com/412073002/diff/120001/chrome/browser/r... chrome/browser/resources/local_ntp/local_ntp.js:494: // Javascript is single-threaded, so don't need to wrap the loop with On 2014/07/30 17:52:14, Jered wrote: > nit: omit this comment. Readers should already know this fact. Done. https://chromiumcodereview.appspot.com/412073002/diff/120001/chrome/browser/r... chrome/browser/resources/local_ntp/local_ntp.js:507: function renderTiles(clearAll) { On 2014/07/30 17:52:14, Jered wrote: > Instead of adding this boolean parameter, either clear or don't clear the tiles > container before calling this function. I think the logic is the same. Done. https://chromiumcodereview.appspot.com/412073002/diff/120001/chrome/browser/r... chrome/browser/resources/local_ntp/local_ntp.js:517: var i; On 2014/07/30 17:52:14, Jered wrote: > nit: you can say var i in the loop initializers below. javascript has function > scope, not block scope. Done. https://chromiumcodereview.appspot.com/412073002/diff/120001/chrome/browser/r... chrome/browser/resources/local_ntp/local_ntp.js:731: var availableWidth = innerWidth + TILE_MARGIN - MIN_TOTAL_HORIZONTAL_PADDING; Yeah it would be bizarre if the extra margin creates unnecessary scroll bar. But then MIN_TOTAL_HORIZONTAL_PADDING gives us ample space to grow into. And on desktop (well, Windows) the window cannot go sufficiently small for this to happen. If we ever need make padding small (mobile?), we can put a slightly shifted #mv-tiles inside a wrapper tile with "overflow:hidden" to solve this problem.
Ping jered@ re. recent changes. Thanks!
lgtm I didn't try this change out. I assume you've tested it pretty thoroughly, manually, since there are not great automated tests. Thanks for your patience and sorry for the long delays in review. https://codereview.chromium.org/412073002/diff/130001/chrome/browser/resource... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/412073002/diff/130001/chrome/browser/resource... chrome/browser/resources/local_ntp/local_ntp.js:523: // invisible then we leave it in DOM to prevent reload if they're shown again. nit: they're -> it's https://codereview.chromium.org/412073002/diff/130001/chrome/browser/resource... chrome/browser/resources/local_ntp/local_ntp.js:553: for (i = 0; i < numDesired; ++i) nit: still write var i here for clarity :-)
Thanks! Committing. https://chromiumcodereview.appspot.com/412073002/diff/130001/chrome/browser/r... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://chromiumcodereview.appspot.com/412073002/diff/130001/chrome/browser/r... chrome/browser/resources/local_ntp/local_ntp.js:523: // invisible then we leave it in DOM to prevent reload if they're shown again. On 2014/08/01 15:07:15, Jered wrote: > nit: they're -> it's Done. https://chromiumcodereview.appspot.com/412073002/diff/130001/chrome/browser/r... chrome/browser/resources/local_ntp/local_ntp.js:553: for (i = 0; i < numDesired; ++i) On 2014/08/01 15:07:15, Jered wrote: > nit: still write var i here for clarity :-) Done.
The CQ bit was checked by huangs@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/412073002/150001
The CQ bit was unchecked by huangs@chromium.org
Ping jhawkins@ for OWNER review of: chrome\browser\browser_resources.grd
lgtm
The CQ bit was checked by mathp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/412073002/150001
Message was sent while issue was closed.
Change committed as 287120 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
