Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2360263002:
[NTPSnippets] Show all downloads on the NTP (3/3): Downloads provider. (Closed)
Description[NTPSnippets] Show all downloads on the NTP (3/3): Downloads provider.
This CL is the third of the three CLs.
The overall aim is to show all types of downloads on the NTP in
downloads section.
This CL adds DownloadSuggestionsProvider, which provides both offline
page (e.g. offlined pages) and asset (e.g. images) downloads.
If any of the two previous CLs is reverted, this CL must be reverted too.
BUG=639233
Committed: https://crrev.com/dbedcae7e3c81448f3fc57f1493263087d2eb1b1
Cr-Commit-Position: refs/heads/master@{#429194}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Marc's comments. #
Total comments: 132
Patch Set 3 : Marc's comments + tests + some corrections. #
Total comments: 60
Patch Set 4 : Marc's comments + rebase + improvements + more tests. #Patch Set 5 : fix for win. #Patch Set 6 : another win fix. #
Total comments: 46
Patch Set 7 : rebase + Marc's comments. #
Total comments: 26
Patch Set 8 : Marc's comments + Bernhard's comments + rebase. #
Total comments: 4
Patch Set 9 : rebase. #Patch Set 10 : more verbose error message. #Patch Set 11 : rebase + more debug output. #Patch Set 12 : more debug output. #Patch Set 13 : debug. #Patch Set 14 : removed my strings #Patch Set 15 : more debug (cmp strings). #Patch Set 16 : debug (output all strings). #Patch Set 17 : debug (reverted component string back). #Patch Set 18 : restored predebug implementation, debug output remains. #Patch Set 19 : dgn@ & bauerb@ comments + removed debug output + rebase. #
Total comments: 5
Patch Set 20 : fixed some comments. #Patch Set 21 : rebase. #Patch Set 22 : the freshest rebase. #
Total comments: 1
Messages
Total messages: 125 (99 generated)
Patchset #1 (id:1) has been deleted
vitaliii@chromium.org changed reviewers: + treib@chromium.org
Hi Marc, could you have a look?
Added an important comment. https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... File components/ntp_snippets/downloads/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... components/ntp_snippets/downloads/download_suggestions_provider.cc:270: FetchAssetsDownloads(); Since this is called when each item is read, the overall complexity is O(N^2), where N is number of downloads. I will add this to TODO.
Sending comments early: I haven't actually looked at the new Provider yet, but there's a dependency problem that needs to be resolved. Probably best to talk in person about that :) https://codereview.chromium.org/2360263002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2360263002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:78: using ntp_snippets::DownloadSuggestionsProvider; I think this should also go into the OS_ANDROID section above? https://codereview.chromium.org/2360263002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:117: bool download_manager_ui_enabled = Instead of the explicit download_manager_ui_enabled flag, could we just pass in null for the DownloadManager? https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... File components/ntp_snippets/downloads/DEPS (right): https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... components/ntp_snippets/downloads/DEPS:2: "+chrome/browser/download", This won't work - code in components/ must not depends on anything in chrome/ https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... File components/ntp_snippets/offline_pages/DEPS (right): https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... components/ntp_snippets/offline_pages/DEPS:2: "+components/offline_pages", ? https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... File components/ntp_snippets/pref_names.cc (right): https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... components/ntp_snippets/pref_names.cc:33: "ntp_suggestions.downloads.offline_pages.dismissed_ids"; FYI: Generally, you cannot just rename prefs - that will leave any existing data orphaned under the old name. But in this case, since this was never launched anywhere, that's okay.
Hi Marc, would you mind having a rough look before I proceed with the tests? https://codereview.chromium.org/2360263002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2360263002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:78: using ntp_snippets::DownloadSuggestionsProvider; On 2016/09/22 13:45:52, Marc Treib wrote: > I think this should also go into the OS_ANDROID section above? Done. https://codereview.chromium.org/2360263002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:117: bool download_manager_ui_enabled = On 2016/09/22 13:45:52, Marc Treib wrote: > Instead of the explicit download_manager_ui_enabled flag, could we just pass in > null for the DownloadManager? This variable has nothing to do with the DownloadManager itself. It is true when "Downloads" UI is turned on (and so we show "More" button). If it is false, we still show the section. https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... File components/ntp_snippets/downloads/DEPS (right): https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... components/ntp_snippets/downloads/DEPS:2: "+chrome/browser/download", On 2016/09/22 13:45:52, Marc Treib wrote: > This won't work - code in components/ must not depends on anything in chrome/ Moved the provider to chrome/browser/. https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... File components/ntp_snippets/downloads/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... components/ntp_snippets/downloads/download_suggestions_provider.cc:270: FetchAssetsDownloads(); On 2016/09/22 13:43:17, vitaliii wrote: > Since this is called when each item is read, the overall complexity is O(N^2), > where N is number of downloads. I will add this to TODO. I rewrote this to avoid fetching all items. https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... File components/ntp_snippets/offline_pages/DEPS (right): https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... components/ntp_snippets/offline_pages/DEPS:2: "+components/offline_pages", On 2016/09/22 13:45:52, Marc Treib wrote: > ? The idea was to add a comma, while we have a DEPS approval. I am not sure whether the comma is required there, but if it is, then it is a shame to ask offline pages team for a new approval when we need to add new dependency (i.e. new line). https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... File components/ntp_snippets/pref_names.cc (right): https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... components/ntp_snippets/pref_names.cc:33: "ntp_suggestions.downloads.offline_pages.dismissed_ids"; On 2016/09/22 13:45:52, Marc Treib wrote: > FYI: Generally, you cannot just rename prefs - that will leave any existing data > orphaned under the old name. But in this case, since this was never launched > anywhere, that's okay. Acknowledged. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:432: if (item->GetEndTime() <= (*oldest)->GetEndTime()) Would using an index here be better?
https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... File components/ntp_snippets/offline_pages/DEPS (right): https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... components/ntp_snippets/offline_pages/DEPS:2: "+components/offline_pages", On 2016/10/11 08:15:56, vitaliii wrote: > On 2016/09/22 13:45:52, Marc Treib wrote: > > ? > > The idea was to add a comma, while we have a DEPS approval. I am not sure > whether the comma is required there, but if it is, then it is a shame to ask > offline pages team for a new approval when we need to add new dependency (i.e. > new line). I think we don't need any particular approval for adding that comma - the DEPS checker should be smart enough for that. Anyway, this is fine. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:118: base::FeatureList::IsEnabled(chrome::android::kDownloadsUiFeature); I think the DownloadSuggestionsProvider can check that itself, so we can save one ctor parameter. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/downloads/DEPS (right): https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/DEPS:2: "+grit/components_strings.h", Is this needed?! If the provider itself isn't in components, then its strings shouldn't be there either. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:34: const int kMaxSuggestionsCount = 5; This should probably be configurable via a variation param - see e.g. the bookmarks provider for an example. (Can happen in another CL, but maybe add a TODO) https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:36: const char kOfflinePageDownloadsPrefix[] = "O"; Just make these two chars (not array) https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:42: ""); s/""/std::string()/ But if you're just appending two strings, might as well just do "string1 + string2". https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:48: {kAssetDownloadsPrefix, base::UintToString(raw_download_id)}, ""); Same here https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:53: bool IsOfflinePageID(const ContentSuggestion::ID& suggestion_id) { nit: "OfflinePageID" is somewhat overloaded. Maybe "CorrespondsToOfflinePage"? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:75: client_id.name_space == offline_pages::kNTPSuggestionsNamespace) && What's kNTPSuggestionsNamespace? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:76: base::IsValidGUID(client_id.id); Is this needed? Would we ever expect to get an invalid GUID here? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:79: bool IsDownloadCompleted(const DownloadItem* item) { nit: "const DownloadItem&" if it can't be null https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:133: return CategoryStatus::NOT_PROVIDED; Just do DCHECK_EQ(provided_category_, category); return category_status_; (no need to handle DCHECK or NOTREACHED failures - treat them like asserts) https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:144: NOTREACHED() << "Unknown category " << category.id(); Same here https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:157: StoreDismissedIDsToPrefs(suggestion_id, dismissed_ids); nit: I find it a bit confusing to pass in the suggestion_id if you only need to know offline vs asset - maybe just pass in that bool? Or just call StoreAsset/OfflinePageDismissedIDsToPrefs directly? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:176: ClearDismissedSuggestionsForDebugging(provided_category_); This will call FetchAllDownloadsAndSubmitSuggestions, which we probably don't want here. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:186: FetchAllDownloadsAndSubmitSuggestions(); Would we expect this to change anything? The caches should already be up to date with the "backends", right? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:217: NotifyStatusChanged(CategoryStatus::AVAILABLE); SubmitContentSuggestions will change the status, so I don't think it's required here. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:227: // other than NOT_PROVIDED, so only notify invalidation in that case. Actually, can the status *ever* be NOT_PROVIDED now? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:249: void DownloadSuggestionsProvider::OnDownloadOpened(DownloadManager* manager, nit: empty line before https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:253: void DownloadSuggestionsProvider::OnDownloadRemoved(DownloadManager* manager, Also here https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:264: DCHECK_NE(CategoryStatus::NOT_PROVIDED, category_status_); Should this check new_status instead of category_status_? Or maybe both? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:268: observer()->OnCategoryStatusChanged(this, provided_category_, new_status); nit: s/new_status/category_status_/ https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:291: NotifyStatusChanged(CategoryStatus::AVAILABLE); Why is this here? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:297: std::set<std::string> new_dismissed_ids; Maybe old_dismissed_ids and retained_dismissed_ids? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:299: for (DownloadItem* item : downloads) { const? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:312: downloads = not_dismissed_downloads; Maybe rename "downloads" to "all_downloads", "not_dismissed_downloads" to just "downloads", and remove this line? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:314: std::nth_element(downloads.begin(), Nice! But since nth_element isn't very common, and IMO the name doesn't really say what it does, maybe this is worth a comment? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:322: if (IsDownloadCompleted(item)) Wouldn't it be easier if the filtering happened before the sorting? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:329: FetchOfflinePagesDownloadsAndSubmitSuggestions(); I was gonna say that this isn't safe since the offline page items might come back before the asset ones. But it looks like FetchAssetsDownloads() is synchronous but FetchOfflinePagesDownloads() isn't. Can we somehow differentiate in the naming to reflect that? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:340: wrapped_item.index = i; Mh, this is a bit yucky. Maybe instead of bool+index, just have two pointers (only one of which will be non-null)? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:353: std::sort(suggestion_items.begin(), suggestion_items.end()); Or in fact: Can the sorting just happen after converting everything to ContentSuggestions? Then we wouldn't need the wrapper at all. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:376: // when the user is online. Do we have a bug for this? If not, could you file one? And then please reference it here, like "crbug.com/12345". This probably applies to some other TODOs as well. (I know from personal experience that otherwise it's super hard to keep track of everything that still needs to be done.) https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:396: ContentSuggestion::ID per_category_id( This is not a per-category-ID. But I'd just inline the "ContentSuggestion::ID(..)" below where you create the ContentSuggestion itself. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:406: base::UTF8ToUTF16(download_item->GetTargetFilePath().BaseName().value())); I think this won't work on all platforms, since paths may be encoded in different ways, not necessarily UTF8. In Windows in particular, they're UTF-16. And FilePath doesn't seem to provide any convenient way to convert to string... :-/ If all this is compiled on Android only, then it'll probably be fine as-is. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:419: std::vector<const content::DownloadItem*>& cache = cached_asset_downloads_; Please avoid non-const references in general. Here in particular, it makes it hard to see that in fact a member variable is getting accessed/modified. Please just spell out cached_asset_downloads_. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:420: if (std::find(cache.begin(), cache.end(), item) != cache.end()) optional: base::ContainsValue might be a bit easier to read https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:427: DCHECK(cache.size() <= kMaxSuggestionsCount); DCHECK_LE? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:429: std::vector<const content::DownloadItem*>::iterator oldest = optional: auto oldest_it = ...? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:432: if (item->GetEndTime() <= (*oldest)->GetEndTime()) On 2016/10/11 08:15:56, vitaliii wrote: > Would using an index here be better? I don't follow - an index for what? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:435: (*oldest) = item; nit: parens not required https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:447: std::vector<offline_pages::OfflinePageItem>& cache = As above: Please spell out cached_offline_page_downloads_ https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:449: for (int i = 0; i < static_cast<int>(cache.size()); ++i) { optional: This might be slightly nicer as an std::find_if with a lambda https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:457: } else { No "else" after "return" https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:458: std::vector<const content::DownloadItem*>& cache = cached_asset_downloads_; Also here: Please spell out https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:474: if (RemoveSuggestionFromCacheIfPresent(suggestion_id)) { if (!Remove...) return; and save a level of indentation. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:506: if (!old_dismissed_ids.count(per_category_id)) I think the dismissed IDs sometimes have the "O"/"D" prefix and sometimes they don't. Actually, if you always store them with the prefix, then there's no need for two separate prefs. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:515: OrderOfflinePagesByMostRecentlyVisitedFirst()); You could use a lambda here https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:574: } else { nit: No "else" after "return" Or maybe just get rid of the two Read methods and do prefs::ReadDismissedIDsFromPrefs(*pref_service_, is_offline ? prefs::kDismissedOfflinePageDownloadSuggestions : prefs::...); ? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:584: } else { Also here https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h (right): https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:5: #ifndef CHROME_BROWSER_NTP_SNIPPETS_DOWNLOADS_DOWNLOAD_SUGGESTIONS_PROVIDER_H_ I don't think this needs a "downloads" subfolder. There isn't that much stuff in c/b/ntp_snippets, and the bookmarks stuff also doesn't have a subfolder. How Android-specific would you say all this is? If it is, then maybe chrome/browser/android/ntp/ would be a better place for it. WDYT? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:22: #include "components/offline_pages/offline_page_item.h" I think a forward declaration is enough here https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:31: namespace ntp_snippets { No namespace; that's used only for the component. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:93: // the service. "the service" being ContentSuggestionsService? Technically the provider doesn't know about that, it just has an observer. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:134: // are no corresponding item is not cached. "if there are no corresponding item is not cached" doesn't parse :) https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:153: // Reads dismissed IDs related to asset downloads from Prefs. nit: prefs, not capitalized. Also below. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:188: // criteria above are cached, otherwise only |kMaxSuggestionsCount|. nit: For the last sentence, I'd just say something like "Contains up to |kMaxSuggestionsCount| items" (or integrate that into one of the previous sentences) https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:199: bool download_manager_ui_enabled_; nit: const? https://codereview.chromium.org/2360263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/downloads/DEPS (right): https://codereview.chromium.org/2360263002/diff/40001/components/ntp_snippets... components/ntp_snippets/downloads/DEPS:2: "+chrome/browser/download", You still need to remove the dependency here.
Hi Marc, would you mind having a look? https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... File components/ntp_snippets/offline_pages/DEPS (right): https://codereview.chromium.org/2360263002/diff/20001/components/ntp_snippets... components/ntp_snippets/offline_pages/DEPS:2: "+components/offline_pages", On 2016/10/11 12:07:39, Marc Treib wrote: > On 2016/10/11 08:15:56, vitaliii wrote: > > On 2016/09/22 13:45:52, Marc Treib wrote: > > > ? > > > > The idea was to add a comma, while we have a DEPS approval. I am not sure > > whether the comma is required there, but if it is, then it is a shame to ask > > offline pages team for a new approval when we need to add new dependency (i.e. > > new line). > > I think we don't need any particular approval for adding that comma - the DEPS > checker should be smart enough for that. Anyway, this is fine. Acknowledged. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:118: base::FeatureList::IsEnabled(chrome::android::kDownloadsUiFeature); On 2016/10/11 12:07:39, Marc Treib wrote: > I think the DownloadSuggestionsProvider can check that itself, so we can save > one ctor parameter. Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/downloads/DEPS (right): https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/DEPS:2: "+grit/components_strings.h", On 2016/10/11 12:07:39, Marc Treib wrote: > Is this needed?! If the provider itself isn't in components, then its strings > shouldn't be there either. Em, how to be with IDS_NTP_DOWNLOAD_SUGGESTIONS_SECTION_HEADER? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:34: const int kMaxSuggestionsCount = 5; On 2016/10/11 12:07:40, Marc Treib wrote: > This should probably be configurable via a variation param - see e.g. the > bookmarks provider for an example. (Can happen in another CL, but maybe add a > TODO) Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:36: const char kOfflinePageDownloadsPrefix[] = "O"; On 2016/10/11 12:07:40, Marc Treib wrote: > Just make these two chars (not array) Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:42: ""); On 2016/10/11 12:07:41, Marc Treib wrote: > s/""/std::string()/ > > But if you're just appending two strings, might as well just do "string1 + > string2". Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:48: {kAssetDownloadsPrefix, base::UintToString(raw_download_id)}, ""); On 2016/10/11 12:07:41, Marc Treib wrote: > Same here Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:53: bool IsOfflinePageID(const ContentSuggestion::ID& suggestion_id) { On 2016/10/11 12:07:40, Marc Treib wrote: > nit: "OfflinePageID" is somewhat overloaded. Maybe "CorrespondsToOfflinePage"? Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:75: client_id.name_space == offline_pages::kNTPSuggestionsNamespace) && On 2016/10/11 12:07:41, Marc Treib wrote: > What's kNTPSuggestionsNamespace? This is for offlined NTP items, e.g. right click on an NTP item and "Save linked page for later". https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:76: base::IsValidGUID(client_id.id); On 2016/10/11 12:07:41, Marc Treib wrote: > Is this needed? Would we ever expect to get an invalid GUID here? It was done like that previously (and the logic was taken from DownloadUIAdapter::IsVisibleInUI). I will replace with DCHECK and experiment. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:79: bool IsDownloadCompleted(const DownloadItem* item) { On 2016/10/11 12:07:40, Marc Treib wrote: > nit: "const DownloadItem&" if it can't be null Wow, I did not know that this is possible with pointers to an abstract class. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:133: return CategoryStatus::NOT_PROVIDED; On 2016/10/11 12:07:40, Marc Treib wrote: > Just do > DCHECK_EQ(provided_category_, category); > return category_status_; > > (no need to handle DCHECK or NOTREACHED failures - treat them like asserts) Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:144: NOTREACHED() << "Unknown category " << category.id(); On 2016/10/11 12:07:40, Marc Treib wrote: > Same here Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:157: StoreDismissedIDsToPrefs(suggestion_id, dismissed_ids); On 2016/10/11 12:07:41, Marc Treib wrote: > nit: I find it a bit confusing to pass in the suggestion_id if you only need to > know offline vs asset - maybe just pass in that bool? Or just call > StoreAsset/OfflinePageDismissedIDsToPrefs directly? I did the former. The latter is less attractive, because this function is called in more than one place. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:176: ClearDismissedSuggestionsForDebugging(provided_category_); On 2016/10/11 12:07:40, Marc Treib wrote: > This will call FetchAllDownloadsAndSubmitSuggestions, which we probably don't > want here. Good point! Actually current cache is completely ignored by all |Fetch.*| functions, so this internal |FetchAllDownloadsAndSubmitSuggestions| call does the job. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:186: FetchAllDownloadsAndSubmitSuggestions(); On 2016/10/11 12:07:41, Marc Treib wrote: > Would we expect this to change anything? The caches should already be up to date > with the "backends", right? Good point. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:217: NotifyStatusChanged(CategoryStatus::AVAILABLE); On 2016/10/11 12:07:41, Marc Treib wrote: > SubmitContentSuggestions will change the status, so I don't think it's required > here. Yes, but it will happen just before the new suggestions are sent. However, I would like to notify the service before we start processing the data (in case some loading indicator is shown). Note that |NotifyStatusChanged| does nothing when called with the same status for the second time. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:227: // other than NOT_PROVIDED, so only notify invalidation in that case. On 2016/10/11 12:07:41, Marc Treib wrote: > Actually, can the status *ever* be NOT_PROVIDED now? Looks like not, but I am not sure. However, I do not like hardcoding this assumption, because it may backfire when we start using NOT_PROVIDED, so I remove this check completely. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:249: void DownloadSuggestionsProvider::OnDownloadOpened(DownloadManager* manager, On 2016/10/11 12:07:40, Marc Treib wrote: > nit: empty line before Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:253: void DownloadSuggestionsProvider::OnDownloadRemoved(DownloadManager* manager, On 2016/10/11 12:07:40, Marc Treib wrote: > Also here Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:264: DCHECK_NE(CategoryStatus::NOT_PROVIDED, category_status_); On 2016/10/11 12:07:40, Marc Treib wrote: > Should this check new_status instead of category_status_? Or maybe both? Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:268: observer()->OnCategoryStatusChanged(this, provided_category_, new_status); On 2016/10/11 12:07:40, Marc Treib wrote: > nit: s/new_status/category_status_/ Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:291: NotifyStatusChanged(CategoryStatus::AVAILABLE); On 2016/10/11 12:07:41, Marc Treib wrote: > Why is this here? Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:297: std::set<std::string> new_dismissed_ids; On 2016/10/11 12:07:40, Marc Treib wrote: > Maybe old_dismissed_ids and retained_dismissed_ids? Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:299: for (DownloadItem* item : downloads) { On 2016/10/11 12:07:41, Marc Treib wrote: > const? Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:312: downloads = not_dismissed_downloads; On 2016/10/11 12:07:40, Marc Treib wrote: > Maybe rename "downloads" to "all_downloads", "not_dismissed_downloads" to just > "downloads", and remove this line? Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:314: std::nth_element(downloads.begin(), On 2016/10/11 12:07:41, Marc Treib wrote: > Nice! > But since nth_element isn't very common, and IMO the name doesn't really say > what it does, maybe this is worth a comment? Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:322: if (IsDownloadCompleted(item)) On 2016/10/11 12:07:41, Marc Treib wrote: > Wouldn't it be easier if the filtering happened before the sorting? Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:329: FetchOfflinePagesDownloadsAndSubmitSuggestions(); On 2016/10/11 12:07:41, Marc Treib wrote: > I was gonna say that this isn't safe since the offline page items might come > back before the asset ones. But it looks like FetchAssetsDownloads() is > synchronous but FetchOfflinePagesDownloads() isn't. Can we somehow differentiate > in the naming to reflect that? Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:340: wrapped_item.index = i; On 2016/10/11 12:07:41, Marc Treib wrote: > Mh, this is a bit yucky. Maybe instead of bool+index, just have two pointers > (only one of which will be non-null)? Acknowledged. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:353: std::sort(suggestion_items.begin(), suggestion_items.end()); On 2016/10/11 12:07:40, Marc Treib wrote: > Or in fact: Can the sorting just happen after converting everything to > ContentSuggestions? Then we wouldn't need the wrapper at all. Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:376: // when the user is online. On 2016/10/11 12:07:40, Marc Treib wrote: > Do we have a bug for this? If not, could you file one? And then please reference > it here, like "crbug.com/12345". This probably applies to some other TODOs as > well. > (I know from personal experience that otherwise it's super hard to keep track of > everything that still needs to be done.) Done. Why do not you like https://cs.chromium.org/search/?q=TODO+file:%5Esrc/components/ntp_snippets/&s... ? :) https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:396: ContentSuggestion::ID per_category_id( On 2016/10/11 12:07:40, Marc Treib wrote: > This is not a per-category-ID. But I'd just inline the > "ContentSuggestion::ID(..)" below where you create the ContentSuggestion itself. Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:406: base::UTF8ToUTF16(download_item->GetTargetFilePath().BaseName().value())); On 2016/10/11 12:07:40, Marc Treib wrote: > I think this won't work on all platforms, since paths may be encoded in > different ways, not necessarily UTF8. In Windows in particular, they're UTF-16. > And FilePath doesn't seem to provide any convenient way to convert to string... > :-/ > If all this is compiled on Android only, then it'll probably be fine as-is. This is Anroid only for now, added DCHECK for future. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:419: std::vector<const content::DownloadItem*>& cache = cached_asset_downloads_; On 2016/10/11 12:07:41, Marc Treib wrote: > Please avoid non-const references in general. > Here in particular, it makes it hard to see that in fact a member variable is > getting accessed/modified. Please just spell out cached_asset_downloads_. Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:420: if (std::find(cache.begin(), cache.end(), item) != cache.end()) On 2016/10/11 12:07:41, Marc Treib wrote: > optional: base::ContainsValue might be a bit easier to read Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:427: DCHECK(cache.size() <= kMaxSuggestionsCount); On 2016/10/11 12:07:40, Marc Treib wrote: > DCHECK_LE? Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:429: std::vector<const content::DownloadItem*>::iterator oldest = On 2016/10/11 12:07:40, Marc Treib wrote: > optional: auto oldest_it = ...? Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:432: if (item->GetEndTime() <= (*oldest)->GetEndTime()) On 2016/10/11 12:07:41, Marc Treib wrote: > On 2016/10/11 08:15:56, vitaliii wrote: > > Would using an index here be better? > > I don't follow - an index for what? i.e. int i = oldest - cached_asset_downloads_.begin(); cached_asset_downloads_[i] = item; https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:435: (*oldest) = item; On 2016/10/11 12:07:41, Marc Treib wrote: > nit: parens not required Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:447: std::vector<offline_pages::OfflinePageItem>& cache = On 2016/10/11 12:07:41, Marc Treib wrote: > As above: Please spell out cached_offline_page_downloads_ Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:449: for (int i = 0; i < static_cast<int>(cache.size()); ++i) { On 2016/10/11 12:07:40, Marc Treib wrote: > optional: This might be slightly nicer as an std::find_if with a lambda Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:457: } else { On 2016/10/11 12:07:40, Marc Treib wrote: > No "else" after "return" Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:458: std::vector<const content::DownloadItem*>& cache = cached_asset_downloads_; On 2016/10/11 12:07:40, Marc Treib wrote: > Also here: Please spell out Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:474: if (RemoveSuggestionFromCacheIfPresent(suggestion_id)) { On 2016/10/11 12:07:41, Marc Treib wrote: > if (!Remove...) > return; > and save a level of indentation. Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:506: if (!old_dismissed_ids.count(per_category_id)) On 2016/10/11 12:07:41, Marc Treib wrote: > I think the dismissed IDs sometimes have the "O"/"D" prefix and sometimes they > don't. > Actually, if you always store them with the prefix, then there's no need for two > separate prefs. For the former - I do not think so, I added DCHECK. For the latter, then pruning becomes much less trivial, however, it should be manageable. So should I have one pref or store ids without prefixes? Also regarding one pref, I am not sure what happens if we get two updates from different sources at the same time. For example, let's say we have an update for offline pages and we read |old_dismissed_ids|. Just after this we have another update, but for asset downloads and read same |old_dismissed_ids|, prune them and store. Then we get back to offline pages, prune their ids (missing asset downloads pruning) and store (rewriting asset downloads pruning). Can this be the case? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:515: OrderOfflinePagesByMostRecentlyVisitedFirst()); On 2016/10/11 12:07:41, Marc Treib wrote: > You could use a lambda here Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:574: } else { On 2016/10/11 12:07:40, Marc Treib wrote: > nit: No "else" after "return" > > Or maybe just get rid of the two Read methods and do > prefs::ReadDismissedIDsFromPrefs(*pref_service_, is_offline ? > prefs::kDismissedOfflinePageDownloadSuggestions : prefs::...); > ? Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:584: } else { On 2016/10/11 12:07:39, Marc Treib wrote: > Also here Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h (right): https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:5: #ifndef CHROME_BROWSER_NTP_SNIPPETS_DOWNLOADS_DOWNLOAD_SUGGESTIONS_PROVIDER_H_ On 2016/10/11 12:07:42, Marc Treib wrote: > I don't think this needs a "downloads" subfolder. There isn't that much stuff in > c/b/ntp_snippets, and the bookmarks stuff also doesn't have a subfolder. > > How Android-specific would you say all this is? If it is, then maybe > chrome/browser/android/ntp/ would be a better place for it. WDYT? As it was discussed, this provider is not by design exclusive to android, to it stays here. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:22: #include "components/offline_pages/offline_page_item.h" On 2016/10/11 12:07:42, Marc Treib wrote: > I think a forward declaration is enough here Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:31: namespace ntp_snippets { On 2016/10/11 12:07:42, Marc Treib wrote: > No namespace; that's used only for the component. Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:93: // the service. On 2016/10/11 12:07:41, Marc Treib wrote: > "the service" being ContentSuggestionsService? Technically the provider doesn't > know about that, it just has an observer. Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:134: // are no corresponding item is not cached. On 2016/10/11 12:07:42, Marc Treib wrote: > "if there are no corresponding item is not cached" doesn't parse :) Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:139: // corresponding downloads to update the cache if there are any not in cache. Can you parse this one? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:153: // Reads dismissed IDs related to asset downloads from Prefs. On 2016/10/11 12:07:41, Marc Treib wrote: > nit: prefs, not capitalized. Also below. Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:188: // criteria above are cached, otherwise only |kMaxSuggestionsCount|. On 2016/10/11 12:07:42, Marc Treib wrote: > nit: For the last sentence, I'd just say something like "Contains up to > |kMaxSuggestionsCount| items" (or integrate that into one of the previous > sentences) But the suggested sentence does not say that there must be |kMaxSuggestionsCount| items cached if the model has at least |kMaxSuggestionsCount| items. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:199: bool download_manager_ui_enabled_; On 2016/10/11 12:07:41, Marc Treib wrote: > nit: const? Done. https://codereview.chromium.org/2360263002/diff/40001/components/ntp_snippets... File components/ntp_snippets/downloads/DEPS (right): https://codereview.chromium.org/2360263002/diff/40001/components/ntp_snippets... components/ntp_snippets/downloads/DEPS:2: "+chrome/browser/download", On 2016/10/11 12:07:42, Marc Treib wrote: > You still need to remove the dependency here. Done.
https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/downloads/DEPS (right): https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/DEPS:2: "+grit/components_strings.h", On 2016/10/13 08:43:04, vitaliii wrote: > On 2016/10/11 12:07:39, Marc Treib wrote: > > Is this needed?! If the provider itself isn't in components, then its strings > > shouldn't be there either. > > Em, how to be with IDS_NTP_DOWNLOAD_SUGGESTIONS_SECTION_HEADER? You could move that string into chrome/app/generated_resources.grd, where most of Chrome's strings are. No need for the string to be in components when the code that uses it isn't. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:176: ClearDismissedSuggestionsForDebugging(provided_category_); On 2016/10/13 08:43:05, vitaliii wrote: > On 2016/10/11 12:07:40, Marc Treib wrote: > > This will call FetchAllDownloadsAndSubmitSuggestions, which we probably don't > > want here. > > Good point! Actually current cache is completely ignored by all |Fetch.*| > functions, > so this internal |FetchAllDownloadsAndSubmitSuggestions| call does the job. Hm, but that is async, right? So maybe it's best to keep the .clear() calls, and add a comment that ClearDismissed...() will trigger a re-fetch. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:217: NotifyStatusChanged(CategoryStatus::AVAILABLE); On 2016/10/13 08:43:07, vitaliii wrote: > On 2016/10/11 12:07:41, Marc Treib wrote: > > SubmitContentSuggestions will change the status, so I don't think it's > required > > here. > > Yes, but it will happen just before the new suggestions are sent. > However, I would like to notify the service before we start processing the data > (in case some loading indicator is shown). > Note that |NotifyStatusChanged| does nothing when called with the same status > for the second time. But all this happens synchronously, so the UI doesn't get a chance to do anything at all in the meantime. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:340: wrapped_item.index = i; On 2016/10/13 08:43:05, vitaliii wrote: > On 2016/10/11 12:07:41, Marc Treib wrote: > > Mh, this is a bit yucky. Maybe instead of bool+index, just have two pointers > > (only one of which will be non-null)? > > Acknowledged. ...this reply confused me ;P "Obsolete, SuggestionItemWrapper doesn't exist anymore" https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:376: // when the user is online. On 2016/10/13 08:43:06, vitaliii wrote: > On 2016/10/11 12:07:40, Marc Treib wrote: > > Do we have a bug for this? If not, could you file one? And then please > reference > > it here, like "crbug.com/12345". This probably applies to some other TODOs as > > well. > > (I know from personal experience that otherwise it's super hard to keep track > of > > everything that still needs to be done.) > > Done. > Why do not you like > https://cs.chromium.org/search/?q=TODO+file:%5Esrc/components/ntp_snippets/&s... > ? :) TODOs in the code aren't a good issue tracking system. E.g. there's no prioritization or timing, and if Patrick or Michael want to know what still needs to be done, they won't find it here. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:432: if (item->GetEndTime() <= (*oldest)->GetEndTime()) On 2016/10/13 08:43:06, vitaliii wrote: > On 2016/10/11 12:07:41, Marc Treib wrote: > > On 2016/10/11 08:15:56, vitaliii wrote: > > > Would using an index here be better? > > > > I don't follow - an index for what? > > i.e. > int i = oldest - cached_asset_downloads_.begin(); > cached_asset_downloads_[i] = item; Ah - no, what you have is fine. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:506: if (!old_dismissed_ids.count(per_category_id)) On 2016/10/13 08:43:05, vitaliii wrote: > On 2016/10/11 12:07:41, Marc Treib wrote: > > I think the dismissed IDs sometimes have the "O"/"D" prefix and sometimes they > > don't. > > Actually, if you always store them with the prefix, then there's no need for > two > > separate prefs. > For the former - I do not think so, I added DCHECK. I looked over all the code, and it looks like you do use them with the prefix always. I guess I just got confused by the many kinds of IDs around here... > For the latter, then pruning becomes much less trivial, however, it should be > manageable. But it makes everything else (adding or checking dismissed IDs) easier :) > So should I have one pref or store ids without prefixes? I think I'd prefer one. > Also regarding one pref, I am not sure what happens if we get two updates from > different sources at the same time. For example, let's say we have an update for > offline pages and we read |old_dismissed_ids|. Just after this we have another > update, but for asset downloads and read same |old_dismissed_ids|, prune them > and store. Then we get back to offline pages, prune their ids (missing asset > downloads pruning) and store (rewriting asset downloads pruning). Can this be > the case? Everything in this class happens on one thread. So unless you explicitly wait for something asynchronous between reading the IDs and writing them back, it's all good. There's no such thing as "at the same time" :) https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h (right): https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:188: // criteria above are cached, otherwise only |kMaxSuggestionsCount|. On 2016/10/13 08:43:07, vitaliii wrote: > On 2016/10/11 12:07:42, Marc Treib wrote: > > nit: For the last sentence, I'd just say something like "Contains up to > > |kMaxSuggestionsCount| items" (or integrate that into one of the previous > > sentences) > > But the suggested sentence does not say that there must be > |kMaxSuggestionsCount| items cached if the model has at least > |kMaxSuggestionsCount| items. ...eh, fair enough. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:73: DCHECK((client_id.name_space == offline_pages::kAsyncNamespace || You can DCHECK the IsValidGUID, but not the namespace part. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:94: struct SuggestionItemWrapper { Not used anymore :) https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:177: // Ignored. Maybe a comment on why it's safe to ignore? https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:237: NotifyStatusChanged(CategoryStatus::AVAILABLE); Not required, SubmitContentSuggestions will do that (synchronously!) https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:253: // ones. Hm, so on startup we'll notify our observer once per download? Not too great, but probably nothing we can do about it. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:298: AsyncronouslyFetchOfflinePagesDownloadsAndSubmitSuggestions() { Add a "bool notify" parameter and merge with AsyncronouslyFetchOfflinePagesDownloads? Then also add that parameter to ProcessAllOfflinePages, then you can just forward it there. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:344: cached_asset_downloads_ = std::move(downloads); You could put the items directly into cached_asset_downloads_ and remove the downloads variable. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:375: suggestions.pop_back(); I think .erase would also work, something like suggestions.erase(suggestions.begin() + kMaxSuggestionsCount, suggestions.end()); (but it's not any prettier, so.. eh) https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:428: return false; Hm, is this always safe? Could the download have changed in the meantime (updated title or whatever)? https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:550: const std::string& per_category_id) { s/per_category_id/id_in_category/ to match the header (or in any case, use the same term everywhere - ContentSuggestion::ID itself uses id_within_category, probably that'd be best) https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.h:107: void AsyncronouslyFetchOfflinePagesDownloads(); s/Asyncronously/Asynchronously/ (missing an "h") https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.h:116: void SyncronouslyFetchAssetsDownloads(); nit: "Synchronously" isn't needed, that's the default assumption :) Maybe just "GetAssetDownloads", or "UpdateAssetDownloadsCache"? https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.h:159: void ProcessAllOfflinePages( Hm. Maybe "UpdateOfflinePagesCache"? That's the main thing it does, and "process" just doesn't tell me anything. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:44: void PrintTo(const ContentSuggestion& value, std::ostream* os) { Is this used anywhere? https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:82: class FakeDownloadItem : public DownloadItem { Since this is so huge, maybe worth putting it into a separate file? https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:347: // The members bellow are to be returned by methods, which return links. s/bellow/below/ s/links/pointers/ ? I assume that's what you mean? https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:354: std::unique_ptr<FakeDownloadItem> CreateDummyAssetDownload(int id) { nit: Please move this down to the other CreateDummyAssetDownload* functions. (or move those here) https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:458: void FireDownloadRemoved(FakeDownloadItem* item) { nit: either FireOnDownloadRemoved (add "On"), or remove the "On" in FireOnDownloadCreated and FireOnDownloadsCreated https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:514: FireOnDownloadCreated(asset_downloads[0].get()); Shouldn't the observer already get a call from this? Probably best to make the observer a StrictMock, so that we make sure we don't get any extra unexpected calls. (I think right now the tests will produce a lot of "uninteresting mock function call" warnings?!) https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:617: Mock::VerifyAndClearExpectations(observer()); Not needed https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:631: Mock::VerifyAndClearExpectations(observer()); nit: This and the EXPECT_CALL above aren't really necessary here https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:644: CreateDummyAssetDownloads({1, 2, 3, 4, 5, 6}); Took me a while to see why you need 6 :D Maybe add a comment that this is one more than kMaxSuggestions? Or expose that number, and programmatically add one more than that? https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:649: GetDummySuggestionId(1, /*is_offline_page=*/true)); Wait, this dismisses an offline page - does the test pass?! Then something is seriously wrong. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:650: Mock::VerifyAndClearExpectations(observer()); Also here. Generally, it's better to avoid VerifyAndClearExpectations when possible. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:661: HasUrl("file:///folder/file6.mhtml")))); Maybe also verify before dismissing that items 1-5 get reported? It might randomly happen that you'd get 2-6 even before dismissing, since none of them have a date. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:686: FireOnDownloadsCreated(downloads_manager()->items()); Also here: Verify that actually items 1-5 get reported? https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:717: EXPECT_THAT(ReadDismissedIDsFromPrefs(/*for_offline_pages=*/true), SizeIs(3)); IMO the prefs are an implementation detail that doesn't really need to get tested. You could test the behavior instead, using GetDismissedSuggestions?
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
Patchset #5 (id:100001) has been deleted
Patchset #4 (id:80001) has been deleted
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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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 #5 (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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Marc, could you have a look? https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/downloads/DEPS (right): https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/DEPS:2: "+grit/components_strings.h", On 2016/10/13 12:11:25, Marc Treib wrote: > On 2016/10/13 08:43:04, vitaliii wrote: > > On 2016/10/11 12:07:39, Marc Treib wrote: > > > Is this needed?! If the provider itself isn't in components, then its > strings > > > shouldn't be there either. > > > > Em, how to be with IDS_NTP_DOWNLOAD_SUGGESTIONS_SECTION_HEADER? > > You could move that string into chrome/app/generated_resources.grd, where most > of Chrome's strings are. No need for the string to be in components when the > code that uses it isn't. Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:176: ClearDismissedSuggestionsForDebugging(provided_category_); On 2016/10/13 12:11:25, Marc Treib wrote: > On 2016/10/13 08:43:05, vitaliii wrote: > > On 2016/10/11 12:07:40, Marc Treib wrote: > > > This will call FetchAllDownloadsAndSubmitSuggestions, which we probably > don't > > > want here. > > > > Good point! Actually current cache is completely ignored by all |Fetch.*| > > functions, > > so this internal |FetchAllDownloadsAndSubmitSuggestions| call does the job. > > Hm, but that is async, right? So maybe it's best to keep the .clear() calls, and > add a comment that ClearDismissed...() will trigger a re-fetch. Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:217: NotifyStatusChanged(CategoryStatus::AVAILABLE); On 2016/10/13 12:11:25, Marc Treib wrote: > On 2016/10/13 08:43:07, vitaliii wrote: > > On 2016/10/11 12:07:41, Marc Treib wrote: > > > SubmitContentSuggestions will change the status, so I don't think it's > > required > > > here. > > > > Yes, but it will happen just before the new suggestions are sent. > > However, I would like to notify the service before we start processing the > data > > (in case some loading indicator is shown). > > Note that |NotifyStatusChanged| does nothing when called with the same status > > for the second time. > > But all this happens synchronously, so the UI doesn't get a chance to do > anything at all in the meantime. Done. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:340: wrapped_item.index = i; On 2016/10/13 12:11:25, Marc Treib wrote: > On 2016/10/13 08:43:05, vitaliii wrote: > > On 2016/10/11 12:07:41, Marc Treib wrote: > > > Mh, this is a bit yucky. Maybe instead of bool+index, just have two pointers > > > (only one of which will be non-null)? > > > > Acknowledged. > > ...this reply confused me ;P > "Obsolete, SuggestionItemWrapper doesn't exist anymore" I've got it. Sorry about that. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:376: // when the user is online. On 2016/10/13 12:11:25, Marc Treib wrote: > On 2016/10/13 08:43:06, vitaliii wrote: > > On 2016/10/11 12:07:40, Marc Treib wrote: > > > Do we have a bug for this? If not, could you file one? And then please > > reference > > > it here, like "crbug.com/12345". This probably applies to some other TODOs > as > > > well. > > > (I know from personal experience that otherwise it's super hard to keep > track > > of > > > everything that still needs to be done.) > > > > Done. > > Why do not you like > > > https://cs.chromium.org/search/?q=TODO+file:%5Esrc/components/ntp_snippets/&s... > > ? :) > > TODOs in the code aren't a good issue tracking system. E.g. there's no > prioritization or timing, and if Patrick or Michael want to know what still > needs to be done, they won't find it here. I see, good point. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:432: if (item->GetEndTime() <= (*oldest)->GetEndTime()) On 2016/10/13 12:11:25, Marc Treib wrote: > On 2016/10/13 08:43:06, vitaliii wrote: > > On 2016/10/11 12:07:41, Marc Treib wrote: > > > On 2016/10/11 08:15:56, vitaliii wrote: > > > > Would using an index here be better? > > > > > > I don't follow - an index for what? > > > > i.e. > > int i = oldest - cached_asset_downloads_.begin(); > > cached_asset_downloads_[i] = item; > > Ah - no, what you have is fine. Acknowledged. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc:506: if (!old_dismissed_ids.count(per_category_id)) On 2016/10/13 12:11:25, Marc Treib wrote: > On 2016/10/13 08:43:05, vitaliii wrote: > > On 2016/10/11 12:07:41, Marc Treib wrote: > > > I think the dismissed IDs sometimes have the "O"/"D" prefix and sometimes > they > > > don't. > > > Actually, if you always store them with the prefix, then there's no need for > > two > > > separate prefs. > > For the former - I do not think so, I added DCHECK. > > I looked over all the code, and it looks like you do use them with the prefix > always. I guess I just got confused by the many kinds of IDs around here... > > > For the latter, then pruning becomes much less trivial, however, it should be > > manageable. > > But it makes everything else (adding or checking dismissed IDs) easier :) > > > So should I have one pref or store ids without prefixes? > > I think I'd prefer one. > > > Also regarding one pref, I am not sure what happens if we get two updates from > > different sources at the same time. For example, let's say we have an update > for > > offline pages and we read |old_dismissed_ids|. Just after this we have another > > update, but for asset downloads and read same |old_dismissed_ids|, prune them > > and store. Then we get back to offline pages, prune their ids (missing asset > > downloads pruning) and store (rewriting asset downloads pruning). Can this be > > the case? > > Everything in this class happens on one thread. So unless you explicitly wait > for something asynchronous between reading the IDs and writing them back, it's > all good. There's no such thing as "at the same time" :) Let's stick with this approach for now and then I will have a look whether there is some neat way to prune IDs in one set. I added a TODO and a bug. https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h (right): https://codereview.chromium.org/2360263002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h:188: // criteria above are cached, otherwise only |kMaxSuggestionsCount|. On 2016/10/13 12:11:25, Marc Treib wrote: > On 2016/10/13 08:43:07, vitaliii wrote: > > On 2016/10/11 12:07:42, Marc Treib wrote: > > > nit: For the last sentence, I'd just say something like "Contains up to > > > |kMaxSuggestionsCount| items" (or integrate that into one of the previous > > > sentences) > > > > But the suggested sentence does not say that there must be > > |kMaxSuggestionsCount| items cached if the model has at least > > |kMaxSuggestionsCount| items. > > ...eh, fair enough. Acknowledged. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:73: DCHECK((client_id.name_space == offline_pages::kAsyncNamespace || On 2016/10/13 12:11:25, Marc Treib wrote: > You can DCHECK the IsValidGUID, but not the namespace part. Actually I DCHECK'ed on the device, so no need for it here. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:94: struct SuggestionItemWrapper { On 2016/10/13 12:11:25, Marc Treib wrote: > Not used anymore :) Done. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:177: // Ignored. On 2016/10/13 12:11:26, Marc Treib wrote: > Maybe a comment on why it's safe to ignore? Done. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:237: NotifyStatusChanged(CategoryStatus::AVAILABLE); On 2016/10/13 12:11:25, Marc Treib wrote: > Not required, SubmitContentSuggestions will do that (synchronously!) Done. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:253: // ones. On 2016/10/13 12:11:25, Marc Treib wrote: > Hm, so on startup we'll notify our observer once per download? Not too great, > but probably nothing we can do about it. Yeap unless DownloadManager changes their Observer interface (e.g. by adding OnStartupCompleted). https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:298: AsyncronouslyFetchOfflinePagesDownloadsAndSubmitSuggestions() { On 2016/10/13 12:11:25, Marc Treib wrote: > Add a "bool notify" parameter and merge with > AsyncronouslyFetchOfflinePagesDownloads? Then also add that parameter to > ProcessAllOfflinePages, then you can just forward it there. Done. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:344: cached_asset_downloads_ = std::move(downloads); On 2016/10/13 12:11:25, Marc Treib wrote: > You could put the items directly into cached_asset_downloads_ and remove the > downloads variable. Done. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:375: suggestions.pop_back(); On 2016/10/13 12:11:25, Marc Treib wrote: > I think .erase would also work, something like > suggestions.erase(suggestions.begin() + kMaxSuggestionsCount, > suggestions.end()); > (but it's not any prettier, so.. eh) Acknowledged. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:428: return false; On 2016/10/13 12:11:25, Marc Treib wrote: > Hm, is this always safe? Could the download have changed in the meantime > (updated title or whatever)? Good point. I leave this check here, because the function is used in |OnDownloadCreated|, but I changed |OnDownloadUpdated|. As far as I saw in DownloadItem's code, something we show should not change, but just in case I submit suggestions if an item from cache has been updated. Also I remove an item if its file gets removed. Thanks! https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:550: const std::string& per_category_id) { On 2016/10/13 12:11:25, Marc Treib wrote: > s/per_category_id/id_in_category/ to match the header (or in any case, use the > same term everywhere - ContentSuggestion::ID itself uses id_within_category, > probably that'd be best) Done. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.h:107: void AsyncronouslyFetchOfflinePagesDownloads(); On 2016/10/13 12:11:26, Marc Treib wrote: > s/Asyncronously/Asynchronously/ > (missing an "h") Done. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.h:116: void SyncronouslyFetchAssetsDownloads(); On 2016/10/13 12:11:26, Marc Treib wrote: > nit: "Synchronously" isn't needed, that's the default assumption :) > Maybe just "GetAssetDownloads", or "UpdateAssetDownloadsCache"? I would prefer Fetch...(), so that it is similar to |AsyncronouslyFetchOfflinePagesDownloads|. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.h:159: void ProcessAllOfflinePages( On 2016/10/13 12:11:26, Marc Treib wrote: > Hm. Maybe "UpdateOfflinePagesCache"? That's the main thing it does, and > "process" just doesn't tell me anything. Done. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:44: void PrintTo(const ContentSuggestion& value, std::ostream* os) { On 2016/10/13 12:11:26, Marc Treib wrote: > Is this used anywhere? Yes, the testing framework uses this when printing a ContentSuggestion. I added a comment. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:82: class FakeDownloadItem : public DownloadItem { On 2016/10/13 12:11:26, Marc Treib wrote: > Since this is so huge, maybe worth putting it into a separate file? Done. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:347: // The members bellow are to be returned by methods, which return links. On 2016/10/13 12:11:26, Marc Treib wrote: > s/bellow/below/ > s/links/pointers/ ? I assume that's what you mean? First - yes. Second - no, I meant by reference. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:354: std::unique_ptr<FakeDownloadItem> CreateDummyAssetDownload(int id) { On 2016/10/13 12:11:26, Marc Treib wrote: > nit: Please move this down to the other CreateDummyAssetDownload* functions. (or > move those here) Done. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:458: void FireDownloadRemoved(FakeDownloadItem* item) { On 2016/10/13 12:11:26, Marc Treib wrote: > nit: either FireOnDownloadRemoved (add "On"), or remove the "On" in > FireOnDownloadCreated and FireOnDownloadsCreated Done. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:514: FireOnDownloadCreated(asset_downloads[0].get()); On 2016/10/13 12:11:26, Marc Treib wrote: > Shouldn't the observer already get a call from this? > Probably best to make the observer a StrictMock, so that we make sure we don't > get any extra unexpected calls. (I think right now the tests will produce a lot > of "uninteresting mock function call" warnings?!) Yes, it should. I use a StrictMock now. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:617: Mock::VerifyAndClearExpectations(observer()); On 2016/10/13 12:11:26, Marc Treib wrote: > Not needed Done. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:631: Mock::VerifyAndClearExpectations(observer()); On 2016/10/13 12:11:26, Marc Treib wrote: > nit: This and the EXPECT_CALL above aren't really necessary here Done. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:644: CreateDummyAssetDownloads({1, 2, 3, 4, 5, 6}); On 2016/10/13 12:11:26, Marc Treib wrote: > Took me a while to see why you need 6 :D > Maybe add a comment that this is one more than kMaxSuggestions? Or expose that > number, and programmatically add one more than that? Done. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:649: GetDummySuggestionId(1, /*is_offline_page=*/true)); On 2016/10/13 12:11:26, Marc Treib wrote: > Wait, this dismisses an offline page - does the test pass?! Then something is > seriously wrong. {2, 3, 4, 5, 6} were shown by the provider. I changed |CreateDummyAssetDownloads| to set up items' time, such that the first items will be shown first. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:650: Mock::VerifyAndClearExpectations(observer()); On 2016/10/13 12:11:26, Marc Treib wrote: > Also here. Generally, it's better to avoid VerifyAndClearExpectations when > possible. Done. Why? https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:661: HasUrl("file:///folder/file6.mhtml")))); On 2016/10/13 12:11:26, Marc Treib wrote: > Maybe also verify before dismissing that items 1-5 get reported? It might > randomly happen that you'd get 2-6 even before dismissing, since none of them > have a date. Done. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:686: FireOnDownloadsCreated(downloads_manager()->items()); On 2016/10/13 12:11:26, Marc Treib wrote: > Also here: Verify that actually items 1-5 get reported? Done. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:717: EXPECT_THAT(ReadDismissedIDsFromPrefs(/*for_offline_pages=*/true), SizeIs(3)); On 2016/10/13 12:11:26, Marc Treib wrote: > IMO the prefs are an implementation detail that doesn't really need to get > tested. You could test the behavior instead, using GetDismissedSuggestions? I use GetDismissedSuggestions now. However, this makes it a bit trickier, because GetDismissedSuggestions fetches all data.
Nice! A few more comments, mostly nits. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:73: DCHECK((client_id.name_space == offline_pages::kAsyncNamespace || On 2016/10/15 18:36:30, vitaliii wrote: > On 2016/10/13 12:11:25, Marc Treib wrote: > > You can DCHECK the IsValidGUID, but not the namespace part. > > Actually I DCHECK'ed on the device, so no need for it here. "on the device"? (Not DCHECKing here is fine, I just don't understand your comment) https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:44: void PrintTo(const ContentSuggestion& value, std::ostream* os) { On 2016/10/15 18:36:31, vitaliii wrote: > On 2016/10/13 12:11:26, Marc Treib wrote: > > Is this used anywhere? > > Yes, the testing framework uses this when printing a ContentSuggestion. > I added a comment. I think defining an "operator<<" also does the trick, and might look a bit less "magic"? Like ostream& operator<<(ostream& s, const ContentSuggestion& value) https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:650: Mock::VerifyAndClearExpectations(observer()); On 2016/10/15 18:36:30, vitaliii wrote: > On 2016/10/13 12:11:26, Marc Treib wrote: > > Also here. Generally, it's better to avoid VerifyAndClearExpectations when > > possible. > > Done. Why? It's a hint that the test is testing more than one thing, and should probably be split up. (Tim feels more strongly about this than I do) https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:109: download_manager_->AddObserver(this); Is this always non-null? https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:110: // The asset downloads are not fetched, since for each of them nit: I'd formulate this as something like No need to explicitly fetch the asset downloads, ... (to make clear that they are in fact fetched, just in a different way) https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:264: download_manager_ = nullptr; Should you also unregister all the item observers here? https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:318: : &DownloadSuggestionsProvider::UpdateOfflinePagesCache, Add the "bool notify" parameter also to UpdateOfflinePagesCache? Then you can just bind that here, and OfflinePageModelChanged will just call UpdateOfflinePagesCache(true). (I find calling OfflinePageModelChanged here a bit weird) https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:427: // #if block there). ...we don't have some helper for this?! Ugh... I'd move the conversion into a helper method (in the anonymous namespace above), so that a) it's localized, and b) we'd get a compiler error ("function doesn't return anything") if any platform isn't handled, since it's not very obvious that POSIX+WIN covers everything. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:628: // TODO(vitaliii): Store one set instead of two. See crbug.com/656024. For this kind of thing (internal cleanup that has no user-visible impact), it's not required to file a bug. (It's fine to file one if you want to, but you don't *have* to.) https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.h:156: // the moment), prunes dismissed IDs and updates internal cache. nit: I don't understand the part in parens - what do you mean by "available"? How would they not be available? https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:53: void PrintTo(const CategoryStatus& value, std::ostream* os) { If multiple tests need this, it's also okay to define an operator<< in category_status.h (with a comment that it's only for testing). I think so far, tests have just cast this to int for output, but clearly this makes for much nicer messages. (Just FYI, no action required :) https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:201: void IgnoreOnCategoryStatusChanged() { nit: This sounds like it ignores all changes, but it ignores only AVAILABLE[_LOADING]. Not sure what a good name would be though... IgnoreOnCategoryStatusChangedToAvailable? https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:216: scoped_refptr<OfflinePageProxy> proxy( DCHECK(!provider_) ? https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:225: void DestroyProvider() { provider_.release(); } This will leak memory - it gives up ownership and returns the raw pointer without deleting it. You probably want provider_ = nullptr; or provider_.reset(); https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:233: DCHECK(provider_.get()); nit: I think the ".get()" isn't required https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:263: base::Bind(&CaptureDismissedSuggestions, &dismissed_suggestions)); Does this always return synchronously? If so, that's worth a comment. (Would also be nice to check it, but that might be a bit complicated.) If it's *not* always synchronous, then you'll have to wait until it finishes, using base::RunLoop. (You can ask me for details if necessary) https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:275: StrictMock<MockContentSuggestionsProviderObserver>* observer() { I think just returning MockContentSuggestionsProviderObserver* (no StrictMock) should work too? https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:406: EXPECT_CALL(*observer(), optional: If you expect calls in a particular order, you can use testing::InSequence. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:429: IgnoreOnSuggestionInvalidated(); Does this do anything here? If so, worth a comment. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:636: // |OnDownloadItemDestroyed| is called from its destructor. nit: s/its/|removed_item|'s/ https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:639: IgnoreOnSuggestionInvalidated(); Is this necessary? https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:689: FireOfflinePageModelChanged(offline_pages_model()->items()); Is the FireOfflinePageModelChanged needed here? Shouldn't destroying removed_item above already result in this call? https://codereview.chromium.org/2360263002/diff/180001/components/ntp_snippet... File components/ntp_snippets/BUILD.gn (right): https://codereview.chromium.org/2360263002/diff/180001/components/ntp_snippet... components/ntp_snippets/BUILD.gn:142: "//components/offline_pages:offline_pages", nit: ":offline_pages" isn't required, just "//components/offline_pages" is enough. https://codereview.chromium.org/2360263002/diff/180001/components/ntp_snippet... components/ntp_snippets/BUILD.gn:170: "//base:base", nit: Also here, ":base" isn't required
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...
Hello Marc, could you have a look? In case there is not much to improve could you please pass the review to Bernhard (mainly for owners approval for browser_prefs.cc)? Thanks! https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:73: DCHECK((client_id.name_space == offline_pages::kAsyncNamespace || On 2016/10/17 10:18:40, Marc Treib wrote: > On 2016/10/15 18:36:30, vitaliii wrote: > > On 2016/10/13 12:11:25, Marc Treib wrote: > > > You can DCHECK the IsValidGUID, but not the namespace part. > > > > Actually I DCHECK'ed on the device, so no need for it here. > > "on the device"? (Not DCHECKing here is fine, I just don't understand your > comment) I meant that I run the app on the device while having the DCHECK and it did not fire. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:44: void PrintTo(const ContentSuggestion& value, std::ostream* os) { On 2016/10/17 10:18:40, Marc Treib wrote: > On 2016/10/15 18:36:31, vitaliii wrote: > > On 2016/10/13 12:11:26, Marc Treib wrote: > > > Is this used anywhere? > > > > Yes, the testing framework uses this when printing a ContentSuggestion. > > I added a comment. > > I think defining an "operator<<" also does the trick, and might look a bit less > "magic"? Like > ostream& operator<<(ostream& s, const ContentSuggestion& value) Done. https://codereview.chromium.org/2360263002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:650: Mock::VerifyAndClearExpectations(observer()); On 2016/10/17 10:18:40, Marc Treib wrote: > On 2016/10/15 18:36:30, vitaliii wrote: > > On 2016/10/13 12:11:26, Marc Treib wrote: > > > Also here. Generally, it's better to avoid VerifyAndClearExpectations when > > > possible. > > > > Done. Why? > > It's a hint that the test is testing more than one thing, and should probably be > split up. (Tim feels more strongly about this than I do) I see, good point! https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:109: download_manager_->AddObserver(this); On 2016/10/17 10:18:40, Marc Treib wrote: > Is this always non-null? Done. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:110: // The asset downloads are not fetched, since for each of them On 2016/10/17 10:18:41, Marc Treib wrote: > nit: I'd formulate this as something like > No need to explicitly fetch the asset downloads, ... > (to make clear that they are in fact fetched, just in a different way) Done. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:264: download_manager_ = nullptr; On 2016/10/17 10:18:41, Marc Treib wrote: > Should you also unregister all the item observers here? Done. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:318: : &DownloadSuggestionsProvider::UpdateOfflinePagesCache, On 2016/10/17 10:18:41, Marc Treib wrote: > Add the "bool notify" parameter also to UpdateOfflinePagesCache? Then you can > just bind that here, and OfflinePageModelChanged will just call > UpdateOfflinePagesCache(true). > (I find calling OfflinePageModelChanged here a bit weird) Done. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:427: // #if block there). On 2016/10/17 10:18:40, Marc Treib wrote: > ...we don't have some helper for this?! Ugh... > > I'd move the conversion into a helper method (in the anonymous namespace above), > so that a) it's localized, and b) we'd get a compiler error ("function doesn't > return anything") if any platform isn't handled, since it's not very obvious > that POSIX+WIN covers everything. Done. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:628: // TODO(vitaliii): Store one set instead of two. See crbug.com/656024. On 2016/10/17 10:18:41, Marc Treib wrote: > For this kind of thing (internal cleanup that has no user-visible impact), it's > not required to file a bug. (It's fine to file one if you want to, but you don't > *have* to.) Acknowledged. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.h:156: // the moment), prunes dismissed IDs and updates internal cache. On 2016/10/17 10:18:41, Marc Treib wrote: > nit: I don't understand the part in parens - what do you mean by "available"? > How would they not be available? Done. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:53: void PrintTo(const CategoryStatus& value, std::ostream* os) { On 2016/10/17 10:18:41, Marc Treib wrote: > If multiple tests need this, it's also okay to define an operator<< in > category_status.h (with a comment that it's only for testing). I think so far, > tests have just cast this to int for output, but clearly this makes for much > nicer messages. > (Just FYI, no action required :) I will do this as a part of clean up later. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:201: void IgnoreOnCategoryStatusChanged() { On 2016/10/17 10:18:41, Marc Treib wrote: > nit: This sounds like it ignores all changes, but it ignores only > AVAILABLE[_LOADING]. Not sure what a good name would be though... > IgnoreOnCategoryStatusChangedToAvailable? Done. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:216: scoped_refptr<OfflinePageProxy> proxy( On 2016/10/17 10:18:41, Marc Treib wrote: > DCHECK(!provider_) ? Done. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:225: void DestroyProvider() { provider_.release(); } On 2016/10/17 10:18:41, Marc Treib wrote: > This will leak memory - it gives up ownership and returns the raw pointer > without deleting it. You probably want > provider_ = nullptr; > or > provider_.reset(); I misunderstood what |release()| does, I meant |reset()|. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:233: DCHECK(provider_.get()); On 2016/10/17 10:18:41, Marc Treib wrote: > nit: I think the ".get()" isn't required Done. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:263: base::Bind(&CaptureDismissedSuggestions, &dismissed_suggestions)); On 2016/10/17 10:18:41, Marc Treib wrote: > Does this always return synchronously? If so, that's worth a comment. (Would > also be nice to check it, but that might be a bit complicated.) > If it's *not* always synchronous, then you'll have to wait until it finishes, > using base::RunLoop. (You can ask me for details if necessary) FakeOfflinePageModel (which we own) returns items synchronously. However, ordinary OfflinePageModel returns asynchronously, but I assume that this difference is irrelevant in our tests. Both ObservedMockDownloadManager and DownloadManager return items synchronously. I added a comment. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:275: StrictMock<MockContentSuggestionsProviderObserver>* observer() { On 2016/10/17 10:18:41, Marc Treib wrote: > I think just returning MockContentSuggestionsProviderObserver* (no StrictMock) > should work too? Done. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:406: EXPECT_CALL(*observer(), On 2016/10/17 10:18:41, Marc Treib wrote: > optional: If you expect calls in a particular order, you can use > testing::InSequence. It breaks my |IgnoreOn*| functions. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:429: IgnoreOnSuggestionInvalidated(); On 2016/10/17 10:18:41, Marc Treib wrote: > Does this do anything here? If so, worth a comment. No, but the reason is quite nontrivial. Basically, |downloads_manager_| is deleted before the |provider_| and so the provider does not get to see download items being deleted. I removed other instances and added a comment above (but only in one place). https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:636: // |OnDownloadItemDestroyed| is called from its destructor. On 2016/10/17 10:18:41, Marc Treib wrote: > nit: s/its/|removed_item|'s/ Done. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:639: IgnoreOnSuggestionInvalidated(); On 2016/10/17 10:18:41, Marc Treib wrote: > Is this necessary? The answer is the same as for the case above. No, because |downloads_manager_| is deleted before the provider. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:689: FireOfflinePageModelChanged(offline_pages_model()->items()); On 2016/10/17 10:18:41, Marc Treib wrote: > Is the FireOfflinePageModelChanged needed here? Yes, it is. > Shouldn't destroying removed_item above already result in this call? No, it should not. |OnSuggestionInvalidated| is used to notify the observer about removed item, while |FireOfflinePageModelChanged| forces |OnNewSuggestions| call. Or am I missing something? As far as I understood, the provider should not call |OnNewSuggestions| when the item is dismissed, so I assume the same for invalidated item. In other words, after dismissal or invalidation some items may not be shown until data source update. https://codereview.chromium.org/2360263002/diff/180001/components/ntp_snippet... File components/ntp_snippets/BUILD.gn (right): https://codereview.chromium.org/2360263002/diff/180001/components/ntp_snippet... components/ntp_snippets/BUILD.gn:142: "//components/offline_pages:offline_pages", On 2016/10/17 10:18:41, Marc Treib wrote: > nit: ":offline_pages" isn't required, just "//components/offline_pages" is > enough. Done. https://codereview.chromium.org/2360263002/diff/180001/components/ntp_snippet... components/ntp_snippets/BUILD.gn:170: "//base:base", On 2016/10/17 10:18:41, Marc Treib wrote: > nit: Also here, ":base" isn't required Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Thanks! Looks good :) https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:406: EXPECT_CALL(*observer(), On 2016/10/26 00:07:55, vitaliii wrote: > On 2016/10/17 10:18:41, Marc Treib wrote: > > optional: If you expect calls in a particular order, you can use > > testing::InSequence. > It breaks my |IgnoreOn*| functions. Interesting - why? https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:689: FireOfflinePageModelChanged(offline_pages_model()->items()); On 2016/10/26 00:07:55, vitaliii wrote: > On 2016/10/17 10:18:41, Marc Treib wrote: > > Is the FireOfflinePageModelChanged needed here? > Yes, it is. > > > Shouldn't destroying removed_item above already result in this call? > No, it should not. |OnSuggestionInvalidated| is used to notify the observer > about removed item, while |FireOfflinePageModelChanged| forces > |OnNewSuggestions| call. > > Or am I missing something? As far as I understood, the provider should not call > |OnNewSuggestions| when the item is dismissed, so I assume the same for > invalidated item. In other words, after dismissal or invalidation some items may > not be shown until data source update. Ah yes, you're right. Destroying the download should result in an OnSuggestionInvalidated (which we're ignoring above), but no OnNewSuggestions. So everything working as intended here :) https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:75: client_id.name_space == offline_pages::kNTPSuggestionsNamespace); nitty nit: parens not required https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:639: if (download_manager_) { optional: "if (!download_manager_) return;" and save a level of indentation. (Or maybe the null check isn't actually required? Seems like we never call this when the manager is null) https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.h:156: // moment offline pages are in the list), prunes dismissed IDs and updates grammar nit: "all offline pages available at the moment", or "all offline pages which are available at the moment". Or even just "assuming that these are all the offline pages that exist"?
treib@chromium.org changed reviewers: + bauerb@chromium.org
+bauerb for browser_prefs.cc. PTAL!
https://codereview.chromium.org/2360263002/diff/200001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2360263002/diff/200001/chrome/app/generated_r... chrome/app/generated_resources.grd:450: + <message name="IDS_NTP_DOWNLOAD_SUGGESTIONS_SECTION_HEADER" desc="Header of the downloads section, which is a list of pages (or files) that the user downloaded, displayed as cards on the New Tab Page."> Can you move this message closer to the other NTP messages? https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:111: const scoped_refptr<OfflinePageProxy>& offline_page_proxy, Per https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md..., scoped_refptr's should be passed without const-ref (and then std::move()d if necessary). https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:93: DCHECK(base::IsStringUTF8(string_type)); Note that this assumption is definitely not true in general -- Linux doesn't make any guarantees about the encoding of file paths. For your particular use case, you probably want LossyDisplayName(). https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:105: const scoped_refptr<ntp_snippets::OfflinePageProxy>& offline_page_proxy, No const-ref here either. https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:392: // |ContentSuggestion| is not copyable either. It just *might* be worth pushing for getting the exception mentioned in https://groups.google.com/a/chromium.org/d/msg/chromium-dev/8i4tMqNpHhg/T-Flw... into the style guide, and then marking the move constructor noexcept. https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:564: if (old_dismissed_ids.size() != retained_dismissed_ids.size()) { Be consistent whether you use braces for single-line bodies or not. https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/fake_download_item.h (right): https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/fake_download_item.h:131: base::FilePath dummy_file_path; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2360263002/diff/200001/components/ntp_snippet... File components/ntp_snippets/offline_pages/offline_pages_test_utils.h (right): https://codereview.chromium.org/2360263002/diff/200001/components/ntp_snippet... components/ntp_snippets/offline_pages/offline_pages_test_utils.h:27: std::vector<offline_pages::OfflinePageItem> items_; DISALLOW_COPY_AND_ASSIGN?
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
dgn@chromium.org changed reviewers: + dgn@chromium.org
https://codereview.chromium.org/2360263002/diff/220001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/220001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:135: /*has_more_button=*/download_manager_ui_enabled_, We now always show the button, and this flag is used only to say whether the button will also be shown when there are suggestions. I suppose here we want to completely remove the button when there is no download manager. This is not supported at the moment. It would currently hit a java assert if asserts are enabled, or be a noop if not. I guess that's all right for now. Filed https://crbug.com/660030 to track that. https://codereview.chromium.org/2360263002/diff/220001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:157: // TODO(vitaliii): Provide site's favicon for assets downloads. See For download thumbnail we could also do the same thing as the download manager? They have a predefined icon for each type and will soon have previews. https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
Hey Bernhard, could you have a look? I will investigate the bot later. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:406: EXPECT_CALL(*observer(), On 2016/10/26 08:54:08, Marc Treib wrote: > On 2016/10/26 00:07:55, vitaliii wrote: > > On 2016/10/17 10:18:41, Marc Treib wrote: > > > optional: If you expect calls in a particular order, you can use > > > testing::InSequence. > > It breaks my |IgnoreOn*| functions. > > Interesting - why? I do not know precise reason, but I speculate that when you have EXPECT_CALL(foo, CallA(...)).Times(AnyNumber); EXPECT_CALL(foo, CallB(...)); and apply |InSequence|, the first line is "forgotten" after the second line. There is a way to "order" only some calls by using InSequence s1, s2; EXPECT_CALL(foo, CallA()).InSequence(s1); EXPECT_CALL(foo, CallA()).InSequence(s2); but this seems like overkill to me. https://codereview.chromium.org/2360263002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:689: FireOfflinePageModelChanged(offline_pages_model()->items()); On 2016/10/26 08:54:08, Marc Treib wrote: > On 2016/10/26 00:07:55, vitaliii wrote: > > On 2016/10/17 10:18:41, Marc Treib wrote: > > > Is the FireOfflinePageModelChanged needed here? > > Yes, it is. > > > > > Shouldn't destroying removed_item above already result in this call? > > No, it should not. |OnSuggestionInvalidated| is used to notify the observer > > about removed item, while |FireOfflinePageModelChanged| forces > > |OnNewSuggestions| call. > > > > Or am I missing something? As far as I understood, the provider should not > call > > |OnNewSuggestions| when the item is dismissed, so I assume the same for > > invalidated item. In other words, after dismissal or invalidation some items > may > > not be shown until data source update. > > Ah yes, you're right. Destroying the download should result in an > OnSuggestionInvalidated (which we're ignoring above), but no OnNewSuggestions. > So everything working as intended here :) Acknowledged. https://codereview.chromium.org/2360263002/diff/200001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2360263002/diff/200001/chrome/app/generated_r... chrome/app/generated_resources.grd:450: + <message name="IDS_NTP_DOWNLOAD_SUGGESTIONS_SECTION_HEADER" desc="Header of the downloads section, which is a list of pages (or files) that the user downloaded, displayed as cards on the New Tab Page."> On 2016/10/26 10:19:32, Bernhard Bauer wrote: > Can you move this message closer to the other NTP messages? As it was discussed, I added a new "section" (or is the syntax different?). https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:111: const scoped_refptr<OfflinePageProxy>& offline_page_proxy, On 2016/10/26 10:19:32, Bernhard Bauer wrote: > Per > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md..., > scoped_refptr's should be passed without const-ref (and then std::move()d if > necessary). Done. https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:75: client_id.name_space == offline_pages::kNTPSuggestionsNamespace); On 2016/10/26 08:54:09, Marc Treib wrote: > nitty nit: parens not required Done. https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:93: DCHECK(base::IsStringUTF8(string_type)); On 2016/10/26 10:19:32, Bernhard Bauer wrote: > Note that this assumption is definitely not true in general -- Linux doesn't > make any guarantees about the encoding of file paths. > > For your particular use case, you probably want LossyDisplayName(). Thanks, I missed LossyDisplayName (though the name is not intuitive). https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:105: const scoped_refptr<ntp_snippets::OfflinePageProxy>& offline_page_proxy, On 2016/10/26 10:19:32, Bernhard Bauer wrote: > No const-ref here either. Done. https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:392: // |ContentSuggestion| is not copyable either. On 2016/10/26 10:19:32, Bernhard Bauer wrote: > It just *might* be worth pushing for getting the exception mentioned in > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/8i4tMqNpHhg/T-Flw... > into the style guide, and then marking the move constructor noexcept. I posted there, though I do not expect to get any progress soon, so I think we should go ahead as it is and modify it later. https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:564: if (old_dismissed_ids.size() != retained_dismissed_ids.size()) { On 2016/10/26 10:19:32, Bernhard Bauer wrote: > Be consistent whether you use braces for single-line bodies or not. Done. https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:639: if (download_manager_) { On 2016/10/26 08:54:09, Marc Treib wrote: > optional: "if (!download_manager_) return;" and save a level of indentation. > (Or maybe the null check isn't actually required? Seems like we never call this > when the manager is null) Now we don't, but what if we will? https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider.h (right): https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.h:156: // moment offline pages are in the list), prunes dismissed IDs and updates On 2016/10/26 08:54:09, Marc Treib wrote: > grammar nit: "all offline pages available at the moment", or "all offline pages > which are available at the moment". Or even just "assuming that these are all > the offline pages that exist"? Done. https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/fake_download_item.h (right): https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/fake_download_item.h:131: base::FilePath dummy_file_path; On 2016/10/26 10:19:33, Bernhard Bauer wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2360263002/diff/200001/components/ntp_snippet... File components/ntp_snippets/offline_pages/offline_pages_test_utils.h (right): https://codereview.chromium.org/2360263002/diff/200001/components/ntp_snippet... components/ntp_snippets/offline_pages/offline_pages_test_utils.h:27: std::vector<offline_pages::OfflinePageItem> items_; On 2016/10/26 10:19:33, Bernhard Bauer wrote: > DISALLOW_COPY_AND_ASSIGN? Done.
LGTM with two small things that would be nice: https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:392: // |ContentSuggestion| is not copyable either. On 2016/10/27 15:49:20, vitaliii wrote: > On 2016/10/26 10:19:32, Bernhard Bauer wrote: > > It just *might* be worth pushing for getting the exception mentioned in > > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/8i4tMqNpHhg/T-Flw... > > into the style guide, and then marking the move constructor noexcept. > > I posted there, though I do not expect to get any progress soon, so I think we > should go ahead as it is and modify it later. Yeah, that's fine; maybe add a TODO to use resize() once it works. https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:639: if (download_manager_) { On 2016/10/27 15:49:20, vitaliii wrote: > On 2016/10/26 08:54:09, Marc Treib wrote: > > optional: "if (!download_manager_) return;" and save a level of indentation. > > (Or maybe the null check isn't actually required? Seems like we never call > this > > when the manager is null) > > Now we don't, but what if we will? Assuming that you remove the check, you'll get a crash, which will show up on the crash dashboard, so you'll learn that your assumption was wrong :) In contrast, if you have a check in there that never fails, you won't actually learn about that. So, you could in fact make your precondition that |download_manager_| is not null explicit and add a DCHECK.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #12 (id:300001) has been deleted
Patchset #12 (id:320001) has been deleted
Patchset #12 (id:340001) 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 #19 (id:490001) has been deleted
Patchset #19 (id:510001) has been deleted
Patchset #20 (id:550001) 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 Marc, could you please have another look? Keep in mind that I did some debug output, which I hope should be gone by now :) I have discovered a workaround for the ios-simulator buildbot, namely not to remove the old string in components/ntp_snippets_strings.grdp, so I left a dummy string there, could you check whether it is ok? By the way, iOS team has similar issues (crbug.com/660343, there you can read my observations). If it looks good, feel free to submit. Also I tried to remove the string just to check for the last time and the ios-simulator failed. https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bu... (link may be invalid because I removed the corresponding patch) Thanks! https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:392: // |ContentSuggestion| is not copyable either. On 2016/10/27 15:57:09, Bernhard (OOO until Nov 11) wrote: > On 2016/10/27 15:49:20, vitaliii wrote: > > On 2016/10/26 10:19:32, Bernhard Bauer wrote: > > > It just *might* be worth pushing for getting the exception mentioned in > > > > > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/8i4tMqNpHhg/T-Flw... > > > into the style guide, and then marking the move constructor noexcept. > > > > I posted there, though I do not expect to get any progress soon, so I think we > > should go ahead as it is and modify it later. > > Yeah, that's fine; maybe add a TODO to use resize() once it works. The style guide change has already been done. However, this is an internal detail, so I will focus on user facing tasks for now. Thus, I added a TODO. https://codereview.chromium.org/2360263002/diff/200001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:639: if (download_manager_) { On 2016/10/27 15:57:09, Bernhard (OOO until Nov 11) wrote: > On 2016/10/27 15:49:20, vitaliii wrote: > > On 2016/10/26 08:54:09, Marc Treib wrote: > > > optional: "if (!download_manager_) return;" and save a level of indentation. > > > (Or maybe the null check isn't actually required? Seems like we never call > > this > > > when the manager is null) > > > > Now we don't, but what if we will? > > Assuming that you remove the check, you'll get a crash, which will show up on > the crash dashboard, so you'll learn that your assumption was wrong :) In > contrast, if you have a check in there that never fails, you won't actually > learn about that. So, you could in fact make your precondition that > |download_manager_| is not null explicit and add a DCHECK. Good point, added the DCHECK. https://codereview.chromium.org/2360263002/diff/220001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2360263002/diff/220001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:135: /*has_more_button=*/download_manager_ui_enabled_, On 2016/10/27 15:35:33, dgn wrote: > We now always show the button, and this flag is used only to say whether the > button will also be shown when there are suggestions. > > I suppose here we want to completely remove the button when there is no download > manager. This is not supported at the moment. It would currently hit a java > assert if asserts are enabled, or be a noop if not. I guess that's all right for > now. > > Filed https://crbug.com/660030 to track that. I did not know that meaning of |has_more_button| has been changed. We definitely want not to show MORE button when there is no downloads UI. I added a TODO. https://codereview.chromium.org/2360263002/diff/220001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/download_suggestions_provider.cc:157: // TODO(vitaliii): Provide site's favicon for assets downloads. See On 2016/10/27 15:35:33, dgn wrote: > For download thumbnail we could also do the same thing as the download manager? > They have a predefined icon for each type and will soon have previews. > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... We could, but so far it is still an open question: type icons vs. site favicons (crbug.com/631447).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2360263002/diff/530001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2360263002/diff/530001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:285: if (base::FeatureList::IsEnabled(ntp_snippets::kDownloadSuggestionsFeature)) { Shouldn't this flag move to //chrome too, since it's not used for anything in //components? https://codereview.chromium.org/2360263002/diff/530001/components/ntp_snippet... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2360263002/diff/530001/components/ntp_snippet... components/ntp_snippets/pref_names.h:46: extern const char kDismissedAssetDownloadSuggestions[]; same here, this is only used in //chrome, right?
Patchset #20 (id:570001) 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...
I am going to submit the CL with the dummy string, because 1) the ios-simulator issue is still there; 2) I've talked to iOS sheriff and restarting the bot is likely to take a day or two. I will remove the dummy string once the issue is resolved. https://codereview.chromium.org/2360263002/diff/530001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2360263002/diff/530001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:285: if (base::FeatureList::IsEnabled(ntp_snippets::kDownloadSuggestionsFeature)) { On 2016/11/01 11:05:18, dgn wrote: > Shouldn't this flag move to //chrome too, since it's not used for anything in > //components? I am not sure about this. We had to move the provider and its strings to /chrome due to dependency on the download manager, however, we do not have to move remaining things and keeping them together with other NTP things looks neat to me. Thus, as we discussed privately, I will proceed as it is and then we may do the move in the following CL. https://codereview.chromium.org/2360263002/diff/530001/components/ntp_snippet... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2360263002/diff/530001/components/ntp_snippet... components/ntp_snippets/pref_names.h:46: extern const char kDismissedAssetDownloadSuggestions[]; On 2016/11/01 11:05:18, dgn wrote: > same here, this is only used in //chrome, right? Same as in previous the comments (i.e. for now we proceed as it is and may move it in the following CL).
Also I am not waiting for another LGTM, because the reviewers are not available until FF. Instead I just looked through the code by myself again.
The CQ bit was unchecked by vitaliii@chromium.org
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2360263002/#ps590001 (title: "fixed some 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 vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2360263002/#ps620001 (title: "the freshest rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #22 (id:620001)
Message was sent while issue was closed.
Description was changed from ========== [NTPSnippets] Show all downloads on the NTP (3/3): Downloads provider. This CL is the third of the three CLs. The overall aim is to show all types of downloads on the NTP in downloads section. This CL adds DownloadSuggestionsProvider, which provides both offline page (e.g. offlined pages) and asset (e.g. images) downloads. If any of the two previous CLs is reverted, this CL must be reverted too. BUG=639233 ========== to ========== [NTPSnippets] Show all downloads on the NTP (3/3): Downloads provider. This CL is the third of the three CLs. The overall aim is to show all types of downloads on the NTP in downloads section. This CL adds DownloadSuggestionsProvider, which provides both offline page (e.g. offlined pages) and asset (e.g. images) downloads. If any of the two previous CLs is reverted, this CL must be reverted too. BUG=639233 Committed: https://crrev.com/dbedcae7e3c81448f3fc57f1493263087d2eb1b1 Cr-Commit-Position: refs/heads/master@{#429194} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/dbedcae7e3c81448f3fc57f1493263087d2eb1b1 Cr-Commit-Position: refs/heads/master@{#429194}
Message was sent while issue was closed.
https://codereview.chromium.org/2360263002/diff/530001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2360263002/diff/530001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:285: if (base::FeatureList::IsEnabled(ntp_snippets::kDownloadSuggestionsFeature)) { On 2016/11/02 00:47:37, vitaliii wrote: > On 2016/11/01 11:05:18, dgn wrote: > > Shouldn't this flag move to //chrome too, since it's not used for anything in > > //components? > > I am not sure about this. We had to move the provider and its strings to /chrome > due to dependency on the download manager, however, we do not have to move > remaining things and keeping them together with other NTP things looks neat to > me. > Thus, as we discussed privately, I will proceed as it is and then we may do the > move in the following CL. Hm. I'd say the flag should be next to the code that uses it (but it's not a huge deal). https://codereview.chromium.org/2360263002/diff/620001/components/ntp_snippet... File components/ntp_snippets_strings.grdp (right): https://codereview.chromium.org/2360263002/diff/620001/components/ntp_snippet... components/ntp_snippets_strings.grdp:28: <!-- TODO(vitaliii): Remove this item when crbug.com/660343 is resolved. --> That bug has been marked "fixed", can you try if the problem still exists? |
