Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2640773002:
[NTP::PhysicalWeb] Filter our pages with same group_id. (Closed)
Description[NTP::PhysicalWeb] Filter our pages with same group_id.
1) If there are pages with the same non-empty group_id, only the closest
one is left.
2) Absence of distance estimate (represented with a negative value) is
handled as infinite distance.
The reason: suggestions for pages with the same group_id appear the same
in the UI.
BUG=681981
Review-Url: https://codereview.chromium.org/2640773002
Cr-Commit-Position: refs/heads/master@{#444353}
Committed: https://chromium.googlesource.com/chromium/src/+/34b12e40418272d36ae5c94c58e50d97bd240697
Patch Set 1 : #
Total comments: 24
Patch Set 2 : treib@ comments. #
Total comments: 4
Patch Set 3 : treib@ nits. #
Messages
Total messages: 26 (17 generated)
Patchset #1 (id:1) 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...
vitaliii@chromium.org changed reviewers: + treib@chromium.org
Hi treib@, could you have a look?
vitaliii@chromium.org changed reviewers: + olivierrobin@chromium.org
Hi olivierrobin@, Could you have a look at my fake_physical_web_data_source.* change? We started using group_id, so I had to add the field in tests, so that our code does not crash.
treib@chromium.org changed reviewers: - olivierrobin@chromium.org
One high-level comment: What does a group_id represent? Is it always okay to only show a single item from each group? https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:40: std::string GetPageId(const DictionaryValue& page_dictionary) { Maybe make a GetGroupId function similar to this, so not all the call sites have to do the error checking? Also, unrelated to this CL, but weren't the physical web folks moving away from the dictionary representation? https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:49: bool CompareByDistanceClosestFirst(const DictionaryValue* left, nit: "ClosestFirst" IMO isn't necessary to state, I'd assume that by default (standard numerical ordering) https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:62: // manually. nit: Treat how, exactly? Or maybe actually just assign infinity to the corresponding double variable? https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:85: LOG(DFATAL) << "Group id field is missing."; So the field must always be there, even if it's empty? https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:97: // Empty group_id must be treated as unique, so we do not apply std::unique to nit: *Each* empty group id must be... https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:99: std::vector<const DictionaryValue*>::iterator nonempty_group_id_begin = optional: You could use auto here https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:101: for (; nonempty_group_id_begin != page_dictionaries->end(); Since you're already using std algorithms quite heavily: This could probably be written as an std::find_if. https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:111: std::vector<const DictionaryValue*>::iterator new_end = std::unique( optional: auto https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:190: pages->GetDictionary(1, &second_page); ASSERT_TRUE? Also, maybe add a comment? (Set the second page to unknown distance.) https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:232: page->SetString(physical_web::kGroupIdKey, ""); Isn't this the default anyway? https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:236: // The closest page should be reported. Inaccurate comment
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by vitaliii@chromium.org 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 treib@, Could you have a look? https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:40: std::string GetPageId(const DictionaryValue& page_dictionary) { On 2017/01/18 11:24:01, Marc Treib wrote: > Maybe make a GetGroupId function similar to this, so not all the call sites have > to do the error checking? > > Also, unrelated to this CL, but weren't the physical web folks moving away from > the dictionary representation? Done. They do, but they may do this after BP. https://codereview.chromium.org/2643453002/ https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:49: bool CompareByDistanceClosestFirst(const DictionaryValue* left, On 2017/01/18 11:24:01, Marc Treib wrote: > nit: "ClosestFirst" IMO isn't necessary to state, I'd assume that by default > (standard numerical ordering) Done. https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:62: // manually. On 2017/01/18 11:24:01, Marc Treib wrote: > nit: Treat how, exactly? > Or maybe actually just assign infinity to the corresponding double variable? Do you how to get infinity for double? I rewrote the comment, it may do now. https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:85: LOG(DFATAL) << "Group id field is missing."; On 2017/01/18 11:24:01, Marc Treib wrote: > So the field must always be there, even if it's empty? As experiments show, yes. This won't be a problem after https://codereview.chromium.org/2643453002/. https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:97: // Empty group_id must be treated as unique, so we do not apply std::unique to On 2017/01/18 11:24:01, Marc Treib wrote: > nit: *Each* empty group id must be... Done. https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:99: std::vector<const DictionaryValue*>::iterator nonempty_group_id_begin = On 2017/01/18 11:24:01, Marc Treib wrote: > optional: You could use auto here Done. https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:101: for (; nonempty_group_id_begin != page_dictionaries->end(); On 2017/01/18 11:24:01, Marc Treib wrote: > Since you're already using std algorithms quite heavily: This could probably be > written as an std::find_if. Done. Good observation. https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:111: std::vector<const DictionaryValue*>::iterator new_end = std::unique( On 2017/01/18 11:24:01, Marc Treib wrote: > optional: auto Done. https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:190: pages->GetDictionary(1, &second_page); On 2017/01/18 11:24:01, Marc Treib wrote: > ASSERT_TRUE? > > Also, maybe add a comment? (Set the second page to unknown distance.) Done. https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:232: page->SetString(physical_web::kGroupIdKey, ""); On 2017/01/18 11:24:01, Marc Treib wrote: > Isn't this the default anyway? Yes, it is. I choose default in fake_physical_web_source helpers, however, I wanted to make it explicit here. https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:236: // The closest page should be reported. On 2017/01/18 11:24:01, Marc Treib wrote: > Inaccurate comment Done.
lgtm https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:62: // manually. On 2017/01/18 11:48:16, vitaliii (OOO until 23rd) wrote: > On 2017/01/18 11:24:01, Marc Treib wrote: > > nit: Treat how, exactly? > > Or maybe actually just assign infinity to the corresponding double variable? > > Do you how to get infinity for double? std::numeric_limits<double>::infinity(); (or there's also some c-style constant for it, but I forget) > I rewrote the comment, it may do now. https://codereview.chromium.org/2640773002/diff/40001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2640773002/diff/40001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:69: // When there is no estimate, the value is <= 0, so we implicilty treat it as s/implicilty/implicitly/ https://codereview.chromium.org/2640773002/diff/40001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:87: right_group_id = GetGroupId(*right); nitty nit: I'd just make two declarations
olivierrobin@chromium.org changed reviewers: + olivierrobin@chromium.org
fake_physical_web_data_source.* LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hi treib@ I addressed your nits, no need to look. https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2640773002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:62: // manually. On 2017/01/18 11:54:47, Marc Treib wrote: > On 2017/01/18 11:48:16, vitaliii (OOO until 23rd) wrote: > > On 2017/01/18 11:24:01, Marc Treib wrote: > > > nit: Treat how, exactly? > > > Or maybe actually just assign infinity to the corresponding double variable? > > > > Do you how to get infinity for double? > > std::numeric_limits<double>::infinity(); > (or there's also some c-style constant for it, but I forget) > > > I rewrote the comment, it may do now. > Acknowledged. https://codereview.chromium.org/2640773002/diff/40001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2640773002/diff/40001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:69: // When there is no estimate, the value is <= 0, so we implicilty treat it as On 2017/01/18 11:54:48, Marc Treib wrote: > s/implicilty/implicitly/ Done. https://codereview.chromium.org/2640773002/diff/40001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:87: right_group_id = GetGroupId(*right); On 2017/01/18 11:54:48, Marc Treib wrote: > nitty nit: I'd just make two declarations Done.
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, olivierrobin@chromium.org Link to the patchset: https://codereview.chromium.org/2640773002/#ps60001 (title: "treib@ nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484748183959500,
"parent_rev": "8e1c144ac1b517fb8406a3a3d17e9b3ed3aeb911", "commit_rev":
"34b12e40418272d36ae5c94c58e50d97bd240697"}
Message was sent while issue was closed.
Description was changed from ========== [NTP::PhysicalWeb] Filter our pages with same group_id. 1) If there are pages with the same non-empty group_id, only the closest one is left. 2) Absence of distance estimate (represented with a negative value) is handled as infinite distance. The reason: suggestions for pages with the same group_id appear the same in the UI. BUG=681981 ========== to ========== [NTP::PhysicalWeb] Filter our pages with same group_id. 1) If there are pages with the same non-empty group_id, only the closest one is left. 2) Absence of distance estimate (represented with a negative value) is handled as infinite distance. The reason: suggestions for pages with the same group_id appear the same in the UI. BUG=681981 Review-Url: https://codereview.chromium.org/2640773002 Cr-Commit-Position: refs/heads/master@{#444353} Committed: https://chromium.googlesource.com/chromium/src/+/34b12e40418272d36ae5c94c58e5... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/34b12e40418272d36ae5c94c58e5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
