[NTP::SectionOrder] Preserve added categories on ClearHistory.

[NTP::SectionOrder] Preserve added categories on ClearHistory.

Before this CL all categories, which were added using
AppendCategoryIfNecessary, were removed on ClearHistory. Since it is not
required to reregister them, there were simply not shown at all. After
this CL they are preserved (sorted by id).

BUG=675953
Committed: https://crrev.com/6343b2cefff8bce62994da5fc6b6156c7d6409a6
Cr-Commit-Position: refs/heads/master@{#441336}

[vitaliii, 2016-12-30 07:25:27]

Patchset #1 (id:1) has been deleted

[vitaliii, 2016-12-30 07:25:37]

The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-30 07:25:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-30 08:18:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-30 08:18:39]

Dry run: This issue passed the CQ dry run.

[vitaliii, 2017-01-03 15:13:19]

Description was changed from

==========
[NTP::SectionOrder] Preserve added categories on ClearHistory.

Before this CL all categories, which were added using
AppendCategoryIfNecessary, were removed on ClearHistory. Since it is not
required to reregister them, there were simply not shown at all. After
this CL they are preserved (sorted by id).

BUG=675953
==========

to

==========
[NTP::SectionOrder] Preserve added categories on ClearHistory.

Before this CL all categories, which were added using
AppendCategoryIfNecessary, were removed on ClearHistory. Since it is not
required to reregister them, there were simply not shown at all. After
this CL they are preserved (sorted by id).

BUG=675953
==========

[vitaliii, 2017-01-03 15:22:40]

The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 15:22:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vitaliii, 2017-01-03 15:27:07]

vitaliii@chromium.org changed reviewers:
+ jkrcal@chromium.org

[vitaliii, 2017-01-03 15:27:08]

Hi jkrcal@,

PTAL.

[jkrcal, 2017-01-03 15:57:16]

lgtm (with a nit)

https://codereview.chromium.org/2605353004/diff/60001/components/ntp_snippets...
File components/ntp_snippets/category_rankers/click_based_category_ranker.cc
(right):

https://codereview.chromium.org/2605353004/diff/60001/components/ntp_snippets...
components/ntp_snippets/category_rankers/click_based_category_ranker.cc:100: }
nit: I find this too complicated (and hard to read). Wouldn't overloading "=="
on RankedCategory help here? The equality operator would compare only the
category members as you do it here.

[vitaliii, 2017-01-03 16:12:28]

Hi jkrcal@,

I partially addressed your nit, PTAL.

https://codereview.chromium.org/2605353004/diff/60001/components/ntp_snippets...
File components/ntp_snippets/category_rankers/click_based_category_ranker.cc
(right):

https://codereview.chromium.org/2605353004/diff/60001/components/ntp_snippets...
components/ntp_snippets/category_rankers/click_based_category_ranker.cc:100: }
On 2017/01/03 15:57:16, jkrcal wrote:
> nit: I find this too complicated (and hard to read). Wouldn't overloading "=="
> on RankedCategory help here? The equality operator would compare only the
> category members as you do it here.

Done (but different).

It would work here, but such nontrivial compare operator (which ignores a field)
may be very misleading (not only here, but for future uses as well).

Instead I moved |std::find_if| outside of the |if| condition.

[vitaliii, 2017-01-03 16:12:37]

The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 16:12:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jkrcal, 2017-01-03 16:32:36]

still lgtm

https://codereview.chromium.org/2605353004/diff/60001/components/ntp_snippets...
File components/ntp_snippets/category_rankers/click_based_category_ranker.cc
(right):

https://codereview.chromium.org/2605353004/diff/60001/components/ntp_snippets...
components/ntp_snippets/category_rankers/click_based_category_ranker.cc:100: }
On 2017/01/03 16:12:28, vitaliii wrote:
> On 2017/01/03 15:57:16, jkrcal wrote:
> > nit: I find this too complicated (and hard to read). Wouldn't overloading
"=="
> > on RankedCategory help here? The equality operator would compare only the
> > category members as you do it here.
> 
> Done (but different).
> 
> It would work here, but such nontrivial compare operator (which ignores a
field)
> may be very misleading (not only here, but for future uses as well).
> 
> Instead I moved |std::find_if| outside of the |if| condition.

That's definitely better than before, thanks!

[vitaliii, 2017-01-03 16:44:37]

No need to look.

https://codereview.chromium.org/2605353004/diff/60001/components/ntp_snippets...
File components/ntp_snippets/category_rankers/click_based_category_ranker.cc
(right):

https://codereview.chromium.org/2605353004/diff/60001/components/ntp_snippets...
components/ntp_snippets/category_rankers/click_based_category_ranker.cc:100: }
On 2017/01/03 16:32:36, jkrcal wrote:
> On 2017/01/03 16:12:28, vitaliii wrote:
> > On 2017/01/03 15:57:16, jkrcal wrote:
> > > nit: I find this too complicated (and hard to read). Wouldn't overloading
> "=="
> > > on RankedCategory help here? The equality operator would compare only the
> > > category members as you do it here.
> > 
> > Done (but different).
> > 
> > It would work here, but such nontrivial compare operator (which ignores a
> field)
> > may be very misleading (not only here, but for future uses as well).
> > 
> > Instead I moved |std::find_if| outside of the |if| condition.
> 
> That's definitely better than before, thanks!

Acknowledged.

[vitaliii, 2017-01-03 16:44:41]

The CQ bit was unchecked by vitaliii@chromium.org

[vitaliii, 2017-01-03 16:44:44]

The CQ bit was checked by vitaliii@chromium.org

[commit-bot: I haz the power, 2017-01-03 16:44:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-03 17:10:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-03 17:10:21]

Failed to apply patch for
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:
While running git apply --index -p1;
  error: patch failed:
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:226
  error:
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:
patch does not apply

Patch:      
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc
Index:
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc
diff --git
a/components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc
b/components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc
index
fc1cb55d57a69efadfb8076f1298a5621c3c7e7b..9d059515eca053b2c648269ffc0bd138b1a4b2a4
100644
---
a/components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc
+++
b/components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc
@@ -226,4 +226,43 @@ TEST_F(ClickBasedCategoryRankerTest,
ShouldPersistOrderAndClicksWhenRestarted) {
   EXPECT_TRUE(CompareCategories(third, first));
 }
 
+TEST_F(ClickBasedCategoryRankerTest, ShouldRestoreDefaultOrderOnClearHistory) {
+  std::vector<KnownCategories> default_order =
+      ConstantCategoryRanker::GetKnownCategoriesDefaultOrder();
+  Category first = Category::FromKnownCategory(default_order[0]);
+  Category second = Category::FromKnownCategory(default_order[1]);
+
+  ASSERT_TRUE(CompareCategories(first, second));
+
+  // Change the order.
+  while (CompareCategories(first, second)) {
+    NotifyOnSuggestionOpened(
+        /*times=*/ClickBasedCategoryRanker::GetPassingMargin(), second);
+  }
+
+  ASSERT_FALSE(CompareCategories(first, second));
+
+  // The user clears history.
+  ranker()->ClearHistory(/*begin=*/base::Time(),
+                         /*end=*/base::Time::Max());
+
+  // The default order must be restored.
+  EXPECT_TRUE(CompareCategories(first, second));
+}
+
+TEST_F(ClickBasedCategoryRankerTest,
+       ShouldPreserveRemoteCategoriesOnClearHistory) {
+  Category first = AddUnusedRemoteCategory();
+  Category second = AddUnusedRemoteCategory();
+
+  ASSERT_TRUE(CompareCategories(first, second));
+
+  // The user clears history.
+  ranker()->ClearHistory(/*begin=*/base::Time(),
+                         /*end=*/base::Time::Max());
+
+  // The order does not matter, but the ranker should not die.
+  CompareCategories(first, second);
+}
+
 }  // namespace ntp_snippets

[vitaliii, 2017-01-04 07:10:53]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2017-01-04 07:10:53]

The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org
Link to the patchset: https://codereview.chromium.org/2605353004/#ps90001
(title: "rebase.")

[commit-bot: I haz the power, 2017-01-04 07:11:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 07:57:15]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1483513853221100,
"parent_rev": "b0264f80959bbf4b024b138f400419205d8b31a9", "commit_rev":
"0f8e22bcfb2d999a5741d676daf6e945bad5d376"}

[commit-bot: I haz the power, 2017-01-04 07:57:45]

Description was changed from

==========
[NTP::SectionOrder] Preserve added categories on ClearHistory.

Before this CL all categories, which were added using
AppendCategoryIfNecessary, were removed on ClearHistory. Since it is not
required to reregister them, there were simply not shown at all. After
this CL they are preserved (sorted by id).

BUG=675953
==========

to

==========
[NTP::SectionOrder] Preserve added categories on ClearHistory.

Before this CL all categories, which were added using
AppendCategoryIfNecessary, were removed on ClearHistory. Since it is not
required to reregister them, there were simply not shown at all. After
this CL they are preserved (sorted by id).

BUG=675953

Review-Url: https://codereview.chromium.org/2605353004
==========

[commit-bot: I haz the power, 2017-01-04 07:57:46]

Committed patchset #5 (id:90001)

[commit-bot: I haz the power, 2017-01-04 07:59:11]

Description was changed from

==========
[NTP::SectionOrder] Preserve added categories on ClearHistory.

Before this CL all categories, which were added using
AppendCategoryIfNecessary, were removed on ClearHistory. Since it is not
required to reregister them, there were simply not shown at all. After
this CL they are preserved (sorted by id).

BUG=675953

Review-Url: https://codereview.chromium.org/2605353004
==========

to

==========
[NTP::SectionOrder] Preserve added categories on ClearHistory.

Before this CL all categories, which were added using
AppendCategoryIfNecessary, were removed on ClearHistory. Since it is not
required to reregister them, there were simply not shown at all. After
this CL they are preserved (sorted by id).

BUG=675953

Committed: https://crrev.com/6343b2cefff8bce62994da5fc6b6156c7d6409a6
Cr-Commit-Position: refs/heads/master@{#441336}
==========

[commit-bot: I haz the power, 2017-01-04 07:59:11]

Patchset 5 (id:??) landed as
https://crrev.com/6343b2cefff8bce62994da5fc6b6156c7d6409a6
Cr-Commit-Position: refs/heads/master@{#441336}