Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2887383003:
Refresh the Most Visited tiles only on user interaction (Closed)
|
Created:
3 years, 7 months ago by gambard Modified:
3 years, 6 months ago CC:
chromium-reviews, marq+watch_chromium.org, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefresh the Most Visited tiles only on user interaction
The Most Visited tiles presented to the user should only changes when the user
interacts with them: deletion or undo deletion.
BUG=712632
Review-Url: https://codereview.chromium.org/2887383003
Cr-Commit-Position: refs/heads/master@{#473887}
Committed: https://chromium.googlesource.com/chromium/src/+/dfc8047496d837d2b9915dd40f5f84ef51853a0e
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address comments #
Total comments: 4
Messages
Total messages: 21 (11 generated)
gambard@chromium.org changed reviewers: + fhorschig@chromium.org
PTAL :)
Thank you very much! lgtm with comment. https://codereview.chromium.org/2887383003/diff/1/ios/chrome/browser/ui/ntp/g... File ios/chrome/browser/ui/ntp/google_landing_mediator.mm (right): https://codereview.chromium.org/2887383003/diff/1/ios/chrome/browser/ui/ntp/g... ios/chrome/browser/ui/ntp/google_landing_mediator.mm:251: if (_mostVisitedData.size() > 0) { Could you please add a comment why the fresh data is not immediately used when we already have tiles?
gambard@chromium.org changed reviewers: + noyau@chromium.org
Thanks. noyau@: PTAL and land it if it is OK with you. https://codereview.chromium.org/2887383003/diff/1/ios/chrome/browser/ui/ntp/g... File ios/chrome/browser/ui/ntp/google_landing_mediator.mm (right): https://codereview.chromium.org/2887383003/diff/1/ios/chrome/browser/ui/ntp/g... ios/chrome/browser/ui/ntp/google_landing_mediator.mm:251: if (_mostVisitedData.size() > 0) { On 2017/05/19 13:09:57, fhorschig wrote: > Could you please add a comment why the fresh data is not immediately used when > we already have tiles? Done.
The CQ bit was checked by gambard@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: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by noyau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fhorschig@chromium.org Link to the patchset: https://codereview.chromium.org/2887383003/#ps20001 (title: "Address comments")
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": 20001, "attempt_start_ts": 1495548798012400,
"parent_rev": "fd2aa6510e3b05e4ca3816a1b0e42ca59f3051dd", "commit_rev":
"dfc8047496d837d2b9915dd40f5f84ef51853a0e"}
Message was sent while issue was closed.
Description was changed from ========== Refresh the Most Visited tiles only on user interaction The Most Visited tiles presented to the user should only changes when the user interacts with them: deletion or undo deletion. BUG=712632 ========== to ========== Refresh the Most Visited tiles only on user interaction The Most Visited tiles presented to the user should only changes when the user interacts with them: deletion or undo deletion. BUG=712632 Review-Url: https://codereview.chromium.org/2887383003 Cr-Commit-Position: refs/heads/master@{#473887} Committed: https://chromium.googlesource.com/chromium/src/+/dfc8047496d837d2b9915dd40f5f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/dfc8047496d837d2b9915dd40f5f...
Message was sent while issue was closed.
justincohen@chromium.org changed reviewers: + justincohen@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2887383003/diff/20001/ios/chrome/browser/ui/n... File ios/chrome/browser/ui/ntp/google_landing_mediator.mm (right): https://codereview.chromium.org/2887383003/diff/20001/ios/chrome/browser/ui/n... ios/chrome/browser/ui/ntp/google_landing_mediator.mm:387: return; should this NOTREACHED?
Message was sent while issue was closed.
https://codereview.chromium.org/2887383003/diff/20001/ios/chrome/browser/ui/n... File ios/chrome/browser/ui/ntp/google_landing_mediator.mm (right): https://codereview.chromium.org/2887383003/diff/20001/ios/chrome/browser/ui/n... ios/chrome/browser/ui/ntp/google_landing_mediator.mm:387: return; On 2017/05/24 15:58:21, justincohen (OOTO til Jun 5) wrote: > should this NOTREACHED? No. If you remove a tile, all the tiles are not necessarily updated. Why do you think not REACHED would be good?
Message was sent while issue was closed.
https://codereview.chromium.org/2887383003/diff/20001/ios/chrome/browser/ui/n... File ios/chrome/browser/ui/ntp/google_landing_mediator.mm (right): https://codereview.chromium.org/2887383003/diff/20001/ios/chrome/browser/ui/n... ios/chrome/browser/ui/ntp/google_landing_mediator.mm:387: return; On 2017/06/02 12:47:04, gambard wrote: > On 2017/05/24 15:58:21, justincohen (OOTO til Jun 5) wrote: > > should this NOTREACHED? > > No. If you remove a tile, all the tiles are not necessarily updated. Why do you > think not REACHED would be good? When would removing a tile not call -onMostVisitedURLsAvailable? It seems -useFreshData is only called after changing the blacklist, which should always fire -onMostVisitedURLsAvailable: and set freshMostVisitedData. I was suggesting that if useFreshData is called without freshMostVisitedData being set, something is wrong. If you only have 1 tile, and you add it to the blacklist, freshMostVisitedData won't get updated (if (_mostVisitedData.size() > 0))... so maybe it's not worth it.
Message was sent while issue was closed.
https://codereview.chromium.org/2887383003/diff/20001/ios/chrome/browser/ui/n... File ios/chrome/browser/ui/ntp/google_landing_mediator.mm (right): https://codereview.chromium.org/2887383003/diff/20001/ios/chrome/browser/ui/n... ios/chrome/browser/ui/ntp/google_landing_mediator.mm:387: return; On 2017/06/05 15:18:59, justincohen (OOTO til Jun 5) wrote: > On 2017/06/02 12:47:04, gambard wrote: > > On 2017/05/24 15:58:21, justincohen (OOTO til Jun 5) wrote: > > > should this NOTREACHED? > > > > No. If you remove a tile, all the tiles are not necessarily updated. Why do > you > > think not REACHED would be good? > > When would removing a tile not call -onMostVisitedURLsAvailable? > > It seems -useFreshData is only called after changing the blacklist, which should > always fire > -onMostVisitedURLsAvailable: and set freshMostVisitedData. I was suggesting > that if useFreshData is called without freshMostVisitedData being set, something > is wrong. > > If you only have 1 tile, and you add it to the blacklist, freshMostVisitedData > won't get updated (if (_mostVisitedData.size() > 0))... so maybe it's not worth > it. Actually this code do not remove the last tile. I will modify it, adding the NOTREACHED and giving the possibility to remove the last tile. |
