Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2255783002:
Support server categories in NTPSnippetsService. (Closed)
DescriptionSupport server categories in NTPSnippetsService.
BUG=633613
Committed: https://crrev.com/330958ddcdcd0084d724571eddaa66f9c364115f
Cr-Commit-Position: refs/heads/master@{#414443}
Patch Set 1 #Patch Set 2 : sync #
Total comments: 39
Patch Set 3 : Address review comments. #Patch Set 4 : Empty category handling. #
Total comments: 28
Patch Set 5 : Use snippet ID only for database images. #Patch Set 6 : Address review comments. #Patch Set 7 : Rebase. #
Total comments: 3
Patch Set 8 : Move TODO. #
Total comments: 2
Patch Set 9 : Fix suggestion_id. #Patch Set 10 : rebase #Patch Set 11 : Add out-of-line *structors. #Patch Set 12 : *structors, rebase #Patch Set 13 : Fix after rebase #
Total comments: 1
Messages
Total messages: 40 (20 generated)
sfiera@chromium.org changed reviewers: + treib@chromium.org
This is as yet untested. The server-side support for the debug category is still broken, so I can't test manually, and I haven't yet written any tests because that first requires a bunch of changes to use the new API. However, it's getting close to branch point and I don't want to get this review started too late. I'm not sure when I'll have tests written, but I should be able to do manual testing by tomorrow.
Not happy about the snippet_id vs. suggestion_id mixing, but I think we can avoid that, see below. Otherwise just nits. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_database.h (right): https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_database.h:66: void LoadImage(const std::string& suggestion_id, So now, suggestions themselves are indexed by within-category-ID, but images by global-ID? That seems extremely confusing and error-prone, especially since both IDs have the same type. (We really need to do something about that!) How about just staying with within-category-ID? If the server returns two suggestions from different categories but with the same ID, then they'd better have to same image :) https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:219: categories_.emplace(articles_category_, CategoryContent()); map::emplace isn't available everywhere yet, see https://chromium-cpp.appspot.com/ https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:221: CategoryStatus::INITIALIZING); Use the actual value from categories_, so the default state isn't hardcoded in two places https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:261: for (const auto& category : categories_) { Use the actual type? auto isn't clear here, since this isn't actually a Category. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:292: // TODO(sfiera): pass back localized category titles. nit: mention that this refers to non-articles categories, otherwise it's a bit confusing: we *do* return a localized string! https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:304: Category category_id = GetCategoryFromUniqueID(suggestion_id); Just "category" please (that's what it's called everywhere else) https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:309: CategoryContent* category = &categories_[category_id]; call this something else, like "content" https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:483: // First, move them over into |to_delete|. This line isn't part of the TODO; move it to the end? https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:486: // and apply the same logic to them here. Maybe never? Eh, since host restricts are likely going away anyway, probably not worth bothering with. (It's fine to leave the TODO there though) https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:522: if (category_id == articles_category_) Braces please https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:552: continue; // skip anything we processed in the first loop. Why? I think the main purpose of calling ClearExpiredSnippets here was to schedule the next expiry run, so we want this particularly for the newly-updated stuff. For the categories that weren't updated, it should already be scheduled. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:562: // category. If the category is now |empty| but it was in |snippets|, then we nit: no pipes around empty https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:802: CategoryStatus::AVAILABLE_LOADING) Braces please https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:139: void ClearCachedSuggestionsForDebugging(Category category_id) override; ? https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:267: void UpdateAllCategoryStatus(CategoryStatus status); Add comment? https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (left): https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:253: run_loop_.Run(); Wait, did this even work? Looks like if |service| was ready already, we'd have an unfulfilled expectation? https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:68: return arg.id() == static_cast<int>(id); return arg.IsKnownCategory(id); ?
sorry for the lame question, but why are we dealing with 2 type of ids? If we really need them internally, can we not expose them outside the snippetsservice? https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_database.h (right): https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_database.h:66: void LoadImage(const std::string& suggestion_id, On 2016/08/22 15:06:46, Marc Treib wrote: > So now, suggestions themselves are indexed by within-category-ID, but images by > global-ID? That seems extremely confusing and error-prone, especially since both > IDs have the same type. (We really need to do something about that!) > How about just staying with within-category-ID? If the server returns two > suggestions from different categories but with the same ID, then they'd better > have to same image :) Yes, we should make sure snippet ids are unique. Actually, I think that assumption is made in other places of the code, too. Can you think of a concrete case where this might become an issue? Shouldn't de-duplicate elements based on snippet ids?
The "snippet_id vs suggestion_id" thing is a consequence of us baking the category into the IDs to make them unique across all categories. I've been thinking about that, and I'm now convinced that was a bad idea. I'll look into improving the situation after branch point. All that is more or less orthogonal to this CL though :)
https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_database.h (right): https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_database.h:66: void LoadImage(const std::string& suggestion_id, On 2016/08/22 15:06:46, Marc Treib wrote: > So now, suggestions themselves are indexed by within-category-ID, but images by > global-ID? That seems extremely confusing and error-prone, especially since both > IDs have the same type. (We really need to do something about that!) > How about just staying with within-category-ID? If the server returns two > suggestions from different categories but with the same ID, then they'd better > have to same image :) We plan to cache all server suggestions in the database, right? I see this as a stopgap before converting |snippet_id| to |suggestion_id| everywhere in the database. Converting the images was easy but I wasn't sure about other suggestion types yet. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:219: categories_.emplace(articles_category_, CategoryContent()); On 2016/08/22 15:06:46, Marc Treib wrote: > map::emplace isn't available everywhere yet, see > https://chromium-cpp.appspot.com/ Done. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:221: CategoryStatus::INITIALIZING); On 2016/08/22 15:06:47, Marc Treib wrote: > Use the actual value from categories_, so the default state isn't hardcoded in > two places Done. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:261: for (const auto& category : categories_) { On 2016/08/22 15:06:47, Marc Treib wrote: > Use the actual type? auto isn't clear here, since this isn't actually a > Category. I've switched this to what I do in most places, which is to break the pair out immediately into locals. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:292: // TODO(sfiera): pass back localized category titles. On 2016/08/22 15:06:47, Marc Treib wrote: > nit: mention that this refers to non-articles categories, otherwise it's a bit > confusing: we *do* return a localized string! Changed. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:304: Category category_id = GetCategoryFromUniqueID(suggestion_id); On 2016/08/22 15:06:47, Marc Treib wrote: > Just "category" please (that's what it's called everywhere else) Done. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:309: CategoryContent* category = &categories_[category_id]; On 2016/08/22 15:06:46, Marc Treib wrote: > call this something else, like "content" Done. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:483: // First, move them over into |to_delete|. On 2016/08/22 15:06:46, Marc Treib wrote: > This line isn't part of the TODO; move it to the end? Done. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:486: // and apply the same logic to them here. Maybe never? On 2016/08/22 15:06:47, Marc Treib wrote: > Eh, since host restricts are likely going away anyway, probably not worth > bothering with. (It's fine to leave the TODO there though) Acknowledged. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:522: if (category_id == articles_category_) On 2016/08/22 15:06:47, Marc Treib wrote: > Braces please Done. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:552: continue; // skip anything we processed in the first loop. On 2016/08/22 15:06:46, Marc Treib wrote: > Why? > I think the main purpose of calling ClearExpiredSnippets here was to schedule > the next expiry run, so we want this particularly for the newly-updated stuff. > For the categories that weren't updated, it should already be scheduled. I'd sort of misunderstood ClearExpiredSnippets(), and had broken it. In particular, my understanding of OneShotTimer now is that only one callback can be scheduled, so the callbacks shouldn't be category-specific. The logic for dropping categories has moved there too. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:562: // category. If the category is now |empty| but it was in |snippets|, then we On 2016/08/22 15:06:46, Marc Treib wrote: > nit: no pipes around empty Done. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:802: CategoryStatus::AVAILABLE_LOADING) On 2016/08/22 15:06:46, Marc Treib wrote: > Braces please Done. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:139: void ClearCachedSuggestionsForDebugging(Category category_id) override; On 2016/08/22 15:06:47, Marc Treib wrote: > ? Done. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:267: void UpdateAllCategoryStatus(CategoryStatus status); On 2016/08/22 15:06:47, Marc Treib wrote: > Add comment? Done. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (left): https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:253: run_loop_.Run(); On 2016/08/22 15:06:47, Marc Treib wrote: > Wait, did this even work? Looks like if |service| was ready already, we'd have > an unfulfilled expectation? I doubt it. Probably, this was never used when the service was already ready. https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:68: return arg.id() == static_cast<int>(id); On 2016/08/22 15:06:47, Marc Treib wrote: > return arg.IsKnownCategory(id); > ? This way is usable with server-categories (e.g. IsCategory(10002)), though that's not used yet.
The CQ bit was checked by sfiera@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...)
https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_database.h (right): https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_database.h:66: void LoadImage(const std::string& suggestion_id, On 2016/08/24 14:35:56, sfiera wrote: > On 2016/08/22 15:06:46, Marc Treib wrote: > > So now, suggestions themselves are indexed by within-category-ID, but images > by > > global-ID? That seems extremely confusing and error-prone, especially since > both > > IDs have the same type. (We really need to do something about that!) > > How about just staying with within-category-ID? If the server returns two > > suggestions from different categories but with the same ID, then they'd better > > have to same image :) > > We plan to cache all server suggestions in the database, right? I see this as a > stopgap before converting |snippet_id| to |suggestion_id| everywhere in the > database. Converting the images was easy but I wasn't sure about other > suggestion types yet. I guess that's the plan, but that will require more changes (e.g. also caching category info somewhere). Also, eventually we'd like to just store the proto we receive on the wire, which doesn't work if we mangle IDs. Anyway, I'm very unhappy about having two different kinds of IDs in the same class, both of which happen to be strings. This has caused problems before in other places, and I'm planning to get rid of the combined suggestion_id concept (and instead make a class composed of category+within_category_id). Anyway, proposal for now: Can we just keep using snippet_id, and ensure on the server side that we never return two suggestions with the same ID, even if they're in different categories? https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:552: // source and report the category as AVAILABLE. What's a "source" here? https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:561: // TODO(sfiera): per-category histograms Since histogram names have to be known at compile time, it'll probably be just one "experimental" one for non-articles categories (which will be fine, assuming we won't experiment with more than one category per user at a time). See content_suggestions_metrics.cc. https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:569: // If there are more snippets than we want to show, delete the extra ones. Move this comment right before the "if" below? https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:578: database_->DeleteSnippets(to_delete); While only articles are in the DB, we should probably only run this for articles. https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:588: for (auto& item : categories_) { const? https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:717: // Finally, actually delete the removed snippets from the DB. Add a TODO to extend this to non-articles? https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:824: callback.Run(suggestion_id, gfx::Image()); Call OnSnippetImageDecodedFromNetwork instead, to be consistent with the case below? https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:979: // categories we're interested in? No, I don't think so. When we get here, we should usually already have stuff that was fetched in the background. We don't want to fetch on every Chrome start. https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:1005: void NTPSnippetsService::NotifyNewSuggestions() { Do we always want to notify for all categories? https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:1006: for (auto& item : categories_) { const? https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:1033: // observer in that case. I don't think it's possible to detect that here?
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_database.h (right): https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_database.h:66: void LoadImage(const std::string& suggestion_id, On 2016/08/24 15:22:08, Marc Treib wrote: > On 2016/08/24 14:35:56, sfiera wrote: > > On 2016/08/22 15:06:46, Marc Treib wrote: > > > So now, suggestions themselves are indexed by within-category-ID, but images > > by > > > global-ID? That seems extremely confusing and error-prone, especially since > > both > > > IDs have the same type. (We really need to do something about that!) > > > How about just staying with within-category-ID? If the server returns two > > > suggestions from different categories but with the same ID, then they'd > better > > > have to same image :) > > > > We plan to cache all server suggestions in the database, right? I see this as > a > > stopgap before converting |snippet_id| to |suggestion_id| everywhere in the > > database. Converting the images was easy but I wasn't sure about other > > suggestion types yet. > > I guess that's the plan, but that will require more changes (e.g. also caching > category info somewhere). Also, eventually we'd like to just store the proto we > receive on the wire, which doesn't work if we mangle IDs. > Anyway, I'm very unhappy about having two different kinds of IDs in the same > class, both of which happen to be strings. This has caused problems before in > other places, and I'm planning to get rid of the combined suggestion_id concept > (and instead make a class composed of category+within_category_id). > > Anyway, proposal for now: Can we just keep using snippet_id, and ensure on the > server side that we never return two suggestions with the same ID, even if > they're in different categories? I had a closer look, and I'm fine with the current implementation. The class name NTPSnippetsService just confused me -- it's a provider. How the provider deals with the data is it's own business -- as long as the service the other components talk to only knows the globally unique ID that's fine. However I don't like the name 'snippets_id'. The name should make sure that these are internal ids from server-side suggestions. Not sure if 'server' is clear in the chrome context. We could call it 'cloud_suggestion_id' -- 'internal_cloud_suggestion_id' might be a bit chatty, but clearly differentiates these and makes clear that it's an implementation details of this provider. WDYT?
https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_database.h (right): https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_database.h:66: void LoadImage(const std::string& suggestion_id, On 2016/08/24 15:33:08, tschumann wrote: > On 2016/08/24 15:22:08, Marc Treib wrote: > > On 2016/08/24 14:35:56, sfiera wrote: > > > On 2016/08/22 15:06:46, Marc Treib wrote: > > > > So now, suggestions themselves are indexed by within-category-ID, but > images > > > by > > > > global-ID? That seems extremely confusing and error-prone, especially > since > > > both > > > > IDs have the same type. (We really need to do something about that!) > > > > How about just staying with within-category-ID? If the server returns two > > > > suggestions from different categories but with the same ID, then they'd > > better > > > > have to same image :) > > > > > > We plan to cache all server suggestions in the database, right? I see this > as > > a > > > stopgap before converting |snippet_id| to |suggestion_id| everywhere in the > > > database. Converting the images was easy but I wasn't sure about other > > > suggestion types yet. > > > > I guess that's the plan, but that will require more changes (e.g. also caching > > category info somewhere). Also, eventually we'd like to just store the proto > we > > receive on the wire, which doesn't work if we mangle IDs. > > Anyway, I'm very unhappy about having two different kinds of IDs in the same > > class, both of which happen to be strings. This has caused problems before in > > other places, and I'm planning to get rid of the combined suggestion_id > concept > > (and instead make a class composed of category+within_category_id). > > > > Anyway, proposal for now: Can we just keep using snippet_id, and ensure on the > > server side that we never return two suggestions with the same ID, even if > > they're in different categories? > > I had a closer look, and I'm fine with the current implementation. The class > name NTPSnippetsService just confused me -- it's a provider. > How the provider deals with the data is it's own business -- as long as the > service the other components talk to only knows the globally unique ID that's > fine. > However I don't like the name 'snippets_id'. The name should make sure that > these are internal ids from server-side suggestions. Not sure if 'server' is > clear in the chrome context. We could call it 'cloud_suggestion_id' -- > 'internal_cloud_suggestion_id' might be a bit chatty, but clearly differentiates > these and makes clear that it's an implementation details of this provider. > WDYT? The main thing that bothers me here is that the DB class now has some methods which take the global unique IDs, and others that take the internal ones. Since both are strings, it's extremely easy to mix them up, and it's just confusing.
https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:552: // source and report the category as AVAILABLE. On 2016/08/24 15:22:08, Marc Treib wrote: > What's a "source" here? Done. https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:561: // TODO(sfiera): per-category histograms On 2016/08/24 15:22:08, Marc Treib wrote: > Since histogram names have to be known at compile time, it'll probably be just > one "experimental" one for non-articles categories (which will be fine, assuming > we won't experiment with more than one category per user at a time). See > content_suggestions_metrics.cc. We can also parameterize histograms, right? e.g. the ones in the NTP with the index of the impression. We could do something like record (id % 8). Anyway, that's not relevant yet. I changed the TODO to just "histograms for server categories" because that's the specific thing we care about. https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:569: // If there are more snippets than we want to show, delete the extra ones. On 2016/08/24 15:22:08, Marc Treib wrote: > Move this comment right before the "if" below? Done. https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:578: database_->DeleteSnippets(to_delete); On 2016/08/24 15:22:08, Marc Treib wrote: > While only articles are in the DB, we should probably only run this for > articles. Done. https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:588: for (auto& item : categories_) { On 2016/08/24 15:22:08, Marc Treib wrote: > const? Done. https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:717: // Finally, actually delete the removed snippets from the DB. On 2016/08/24 15:22:08, Marc Treib wrote: > Add a TODO to extend this to non-articles? There's one TODO for putting non-articles in the database. I don't think it's useful to have one each place the database is used—it's pretty easy to find the places that treat articles_category_ specially. https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:824: callback.Run(suggestion_id, gfx::Image()); On 2016/08/24 15:22:08, Marc Treib wrote: > Call OnSnippetImageDecodedFromNetwork instead, to be consistent with the case > below? Sure, done. (was that function introduced recently? I would've thought I'd be consistent, but this CL has gone through a few rebases) https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:979: // categories we're interested in? On 2016/08/24 15:22:08, Marc Treib wrote: > No, I don't think so. When we get here, we should usually already have stuff > that was fetched in the background. We don't want to fetch on every Chrome > start. Removed. https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:1005: void NTPSnippetsService::NotifyNewSuggestions() { On 2016/08/24 15:22:09, Marc Treib wrote: > Do we always want to notify for all categories? I think it's fine for a first cut (but there's the TODO below). https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:1006: for (auto& item : categories_) { On 2016/08/24 15:22:08, Marc Treib wrote: > const? Done. https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:1033: // observer in that case. On 2016/08/24 15:22:09, Marc Treib wrote: > I don't think it's possible to detect that here? It wouldn't be possible here. We could detect it in OnFetchFinished(), but this felt like the right function for the TODO. Would you prefer it above?
The CQ bit was checked by sfiera@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_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Renaming suggestion_id back to snippet_id in the appropriate places is still open, otherwise LG https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:561: // TODO(sfiera): per-category histograms On 2016/08/24 18:56:45, sfiera wrote: > On 2016/08/24 15:22:08, Marc Treib wrote: > > Since histogram names have to be known at compile time, it'll probably be just > > one "experimental" one for non-articles categories (which will be fine, > assuming > > we won't experiment with more than one category per user at a time). See > > content_suggestions_metrics.cc. > > We can also parameterize histograms, right? e.g. the ones in the NTP with the > index of the impression. We could do something like record (id % 8). Parameterize: Yes, but again, the parameters have to be known statically. The id%8 thing would work, but I'm not sure it's worth the trouble. > Anyway, that's not relevant yet. I changed the TODO to just "histograms for > server categories" because that's the specific thing we care about. Indeed :) https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:717: // Finally, actually delete the removed snippets from the DB. On 2016/08/24 18:56:45, sfiera wrote: > On 2016/08/24 15:22:08, Marc Treib wrote: > > Add a TODO to extend this to non-articles? > > There's one TODO for putting non-articles in the database. I don't think it's > useful to have one each place the database is used—it's pretty easy to find the > places that treat articles_category_ specially. Acknowledged. https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:824: callback.Run(suggestion_id, gfx::Image()); On 2016/08/24 18:56:45, sfiera wrote: > On 2016/08/24 15:22:08, Marc Treib wrote: > > Call OnSnippetImageDecodedFromNetwork instead, to be consistent with the case > > below? > > Sure, done. > > (was that function introduced recently? I would've thought I'd be consistent, > but this CL has gone through a few rebases) Semi-recently I think - a week or two ago maybe? https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:1005: void NTPSnippetsService::NotifyNewSuggestions() { On 2016/08/24 18:56:45, sfiera wrote: > On 2016/08/24 15:22:09, Marc Treib wrote: > > Do we always want to notify for all categories? > > I think it's fine for a first cut (but there's the TODO below). Acknowledged. https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:1033: // observer in that case. On 2016/08/24 18:56:45, sfiera wrote: > On 2016/08/24 15:22:09, Marc Treib wrote: > > I don't think it's possible to detect that here? > > It wouldn't be possible here. We could detect it in OnFetchFinished(), but this > felt like the right function for the TODO. Would you prefer it above? I think so, yes - to me, the TODO being here kinda implies that some code should go here. https://codereview.chromium.org/2255783002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2255783002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:242: const std::string& suggestion_id, snippet_id, also below and in the .cc
https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2255783002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:1033: // observer in that case. On 2016/08/25 09:10:50, Marc Treib wrote: > On 2016/08/24 18:56:45, sfiera wrote: > > On 2016/08/24 15:22:09, Marc Treib wrote: > > > I don't think it's possible to detect that here? > > > > It wouldn't be possible here. We could detect it in OnFetchFinished(), but > this > > felt like the right function for the TODO. Would you prefer it above? > > I think so, yes - to me, the TODO being here kinda implies that some code should > go here. Moved. https://codereview.chromium.org/2255783002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2255783002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:242: const std::string& suggestion_id, On 2016/08/25 09:10:50, Marc Treib wrote: > snippet_id, also below and in the .cc |suggestion_id| is correct, I think. Nowhere do we pass |suggestion_id| to the database; we extract a snippet ID for the database and bind the suggestion_id (which is what we need in this class) to callbacks.
lgtm https://codereview.chromium.org/2255783002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2255783002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:242: const std::string& suggestion_id, On 2016/08/25 12:45:13, sfiera wrote: > On 2016/08/25 09:10:50, Marc Treib wrote: > > snippet_id, also below and in the .cc > > |suggestion_id| is correct, I think. Nowhere do we pass |suggestion_id| to the > database; we extract a snippet ID for the database and bind the suggestion_id > (which is what we need in this class) to callbacks. Ah, you're right. In fact, ... https://codereview.chromium.org/2255783002/diff/140001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2255783002/diff/140001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:253: const std::string& snippet_id, This one should be suggestion_id too!
https://codereview.chromium.org/2255783002/diff/140001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2255783002/diff/140001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:253: const std::string& snippet_id, On 2016/08/25 12:56:24, Marc Treib wrote: > This one should be suggestion_id too! Right, thanks! …now that I think of it, it's surprising to me that we don't get lint or clang warnings about name mismatches. Seems like it must be a problem as old as function prototypes.
The CQ bit was checked by sfiera@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2255783002/#ps180001 (title: "rebase")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by sfiera@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_ozone_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 sfiera@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2255783002/#ps240001 (title: "Fix after 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 #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Support server categories in NTPSnippetsService. BUG=633613 ========== to ========== Support server categories in NTPSnippetsService. BUG=633613 Committed: https://crrev.com/330958ddcdcd0084d724571eddaa66f9c364115f Cr-Commit-Position: refs/heads/master@{#414443} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/330958ddcdcd0084d724571eddaa66f9c364115f Cr-Commit-Position: refs/heads/master@{#414443}
Message was sent while issue was closed.
https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_database.h (right): https://codereview.chromium.org/2255783002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_database.h:66: void LoadImage(const std::string& suggestion_id, On 2016/08/24 15:38:54, Marc Treib wrote: > On 2016/08/24 15:33:08, tschumann wrote: > > On 2016/08/24 15:22:08, Marc Treib wrote: > > > On 2016/08/24 14:35:56, sfiera wrote: > > > > On 2016/08/22 15:06:46, Marc Treib wrote: > > > > > So now, suggestions themselves are indexed by within-category-ID, but > > images > > > > by > > > > > global-ID? That seems extremely confusing and error-prone, especially > > since > > > > both > > > > > IDs have the same type. (We really need to do something about that!) > > > > > How about just staying with within-category-ID? If the server returns > two > > > > > suggestions from different categories but with the same ID, then they'd > > > better > > > > > have to same image :) > > > > > > > > We plan to cache all server suggestions in the database, right? I see this > > as > > > a > > > > stopgap before converting |snippet_id| to |suggestion_id| everywhere in > the > > > > database. Converting the images was easy but I wasn't sure about other > > > > suggestion types yet. > > > > > > I guess that's the plan, but that will require more changes (e.g. also > caching > > > category info somewhere). Also, eventually we'd like to just store the proto > > we > > > receive on the wire, which doesn't work if we mangle IDs. > > > Anyway, I'm very unhappy about having two different kinds of IDs in the same > > > class, both of which happen to be strings. This has caused problems before > in > > > other places, and I'm planning to get rid of the combined suggestion_id > > concept > > > (and instead make a class composed of category+within_category_id). > > > > > > Anyway, proposal for now: Can we just keep using snippet_id, and ensure on > the > > > server side that we never return two suggestions with the same ID, even if > > > they're in different categories? > > > > I had a closer look, and I'm fine with the current implementation. The class > > name NTPSnippetsService just confused me -- it's a provider. > > How the provider deals with the data is it's own business -- as long as the > > service the other components talk to only knows the globally unique ID that's > > fine. > > However I don't like the name 'snippets_id'. The name should make sure that > > these are internal ids from server-side suggestions. Not sure if 'server' is > > clear in the chrome context. We could call it 'cloud_suggestion_id' -- > > 'internal_cloud_suggestion_id' might be a bit chatty, but clearly > differentiates > > these and makes clear that it's an implementation details of this provider. > > WDYT? > > The main thing that bothers me here is that the DB class now has some methods > which take the global unique IDs, and others that take the internal ones. Since > both are strings, it's extremely easy to mix them up, and it's just confusing. Correct! Yes, the NTPSnippetsDataBase should only handle internal ids (i.e. ids of one type). I guess we don't have to care about existing data and can just change that. -->Separate CL? https://codereview.chromium.org/2255783002/diff/240001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2255783002/diff/240001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:754: expiry_timer_.Start(FROM_HERE, next_expiry - expiry, i wonder if we should add some leeway here for the case that many snippets are expiring shortly after each other. Right now, the expiration time is based on crawltime which should be well distributed; i'm more afraid of a case where we have snippets expiring 30ms after each other and we run through this code 20 times in a row. Then again, I'm not sure this is an actual concern even if that scenario happens. On the other hands adding 5s (or 10) to make sure we're not triggering this path more than every 5s wouldn't be too bad.
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2277323003/ by johnme@chromium.org. The reason for reverting is: Following tests failing: NTPSnippetsServiceTest.GetDismissed NTPSnippetsServiceTest.LogNumArticlesHistogram NTPSnippetsServiceTest.DismissShouldRespectAllKnownUrls NTPSnippetsServiceTest.ImageReturnedWithTheSameId NTPSnippetsServiceTest.Dismiss on bots: https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/45385 https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/27331 https://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%2.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
