Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710473003:
📰 Ensure NTP Tiles keep tracking recent data (Closed)
Description📰 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}
Committed: https://chromium.googlesource.com/chromium/src/+/77150528cdf2a84cc61a23f1f4bc1bdb20dc31a5
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add unit tests #Patch Set 3 : Fix tests and ChromeHome #Patch Set 4 : Fix initialisation with no MV Data, move back to array #
Total comments: 32
Patch Set 5 : Tiny fixes, add extra debugging to investigate tester issue #Patch Set 6 : address comments #
Total comments: 3
Patch Set 7 : address comments #Patch Set 8 : Add OWNERS file for new test folder #Messages
Total messages: 43 (33 generated)
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dgn@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
PTAL https://codereview.chromium.org/2710473003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (left): https://codereview.chromium.org/2710473003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:247: mPendingLoadTasks++; Moved inside of startObserving() to have the tilegroup notify about both adding and removing the task.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dgn@chromium.org changed reviewers: + bauerb@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/Tile.java (right): https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/Tile.java:53: * Import transient data from an old time, and report whether there is a significant difference nit: 'Imports transient data from another tile, and reports [...]' https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/Tile.java:56: public boolean importData(Tile tile) { nit: mark the argument @Nullable https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/Tile.java:59: assert tile.getUrl().equals(mUrl); // precondition for triggering the import. This requirement should be mentioned in the method documentation. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/Tile.java:64: if (!tile.getTitle().equals(mTitle)) return true; Use TextUtils.equals? https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java (right): https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:118: private final int mTitleLinesCount; nit: move this before mIconGenerator as it is assigned earlier. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:131: public TileGroup(Context context, SuggestionsUiDelegate uiDelegate, Maybe it's time for documenting all these params. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:133: int tileTitleLines) { nit: maybe rename tileTitleLines to titleLines, as we're in TileGroup and the 'tile' part is pretty clear. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:179: if (tile == null) return; // The tile might have been removed. Should this ever happen? That would be surprising, as we blacklist the url in the same place that calls this method. If this should never happen, we can assert that instead. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:217: // TODO(dgn): That implies we had duplicated tiles. WAI? Yea not sure how that ever happened, but it was the only way I could that crash happening. I do not believe we should ever have duplicate tiles, so we could chase that further. Maybe with UMA tracking. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:245: boolean dataChanged = countChanged; This seems wrong, couldn't the data change without affecting the number of tiles? https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:262: * TODO update doc, it does not load the view anymore. Maybe do this TODO :-) https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:276: // Note: It is important that the callbacks below don't hold to the tile or modify them 'don't keep a reference to the tile' https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:305: private Tile getTile(String url) { nit: please document under what circumstances this might return null. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:307: if (tile.getUrl().equals(url)) return tile; use TextUtils.equals? https://codereview.chromium.org/2710473003/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java (right): https://codereview.chromium.org/2710473003/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java:72: public void testInitialiseWithEmptyMVList() { Can we avoid using 'MV'? We're moving away from the MostVisited name as it is incorrect. Also, please avoid such abbreviations. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java:77: notifyMVUrlsAvailable(); Same here, please avoid the MV bit.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/Tile.java (right): https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/Tile.java:53: * Import transient data from an old time, and report whether there is a significant difference On 2017/02/22 12:11:59, Michael van Ouwerkerk wrote: > nit: 'Imports transient data from another tile, and reports [...]' Done. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/Tile.java:56: public boolean importData(Tile tile) { On 2017/02/22 12:11:59, Michael van Ouwerkerk wrote: > nit: mark the argument @Nullable Done. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/Tile.java:59: assert tile.getUrl().equals(mUrl); // precondition for triggering the import. On 2017/02/22 12:11:59, Michael van Ouwerkerk wrote: > This requirement should be mentioned in the method documentation. Done. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/Tile.java:64: if (!tile.getTitle().equals(mTitle)) return true; On 2017/02/22 12:11:59, Michael van Ouwerkerk wrote: > Use TextUtils.equals? url can't be null. Added check for that in buildTiles() https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java (right): https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:118: private final int mTitleLinesCount; On 2017/02/22 12:11:59, Michael van Ouwerkerk wrote: > nit: move this before mIconGenerator as it is assigned earlier. Done. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:131: public TileGroup(Context context, SuggestionsUiDelegate uiDelegate, On 2017/02/22 12:11:59, Michael van Ouwerkerk wrote: > Maybe it's time for documenting all these params. Done. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:133: int tileTitleLines) { On 2017/02/22 12:11:59, Michael van Ouwerkerk wrote: > nit: maybe rename tileTitleLines to titleLines, as we're in TileGroup and the > 'tile' part is pretty clear. Done. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:179: if (tile == null) return; // The tile might have been removed. On 2017/02/22 12:11:59, Michael van Ouwerkerk wrote: > Should this ever happen? That would be surprising, as we blacklist the url in > the same place that calls this method. If this should never happen, we can > assert that instead. It could also be that other tiles have been fetched that have higher priority and kicked this one off the group. So it's not rendered in the UI even though it's not blacklisted. Also the old code doesn't assert in case it doesn't find the Tile. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:217: // TODO(dgn): That implies we had duplicated tiles. WAI? On 2017/02/22 12:12:00, Michael van Ouwerkerk wrote: > Yea not sure how that ever happened, but it was the only way I could that crash > happening. I do not believe we should ever have duplicate tiles, so we could > chase that further. Maybe with UMA tracking. Moved the check to where we build mTiles, since the map used above means we should not have duplicates at the level of the TileViews themselves. We'll see if we still get crashes. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:245: boolean dataChanged = countChanged; On 2017/02/22 12:11:59, Michael van Ouwerkerk wrote: > This seems wrong, couldn't the data change without affecting the number of > tiles? Yes, but for initialisation purposes, if the count changes, then the data has changed. Then if the data for the tiles themselves is different, we would also set the flag. Otherwise, if it's only that a tile was removed, it wouldn't be marked as a data change, since the remaining tiles are the same as before. It also ensures that we report the initial load as a data change. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:262: * TODO update doc, it does not load the view anymore. On 2017/02/22 12:11:59, Michael van Ouwerkerk wrote: > Maybe do this TODO :-) It's still up to date. Removed the comment. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:276: // Note: It is important that the callbacks below don't hold to the tile or modify them On 2017/02/22 12:11:59, Michael van Ouwerkerk wrote: > 'don't keep a reference to the tile' Done. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:305: private Tile getTile(String url) { On 2017/02/22 12:11:59, Michael van Ouwerkerk wrote: > nit: please document under what circumstances this might return null. Done. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:307: if (tile.getUrl().equals(url)) return tile; On 2017/02/22 12:11:59, Michael van Ouwerkerk wrote: > use TextUtils.equals? the tile's url should never be null, so TextUtils.equals is not needed here. I could assert that the url is not null when building tiles if you think we should really make sure of that. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java (right): https://codereview.chromium.org/2710473003/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java:72: public void testInitialiseWithEmptyMVList() { On 2017/02/22 12:12:00, Michael van Ouwerkerk wrote: > Can we avoid using 'MV'? We're moving away from the MostVisited name as it is > incorrect. Also, please avoid such abbreviations. Done. https://codereview.chromium.org/2710473003/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java:77: notifyMVUrlsAvailable(); On 2017/02/22 12:12:00, Michael van Ouwerkerk wrote: > Same here, please avoid the MV bit. Done. https://codereview.chromium.org/2710473003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java (right): https://codereview.chromium.org/2710473003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:256: // should not even be necessary as the backend is supposed to send non dupes URLs. TBD as separate bug/CL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nit. Thanks! https://codereview.chromium.org/2710473003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileView.java (left): https://codereview.chromium.org/2710473003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileView.java:37: * @param tile The tile that holds the data to populate this view. No need to delete this param doc, the param is unchanged.
Thanks! https://codereview.chromium.org/2710473003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileView.java (left): https://codereview.chromium.org/2710473003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileView.java:37: * @param tile The tile that holds the data to populate this view. On 2017/02/23 14:25:19, Michael van Ouwerkerk wrote: > No need to delete this param doc, the param is unchanged. Done.
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2710473003/#ps120001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2710473003/#ps140001 (title: "Add OWNERS file for new test folder")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487864217122190,
"parent_rev": "26546b45707c7f8247976dfe4a1436d19cb0c172", "commit_rev":
"77150528cdf2a84cc61a23f1f4bc1bdb20dc31a5"}
Message was sent while issue was closed.
Description was changed from ========== 📰 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 ========== to ========== 📰 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} Committed: https://chromium.googlesource.com/chromium/src/+/77150528cdf2a84cc61a23f1f4bc... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/77150528cdf2a84cc61a23f1f4bc... |
