New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 694297 link

Starred by 2 users
Status: Fixed
Owner:
█ dgn@chromium.org
Not on Chrome anymore
Closed: Feb 2017
Cc:
mastiz@chromium.org
mvanouwe...@chromium.org
█ ramyan@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug

Blocking:
issue 693514



Sign in to add a comment

NTP Tiles: View and model data get out of sync

Project Member Reported by dgn@chromium.org, Feb 20 2017 
The flow is something like:

1. NTP Created
2. TileGroup Created
3. MostVisitedData received (callback) ->  Tiles built and rendered
4. Large icon fetch completes (callback) -> Tile data updated and rerender

3 and 4 are asynchronous and the way we build tiles makes us keep the views while we discard the model. Rendering is based on the data the view keep internally, so if 3 happens a second time before 4, the fetched large icons are applied on the new instances of our tile model while the render is based on the old model, that doesn't have the large icons. And then we can't render it.

Comment 1 by tschumann@chromium.org, Feb 21 2017 

So this would explain why oftentimes we only see nice (non-gray) icons when we open the next NTP? 
That's a huge deal for FRE with popular sites -- would be awesome to get fixed.

Comment 2 by tschumann@chromium.org, Feb 21 2017

Cc: mastiz@chromium.org

Comment 3 by dgn@chromium.org, Feb 21 2017

Blocking: 693514
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 23 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/77150528cdf2a84cc61a23f1f4bc1bdb20dc31a5

commit 77150528cdf2a84cc61a23f1f4bc1bdb20dc31a5
Author: dgn <dgn@chromium.org>
Date: Thu Feb 23 17:09:58 2017

📰 Ensure NTP Tiles keep tracking recent data

Makes TileViews stop relying on the Tile they have been initialised
with as data source. The TileGroup does not hold old ones around
and the data would get out of sync with the Tiles used by the Views

This change switches to keeping track of URLs from TileViews, and
holding the Tiles in a Map to link them back to the full data.

BUG= 694297 

Review-Url: https://codereview.chromium.org/2710473003
Cr-Commit-Position: refs/heads/master@{#452525}

[modify] https://crrev.com/77150528cdf2a84cc61a23f1f4bc1bdb20dc31a5/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
[modify] https://crrev.com/77150528cdf2a84cc61a23f1f4bc1bdb20dc31a5/chrome/android/java/src/org/chromium/chrome/browser/suggestions/Tile.java
[modify] https://crrev.com/77150528cdf2a84cc61a23f1f4bc1bdb20dc31a5/chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGrid.java
[modify] https://crrev.com/77150528cdf2a84cc61a23f1f4bc1bdb20dc31a5/chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGridLayout.java
[modify] https://crrev.com/77150528cdf2a84cc61a23f1f4bc1bdb20dc31a5/chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java
[modify] https://crrev.com/77150528cdf2a84cc61a23f1f4bc1bdb20dc31a5/chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileView.java
[modify] https://crrev.com/77150528cdf2a84cc61a23f1f4bc1bdb20dc31a5/chrome/android/java_sources.gni
[add] https://crrev.com/77150528cdf2a84cc61a23f1f4bc1bdb20dc31a5/chrome/android/junit/src/org/chromium/chrome/browser/suggestions/OWNERS
[add] https://crrev.com/77150528cdf2a84cc61a23f1f4bc1bdb20dc31a5/chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java

Comment 5 by dgn@chromium.org, Feb 27 2017

Status: Fixed (was: Started)
Re #1: I don't know if it's the only reason, but I definitely see that being involved.
►

Sign in to add a comment