Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629603002:
[NTP::Downloads] Fetch assets once the manager is loaded. (Closed)
Description[NTP::Downloads] Fetch assets once the manager is loaded.
1) Make DownloadsProvider listen to
DownloadHistory::OnHistoryQueryComplete. It is used as a proxy for when
DownloadManager is loaded.
2) Fetch assets on DownloadHistory::OnHistoryQueryComplete. This allows
us to be sure that the obtained list is complete and it can be used to
prune dismissed IDs.
3) Prune dismissed IDs properly when the assets are fetched. This
includes removing the previous hack with storing at most 100 dismissed
IDs (this CL is based on a revert of https://codereview.chromium.org/2562073002).
BUG=672758
Review-Url: https://codereview.chromium.org/2629603002
Cr-Commit-Position: refs/heads/master@{#444297}
Committed: https://chromium.googlesource.com/chromium/src/+/bab650391a6a91e98bfbcb075b0295dd888c231d
Patch Set 1 #
Total comments: 10
Patch Set 2 : clean rebase. #
Total comments: 19
Patch Set 3 : tschumann@ & treib@ comments. #
Total comments: 4
Patch Set 4 : clean rebase. #Patch Set 5 : tschumann@ & treib@ comments. #Patch Set 6 : constructed DownloadHistory + tests. #
Messages
Total messages: 34 (20 generated)
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.cc:500: item->RemoveObserver(this); what is this doing? Which role is 'this' playing here? with so many interfaces we're implementing this is getting really hard to follow. https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.h:42: public DownloadHistory::Observer { wow, that's a massive list of interfaces to implement. I'd try to avoid this and use aggregation instead. (it makes much more sense to think of the provider having these observers as opposed to being so many observers). What I would do instead is a wrapper class, like: class DownloadHistoryAvailableObserver; You can provide a fake for this to properly test it. The DownloadSugestionsProvider can then provide a callback which gets called when the initial query is completed and in addition ask whether it's already done (so no need for magic, you could write it in the constructor like this): if (history_available_observer_->IsHistoryLoaded()) { AsynchronouslyFetchOfflinePagesDownloads(); } else { history_available_observer_->NotifyWhenReader(base::Bind(this, DownloadSuggestionsProvider::AsynchronouslyFetchOfflinePagesDownloads)); } no extra implementations to override, no nullptr to deal with, and you can test your provider logic. Testing the wrapper is still tricky, but at least this is very encapsulated now.
Patchset #2 (id:20001) has been deleted
vitaliii@chromium.org changed reviewers: + treib@chromium.org
Hi treib@, Could you be so king to advise me on this? Basically I need to listen to DownloadHistory, but then I cannot mock it in tests, because it is difficult to construct. It does not implement any interface and its constructor takes 2 other classes, which do not implement interfaces either. https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.cc:500: item->RemoveObserver(this); On 2017/01/12 11:06:09, tschumann wrote: > what is this doing? Which role is 'this' playing here? with so many interfaces > we're implementing this is getting really hard to follow. This prevents adding this class as observer twice to the same DownloadItem. Otherwise, the following could occur: 1) We receive OnDownloadCreated(DownloadA) and start listening; 2) Then we do fetch assets and obtain a list with DownloadA and DownloadB (the latter we have never seen before), we start listening to both. So either we store what we are listening to or do the trick above. 'this' means DownloadSuggestionsProvider. It is listening to each download item, because this is the only way to get OnDownloadDestroyed after which the item is removed from the list of all downloads, so we can query all downloads if needed. https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.h:42: public DownloadHistory::Observer { On 2017/01/12 11:06:09, tschumann wrote: > wow, that's a massive list of interfaces to implement. > I'd try to avoid this and use aggregation instead. > (it makes much more sense to think of the provider having these observers as > opposed to being so many observers). > > What I would do instead is a wrapper class, like: > > class DownloadHistoryAvailableObserver; > > You can provide a fake for this to properly test it. > The DownloadSugestionsProvider can then provide a callback which gets called > when the initial query is completed and in addition ask whether it's already > done (so no need for magic, you could write it in the constructor like this): > if (history_available_observer_->IsHistoryLoaded()) { > AsynchronouslyFetchOfflinePagesDownloads(); > } else { > history_available_observer_->NotifyWhenReader(base::Bind(this, > DownloadSuggestionsProvider::AsynchronouslyFetchOfflinePagesDownloads)); > } > > no extra implementations to override, no nullptr to deal with, and you can test > your provider logic. > Testing the wrapper is still tricky, but at least this is very encapsulated now. I have never seen this before in Chromium. treib@, WDYT?
https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.cc:500: item->RemoveObserver(this); On 2017/01/16 08:51:35, vitaliii wrote: > On 2017/01/12 11:06:09, tschumann wrote: > > what is this doing? Which role is 'this' playing here? with so many interfaces > > we're implementing this is getting really hard to follow. > > This prevents adding this class as observer twice to the same DownloadItem. > Otherwise, the following could occur: > 1) We receive OnDownloadCreated(DownloadA) and start listening; > 2) Then we do fetch assets and obtain a list with DownloadA and DownloadB (the > latter we have never seen before), we start listening to both. > > So either we store what we are listening to or do the trick above. > > 'this' means DownloadSuggestionsProvider. It is listening to each download item, > because this is the only way to get OnDownloadDestroyed after which the item is > removed from the list of all downloads, so we can query all downloads if needed. =) A comment explaining this briefly would be great. What I meant with 'which role is "this" playing?' is what interface AddObserver takes (in this case it's DownloadItem::Observer). It's another sign of cost we're paying with one class doing too many things -- it's hard to reason about what's happening. Let's have a closer look at the flows and see if we can move some functionality into another class and keep the provider simpler (800+ lines of code for one class is massive). https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.h:42: public DownloadHistory::Observer { On 2017/01/16 08:51:35, vitaliii wrote: > On 2017/01/12 11:06:09, tschumann wrote: > > wow, that's a massive list of interfaces to implement. > > I'd try to avoid this and use aggregation instead. > > (it makes much more sense to think of the provider having these observers as > > opposed to being so many observers). > > > > What I would do instead is a wrapper class, like: > > > > class DownloadHistoryAvailableObserver; > > > > You can provide a fake for this to properly test it. > > The DownloadSugestionsProvider can then provide a callback which gets called > > when the initial query is completed and in addition ask whether it's already > > done (so no need for magic, you could write it in the constructor like this): > > if (history_available_observer_->IsHistoryLoaded()) { > > AsynchronouslyFetchOfflinePagesDownloads(); > > } else { > > history_available_observer_->NotifyWhenReader(base::Bind(this, > > DownloadSuggestionsProvider::AsynchronouslyFetchOfflinePagesDownloads)); > > } > > > > no extra implementations to override, no nullptr to deal with, and you can > test > > your provider logic. > > Testing the wrapper is still tricky, but at least this is very encapsulated > now. > > I have never seen this before in Chromium. > > treib@, WDYT? =) Well, not sure if you'd notice after the refactoring. What I definitely see too often are very heavy classes like this one (it's implementation is huge and getting proper test coverage is tricky).
On 2017/01/16 09:14:05, tschumann wrote: > https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... > File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): > > https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... > chrome/browser/ntp_snippets/download_suggestions_provider.cc:500: > item->RemoveObserver(this); > On 2017/01/16 08:51:35, vitaliii wrote: > > On 2017/01/12 11:06:09, tschumann wrote: > > > what is this doing? Which role is 'this' playing here? with so many > interfaces > > > we're implementing this is getting really hard to follow. > > > > This prevents adding this class as observer twice to the same DownloadItem. > > Otherwise, the following could occur: > > 1) We receive OnDownloadCreated(DownloadA) and start listening; > > 2) Then we do fetch assets and obtain a list with DownloadA and DownloadB (the > > latter we have never seen before), we start listening to both. > > > > So either we store what we are listening to or do the trick above. > > > > 'this' means DownloadSuggestionsProvider. It is listening to each download > item, > > because this is the only way to get OnDownloadDestroyed after which the item > is > > removed from the list of all downloads, so we can query all downloads if > needed. > > =) A comment explaining this briefly would be great. > What I meant with 'which role is "this" playing?' is what interface AddObserver > takes (in this case it's DownloadItem::Observer). It's another sign of cost > we're paying with one class doing too many things -- it's hard to reason about > what's happening. > > Let's have a closer look at the flows and see if we can move some functionality > into another class and keep the provider simpler (800+ lines of code for one > class is massive). > > https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... > File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): > > https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... > chrome/browser/ntp_snippets/download_suggestions_provider.h:42: public > DownloadHistory::Observer { > On 2017/01/16 08:51:35, vitaliii wrote: > > On 2017/01/12 11:06:09, tschumann wrote: > > > wow, that's a massive list of interfaces to implement. > > > I'd try to avoid this and use aggregation instead. > > > (it makes much more sense to think of the provider having these observers as > > > opposed to being so many observers). > > > > > > What I would do instead is a wrapper class, like: > > > > > > class DownloadHistoryAvailableObserver; > > > > > > You can provide a fake for this to properly test it. > > > The DownloadSugestionsProvider can then provide a callback which gets called > > > when the initial query is completed and in addition ask whether it's already > > > done (so no need for magic, you could write it in the constructor like > this): > > > if (history_available_observer_->IsHistoryLoaded()) { > > > AsynchronouslyFetchOfflinePagesDownloads(); > > > } else { > > > history_available_observer_->NotifyWhenReader(base::Bind(this, > > > DownloadSuggestionsProvider::AsynchronouslyFetchOfflinePagesDownloads)); > > > } > > > > > > no extra implementations to override, no nullptr to deal with, and you can > > test > > > your provider logic. > > > Testing the wrapper is still tricky, but at least this is very encapsulated > > now. > > > > I have never seen this before in Chromium. > > > > treib@, WDYT? > > =) Well, not sure if you'd notice after the refactoring. What I definitely see > too often are very heavy classes like this one (it's implementation is huge and > getting proper test coverage is tricky). I also realize that this will be a larger refactoring and I won't block this feature for that. Given the current team's focus on the hackathon, I'm fine with you moving forward with the current design, and I'll pick up the task of refactoring it today or tomorrow.
https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.cc:500: item->RemoveObserver(this); On 2017/01/16 09:14:05, tschumann wrote: > On 2017/01/16 08:51:35, vitaliii wrote: > > On 2017/01/12 11:06:09, tschumann wrote: > > > what is this doing? Which role is 'this' playing here? with so many > interfaces > > > we're implementing this is getting really hard to follow. > > > > This prevents adding this class as observer twice to the same DownloadItem. > > Otherwise, the following could occur: > > 1) We receive OnDownloadCreated(DownloadA) and start listening; > > 2) Then we do fetch assets and obtain a list with DownloadA and DownloadB (the > > latter we have never seen before), we start listening to both. > > > > So either we store what we are listening to or do the trick above. > > > > 'this' means DownloadSuggestionsProvider. It is listening to each download > item, > > because this is the only way to get OnDownloadDestroyed after which the item > is > > removed from the list of all downloads, so we can query all downloads if > needed. > > =) A comment explaining this briefly would be great. > What I meant with 'which role is "this" playing?' is what interface AddObserver > takes (in this case it's DownloadItem::Observer). It's another sign of cost > we're paying with one class doing too many things -- it's hard to reason about > what's happening. > > Let's have a closer look at the flows and see if we can move some functionality > into another class and keep the provider simpler (800+ lines of code for one > class is massive). Yup - please at least add a comment for now, and let's try to improve this whole situation in a follow-up. https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.h:42: public DownloadHistory::Observer { On 2017/01/16 09:14:05, tschumann wrote: > On 2017/01/16 08:51:35, vitaliii wrote: > > On 2017/01/12 11:06:09, tschumann wrote: > > > wow, that's a massive list of interfaces to implement. > > > I'd try to avoid this and use aggregation instead. > > > (it makes much more sense to think of the provider having these observers as > > > opposed to being so many observers). > > > > > > What I would do instead is a wrapper class, like: > > > > > > class DownloadHistoryAvailableObserver; > > > > > > You can provide a fake for this to properly test it. > > > The DownloadSugestionsProvider can then provide a callback which gets called > > > when the initial query is completed and in addition ask whether it's already > > > done (so no need for magic, you could write it in the constructor like > this): > > > if (history_available_observer_->IsHistoryLoaded()) { > > > AsynchronouslyFetchOfflinePagesDownloads(); > > > } else { > > > history_available_observer_->NotifyWhenReader(base::Bind(this, > > > DownloadSuggestionsProvider::AsynchronouslyFetchOfflinePagesDownloads)); > > > } > > > > > > no extra implementations to override, no nullptr to deal with, and you can > > test > > > your provider logic. > > > Testing the wrapper is still tricky, but at least this is very encapsulated > > now. > > > > I have never seen this before in Chromium. > > > > treib@, WDYT? > > =) Well, not sure if you'd notice after the refactoring. What I definitely see > too often are very heavy classes like this one (it's implementation is huge and > getting proper test coverage is tricky). There definitely are instances of what Tim suggests, though directly implementing observer interfaces is much more common. Usually I'm okay with this pattern, but in this case, it is getting a bit ridiculous, and I agree that Tim's approach would improve things. Doesn't have to be in this CL though. https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:159: // All downloads are fetched when the download manager is loaded, but if it Usually, all downloads are fetched... (to make clear that in the case we're in right now, that isn't actually the case) https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:178: } nit: It's good practice to do shutdown in the inverse order of initialization, i.e. here first history, then manager, then offline pages. https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:371: if (!is_download_manager_loaded_) { Would it make things easier if we registered this observer only after the history init is complete? https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:396: DCHECK(is_download_manager_loaded_); Couldn't this one happen before everything is loaded? https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:424: DCHECK(is_download_manager_loaded_); Also here https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:501: item->AddObserver(this); ? https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.h:229: bool is_download_manager_loaded_; Can we find a better name for this? It's not really related to the manager. is_downloads_initialization_complete_ or something like that? (It's unfortunate that "downloads" is used both for this class, and for one of its sources...)
Hi tschumann@ and treib@, I addressed your comments, PTAL. https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.cc:500: item->RemoveObserver(this); On 2017/01/16 10:33:05, Marc Treib wrote: > On 2017/01/16 09:14:05, tschumann wrote: > > On 2017/01/16 08:51:35, vitaliii wrote: > > > On 2017/01/12 11:06:09, tschumann wrote: > > > > what is this doing? Which role is 'this' playing here? with so many > > interfaces > > > > we're implementing this is getting really hard to follow. > > > > > > This prevents adding this class as observer twice to the same DownloadItem. > > > Otherwise, the following could occur: > > > 1) We receive OnDownloadCreated(DownloadA) and start listening; > > > 2) Then we do fetch assets and obtain a list with DownloadA and DownloadB > (the > > > latter we have never seen before), we start listening to both. > > > > > > So either we store what we are listening to or do the trick above. > > > > > > 'this' means DownloadSuggestionsProvider. It is listening to each download > > item, > > > because this is the only way to get OnDownloadDestroyed after which the item > > is > > > removed from the list of all downloads, so we can query all downloads if > > needed. > > > > =) A comment explaining this briefly would be great. > > What I meant with 'which role is "this" playing?' is what interface > AddObserver > > takes (in this case it's DownloadItem::Observer). It's another sign of cost > > we're paying with one class doing too many things -- it's hard to reason about > > what's happening. > > > > Let's have a closer look at the flows and see if we can move some > functionality > > into another class and keep the provider simpler (800+ lines of code for one > > class is massive). > > Yup - please at least add a comment for now, and let's try to improve this whole > situation in a follow-up. Done. I added a comment about RemoveObserver. https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider.h:42: public DownloadHistory::Observer { On 2017/01/16 10:33:05, Marc Treib wrote: > On 2017/01/16 09:14:05, tschumann wrote: > > On 2017/01/16 08:51:35, vitaliii wrote: > > > On 2017/01/12 11:06:09, tschumann wrote: > > > > wow, that's a massive list of interfaces to implement. > > > > I'd try to avoid this and use aggregation instead. > > > > (it makes much more sense to think of the provider having these observers > as > > > > opposed to being so many observers). > > > > > > > > What I would do instead is a wrapper class, like: > > > > > > > > class DownloadHistoryAvailableObserver; > > > > > > > > You can provide a fake for this to properly test it. > > > > The DownloadSugestionsProvider can then provide a callback which gets > called > > > > when the initial query is completed and in addition ask whether it's > already > > > > done (so no need for magic, you could write it in the constructor like > > this): > > > > if (history_available_observer_->IsHistoryLoaded()) { > > > > AsynchronouslyFetchOfflinePagesDownloads(); > > > > } else { > > > > history_available_observer_->NotifyWhenReader(base::Bind(this, > > > > DownloadSuggestionsProvider::AsynchronouslyFetchOfflinePagesDownloads)); > > > > } > > > > > > > > no extra implementations to override, no nullptr to deal with, and you can > > > test > > > > your provider logic. > > > > Testing the wrapper is still tricky, but at least this is very > encapsulated > > > now. > > > > > > I have never seen this before in Chromium. > > > > > > treib@, WDYT? > > > > =) Well, not sure if you'd notice after the refactoring. What I definitely see > > too often are very heavy classes like this one (it's implementation is huge > and > > getting proper test coverage is tricky). > > There definitely are instances of what Tim suggests, though directly > implementing observer interfaces is much more common. > Usually I'm okay with this pattern, but in this case, it is getting a bit > ridiculous, and I agree that Tim's approach would improve things. Doesn't have > to be in this CL though. Acknowledged. https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:159: // All downloads are fetched when the download manager is loaded, but if it On 2017/01/16 10:33:05, Marc Treib wrote: > Usually, all downloads are fetched... > (to make clear that in the case we're in right now, that isn't actually the > case) Done. https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:178: } On 2017/01/16 10:33:05, Marc Treib wrote: > nit: It's good practice to do shutdown in the inverse order of initialization, > i.e. here first history, then manager, then offline pages. Done. https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:371: if (!is_download_manager_loaded_) { On 2017/01/16 10:33:05, Marc Treib wrote: > Would it make things easier if we registered this observer only after the > history init is complete? Done. https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:396: DCHECK(is_download_manager_loaded_); On 2017/01/16 10:33:05, Marc Treib wrote: > Couldn't this one happen before everything is loaded? Nope, this is DownloadItem::Observer method, not DownloadManager::Observer. We start listening to DownloadItem only when DownloadManager finished loading. https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:424: DCHECK(is_download_manager_loaded_); On 2017/01/16 10:33:05, Marc Treib wrote: > Also here Same as above. https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:501: item->AddObserver(this); On 2017/01/16 10:33:05, Marc Treib wrote: > ? Same as in the previous patch set. Quote: "This prevents adding this class as observer twice to the same DownloadItem. Otherwise, the following could occur: 1) We receive OnDownloadCreated(DownloadA) and start listening; 2) Then we do fetch assets and obtain a list with DownloadA and DownloadB (the latter we have never seen before), we start listening to both. So either we store what we are listening to or do the trick above." I added a comment. https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.h:229: bool is_download_manager_loaded_; On 2017/01/16 10:33:05, Marc Treib wrote: > Can we find a better name for this? It's not really related to the manager. > is_downloads_initialization_complete_ or something like that? (It's unfortunate > that "downloads" is used both for this class, and for one of its sources...) Done. |is_asset_downloads_initialization_complete_| https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:268: /*download_history=*/nullptr, pref_service(), treib@, do you have any idea how I could test this properly? |nullptr| works here, however, this is not what we expect on device. I cannot mock |DownloadHistory| because it has complex constructor.
lgtm https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:268: /*download_history=*/nullptr, pref_service(), On 2017/01/16 14:32:39, vitaliii wrote: > treib@, > do you have any idea how I could test this properly? > > |nullptr| works here, however, this is not what we expect on device. > I cannot mock |DownloadHistory| because it has complex constructor. There are some tests that instantiate a DownloadHistory, but it seems to be quite a hassle: https://cs.chromium.org/chromium/src/chrome/browser/download/download_history... https://cs.chromium.org/chromium/src/chrome/browser/download/download_ui_cont... The "proper" way would probably be to extract an interface from DownloadHistory, and move the implementation to a DownloadHistoryImpl. Sorry, no smart ideas to save effort :-/
lgtm i primarily watched out for readability and inspected the new code closer. However, I trust that you got all the wiring right as I didn't check all data flows through this class. https://codereview.chromium.org/2629603002/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2629603002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:502: } else { nit: this would be easier to follow if inversed: if (old_dismissed_ids.count(within_category_id)) { retained_dismissed_ids.insert(within_category_id); } else if (IsDownloadCompleted(*item)) { ... } (translating the negation of count into "not dismissed" is a bit of a mental step -- at least for me.) https://codereview.chromium.org/2629603002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:795: bool for_offline_page_downloads) const { this has a smell. You'd better split this into 2 functions and get rid of the parameter, e.g.: ReadDismissed{OfflinePage, Download}IdsFromPrefs Same for the Store variant.
I addressed your comments. PTAL. https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:268: /*download_history=*/nullptr, pref_service(), On 2017/01/16 14:51:07, Marc Treib wrote: > On 2017/01/16 14:32:39, vitaliii wrote: > > treib@, > > do you have any idea how I could test this properly? > > > > |nullptr| works here, however, this is not what we expect on device. > > I cannot mock |DownloadHistory| because it has complex constructor. > > There are some tests that instantiate a DownloadHistory, but it seems to be > quite a hassle: > https://cs.chromium.org/chromium/src/chrome/browser/download/download_history... > https://cs.chromium.org/chromium/src/chrome/browser/download/download_ui_cont... > > The "proper" way would probably be to extract an interface from DownloadHistory, > and move the implementation to a DownloadHistoryImpl. > Sorry, no smart ideas to save effort :-/ Thank you for the links. Constructing it happened to be not that difficult (one can do HistoryAdapter(nullptr)). However, its constructor requires UI thread, which breaks the tests. I may extract the interface, but definitely not in this CL. I added a TODO and crbug.com/681766. https://codereview.chromium.org/2629603002/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2629603002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:502: } else { On 2017/01/16 15:18:57, tschumann wrote: > nit: this would be easier to follow if inversed: > if (old_dismissed_ids.count(within_category_id)) { > retained_dismissed_ids.insert(within_category_id); > } else if (IsDownloadCompleted(*item)) { > ... > } > > > (translating the negation of count into "not dismissed" is a bit of a mental > step -- at least for me.) Done. https://codereview.chromium.org/2629603002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:795: bool for_offline_page_downloads) const { On 2017/01/16 15:18:57, tschumann wrote: > this has a smell. You'd better split this into 2 functions and get rid of the > parameter, e.g.: ReadDismissed{OfflinePage, Download}IdsFromPrefs > > Same for the Store variant. Ack. There are {Read, Store}Dismissed{OfflinePage, Download}IdsFromPrefs. The general function ReadDismissedIDsFromPrefs is needed for {Dismiss, Invalidate}Suggestion, otherwise there is code duplication and it looks bad.
The CQ bit was checked by vitaliii@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/2629603002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:268: /*download_history=*/nullptr, pref_service(), On 2017/01/17 08:33:28, vitaliii wrote: > On 2017/01/16 14:51:07, Marc Treib wrote: > > On 2017/01/16 14:32:39, vitaliii wrote: > > > treib@, > > > do you have any idea how I could test this properly? > > > > > > |nullptr| works here, however, this is not what we expect on device. > > > I cannot mock |DownloadHistory| because it has complex constructor. > > > > There are some tests that instantiate a DownloadHistory, but it seems to be > > quite a hassle: > > > https://cs.chromium.org/chromium/src/chrome/browser/download/download_history... > > > https://cs.chromium.org/chromium/src/chrome/browser/download/download_ui_cont... > > > > The "proper" way would probably be to extract an interface from > DownloadHistory, > > and move the implementation to a DownloadHistoryImpl. > > Sorry, no smart ideas to save effort :-/ > > Thank you for the links. > > Constructing it happened to be not that difficult (one can do > HistoryAdapter(nullptr)). However, its constructor requires UI thread, which > breaks the tests. > > I may extract the interface, but definitely not in this CL. > > I added a TODO and crbug.com/681766. What exactly do you mean by "requires UI thread"? If it just needs a message loop so it can PostTask etc, then it might be enough to just have a base::MessageLoop instance in the test class.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #6 (id:120001) has been deleted
Patchset #6 (id:140001) has been deleted
The CQ bit was checked by vitaliii@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...
Patchset #6 (id:160001) has been deleted
The CQ bit was checked by vitaliii@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...
Hi, I finally constructed DownloadHistory, however, it seems awkward. However, this should work for now. Could you have a look? https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:268: /*download_history=*/nullptr, pref_service(), On 2017/01/17 08:50:04, Marc Treib wrote: > On 2017/01/17 08:33:28, vitaliii wrote: > > On 2017/01/16 14:51:07, Marc Treib wrote: > > > On 2017/01/16 14:32:39, vitaliii wrote: > > > > treib@, > > > > do you have any idea how I could test this properly? > > > > > > > > |nullptr| works here, however, this is not what we expect on device. > > > > I cannot mock |DownloadHistory| because it has complex constructor. > > > > > > There are some tests that instantiate a DownloadHistory, but it seems to be > > > quite a hassle: > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/download/download_history... > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/download/download_ui_cont... > > > > > > The "proper" way would probably be to extract an interface from > > DownloadHistory, > > > and move the implementation to a DownloadHistoryImpl. > > > Sorry, no smart ideas to save effort :-/ > > > > Thank you for the links. > > > > Constructing it happened to be not that difficult (one can do > > HistoryAdapter(nullptr)). However, its constructor requires UI thread, which > > breaks the tests. > > > > I may extract the interface, but definitely not in this CL. > > > > I added a TODO and crbug.com/681766. > > What exactly do you mean by "requires UI thread"? > If it just needs a message loop so it can PostTask etc, then it might be enough > to just have a base::MessageLoop instance in the test class. The issue was in |DCHECK_CURRENTLY_ON(content::BrowserThread::UI)| in DownloadHistory's constructor. Having content::TestingBrowserThreadBundle as a member was the answer :)
On 2017/01/17 16:12:41, vitaliii wrote: > Hi, > > I finally constructed DownloadHistory, however, it seems awkward. However, this > should work for now. > > Could you have a look? > > https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... > File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc > (right): > > https://codereview.chromium.org/2629603002/diff/40001/chrome/browser/ntp_snip... > chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:268: > /*download_history=*/nullptr, pref_service(), > On 2017/01/17 08:50:04, Marc Treib wrote: > > On 2017/01/17 08:33:28, vitaliii wrote: > > > On 2017/01/16 14:51:07, Marc Treib wrote: > > > > On 2017/01/16 14:32:39, vitaliii wrote: > > > > > treib@, > > > > > do you have any idea how I could test this properly? > > > > > > > > > > |nullptr| works here, however, this is not what we expect on device. > > > > > I cannot mock |DownloadHistory| because it has complex constructor. > > > > > > > > There are some tests that instantiate a DownloadHistory, but it seems to > be > > > > quite a hassle: > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/download/download_history... > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/download/download_ui_cont... > > > > > > > > The "proper" way would probably be to extract an interface from > > > DownloadHistory, > > > > and move the implementation to a DownloadHistoryImpl. > > > > Sorry, no smart ideas to save effort :-/ > > > > > > Thank you for the links. > > > > > > Constructing it happened to be not that difficult (one can do > > > HistoryAdapter(nullptr)). However, its constructor requires UI thread, which > > > breaks the tests. > > > > > > I may extract the interface, but definitely not in this CL. > > > > > > I added a TODO and crbug.com/681766. > > > > What exactly do you mean by "requires UI thread"? > > If it just needs a message loop so it can PostTask etc, then it might be > enough > > to just have a base::MessageLoop instance in the test class. > > The issue was in |DCHECK_CURRENTLY_ON(content::BrowserThread::UI)| in > DownloadHistory's constructor. Having content::TestingBrowserThreadBundle as a > member was the answer :) lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2629603002/#ps180001 (title: "constructed DownloadHistory + tests.")
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": 180001, "attempt_start_ts": 1484722875102710,
"parent_rev": "5d804c8487702e7a56e7b6f8370a0d10a756df6c", "commit_rev":
"bab650391a6a91e98bfbcb075b0295dd888c231d"}
Message was sent while issue was closed.
Description was changed from ========== [NTP::Downloads] Fetch assets once the manager is loaded. 1) Make DownloadsProvider listen to DownloadHistory::OnHistoryQueryComplete. It is used as a proxy for when DownloadManager is loaded. 2) Fetch assets on DownloadHistory::OnHistoryQueryComplete. This allows us to be sure that the obtained list is complete and it can be used to prune dismissed IDs. 3) Prune dismissed IDs properly when the assets are fetched. This includes removing the previous hack with storing at most 100 dismissed IDs (this CL is based on a revert of https://codereview.chromium.org/2562073002). BUG=672758 ========== to ========== [NTP::Downloads] Fetch assets once the manager is loaded. 1) Make DownloadsProvider listen to DownloadHistory::OnHistoryQueryComplete. It is used as a proxy for when DownloadManager is loaded. 2) Fetch assets on DownloadHistory::OnHistoryQueryComplete. This allows us to be sure that the obtained list is complete and it can be used to prune dismissed IDs. 3) Prune dismissed IDs properly when the assets are fetched. This includes removing the previous hack with storing at most 100 dismissed IDs (this CL is based on a revert of https://codereview.chromium.org/2562073002). BUG=672758 Review-Url: https://codereview.chromium.org/2629603002 Cr-Commit-Position: refs/heads/master@{#444297} Committed: https://chromium.googlesource.com/chromium/src/+/bab650391a6a91e98bfbcb075b02... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:180001) as https://chromium.googlesource.com/chromium/src/+/bab650391a6a91e98bfbcb075b02... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
