Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2896233003:
Cancel the JumpList update if top 5 most visited URLs are unchanged (Closed)
|
Created:
3 years, 7 months ago by chengx Modified:
3 years, 6 months ago Reviewers:
grt (UTC plus 2) CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCancel the JumpList update if top 5 most visited URLs are unchanged
Previously, if TopSites service detects any changes in the top 24 most
visited URLs, a JumpList update is scheduled. However, since only the
top 5 URLs are displayed in JumpList, updates caused by the changes
outside of the top 5 URLs are wasted. This CL trims out that waste.
BUG=40407, 179576, 715902, 717236, 498987
Review-Url: https://codereview.chromium.org/2896233003
Cr-Commit-Position: refs/heads/master@{#476017}
Committed: https://chromium.googlesource.com/chromium/src/+/61e622b8b398999d4884231999375245c07dad01
Patch Set 1 #Patch Set 2 : Fix signed/unsigned int comparison build error #
Total comments: 6
Patch Set 3 : Address comments #
Total comments: 2
Patch Set 4 : Add unittest #
Total comments: 26
Patch Set 5 : Address comments #
Total comments: 8
Patch Set 6 : Git pull #Patch Set 7 : Fix nits #
Messages
Total messages: 90 (79 generated)
The CQ bit was checked by chengx@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...
Description was changed from ========== cancel this update if top most visited urls are all in cache BUG= ========== to ========== Cancel jumplist update if top 5 urls are unchanged if not considering their order BUG= ==========
Description was changed from ========== Cancel jumplist update if top 5 urls are unchanged if not considering their order BUG= ========== to ========== Cancel jumplist update if top 5 most visited urls are in the cache BUG=40407, 179576, 715902,717236 ==========
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 chengx@chromium.org to run a CQ dry run
Patchset #1 (id:1) 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...
Description was changed from ========== Cancel jumplist update if top 5 most visited urls are in the cache BUG=40407, 179576, 715902,717236 ========== to ========== Cancel jumplist update if top 5 most visited urls are in the cache This change cancels any jumplist update if the top 5 most visited urls are already in the cache, even if their order has changed. Previously, if there BUG=40407, 179576, 715902,717236 ==========
The CQ bit was checked by chengx@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...
Description was changed from ========== Cancel jumplist update if top 5 most visited urls are in the cache This change cancels any jumplist update if the top 5 most visited urls are already in the cache, even if their order has changed. Previously, if there BUG=40407, 179576, 715902,717236 ========== to ========== Cancel JumpList update if top 5 most visited URLs are in the cache This change cancels any JumpList update if the top 5 most visited URLs are already in the cache, even if their order has changed. Previously, if TopSites service detects any change in the top 24 most visited URLs, a JumpList update is scheduled. However, since only the top 5 URLs are displayed in JumpList, we should only care about the changes in them. Also, the order of display for those 5 URLs doesn't matter that much as long as they all show up. BUG=40407, 179576, 715902,717236 ==========
Description was changed from ========== Cancel JumpList update if top 5 most visited URLs are in the cache This change cancels any JumpList update if the top 5 most visited URLs are already in the cache, even if their order has changed. Previously, if TopSites service detects any change in the top 24 most visited URLs, a JumpList update is scheduled. However, since only the top 5 URLs are displayed in JumpList, we should only care about the changes in them. Also, the order of display for those 5 URLs doesn't matter that much as long as they all show up. BUG=40407, 179576, 715902,717236 ========== to ========== Cancel jumplist update if top 5 most visited urls are unchanged Previously, if TopSites service detects any change in the top 24 most visited URLs, a JumpList update is scheduled. However, since only the top 5 URLs are displayed in JumpList, updates caused by changes outside of the top 5 URLs are wasted. This CL trims out the waste. BUG=40407, 179576, 715902,717236 ==========
The CQ bit was checked by chengx@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 #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Cancel jumplist update if top 5 most visited urls are unchanged Previously, if TopSites service detects any change in the top 24 most visited URLs, a JumpList update is scheduled. However, since only the top 5 URLs are displayed in JumpList, updates caused by changes outside of the top 5 URLs are wasted. This CL trims out the waste. BUG=40407, 179576, 715902,717236 ========== to ========== Cancel jumplist update if top 5 most visited urls are unchanged Previously, if TopSites service detects any change in the top 24 most visited URLs, a JumpList update is scheduled. However, since only the top 5 URLs are displayed in JumpList, updates caused by the changes outside of the top 5 URLs are wasted. This CL trims out that waste. BUG=40407, 179576, 715902,717236 ==========
Description was changed from ========== Cancel jumplist update if top 5 most visited urls are unchanged Previously, if TopSites service detects any change in the top 24 most visited URLs, a JumpList update is scheduled. However, since only the top 5 URLs are displayed in JumpList, updates caused by the changes outside of the top 5 URLs are wasted. This CL trims out that waste. BUG=40407, 179576, 715902,717236 ========== to ========== Cancel jumplist update if top 5 most visited urls are unchanged Previously, if TopSites service detects any changes in the top 24 most visited URLs, a JumpList update is scheduled. However, since only the top 5 URLs are displayed in JumpList, updates caused by the changes outside of the top 5 URLs are wasted. This CL trims out that waste. BUG=40407, 179576, 715902,717236 ==========
Description was changed from ========== Cancel jumplist update if top 5 most visited urls are unchanged Previously, if TopSites service detects any changes in the top 24 most visited URLs, a JumpList update is scheduled. However, since only the top 5 URLs are displayed in JumpList, updates caused by the changes outside of the top 5 URLs are wasted. This CL trims out that waste. BUG=40407, 179576, 715902,717236 ========== to ========== Cancel JumpList update if top 5 most visited URLs are unchanged Previously, if TopSites service detects any changes in the top 24 most visited URLs, a JumpList update is scheduled. However, since only the top 5 URLs are displayed in JumpList, updates caused by the changes outside of the top 5 URLs are wasted. This CL trims out that waste. BUG=40407, 179576, 715902,717236 ==========
Description was changed from ========== Cancel JumpList update if top 5 most visited URLs are unchanged Previously, if TopSites service detects any changes in the top 24 most visited URLs, a JumpList update is scheduled. However, since only the top 5 URLs are displayed in JumpList, updates caused by the changes outside of the top 5 URLs are wasted. This CL trims out that waste. BUG=40407, 179576, 715902,717236 ========== to ========== Cancel the JumpList update if top 5 most visited URLs are unchanged Previously, if TopSites service detects any changes in the top 24 most visited URLs, a JumpList update is scheduled. However, since only the top 5 URLs are displayed in JumpList, updates caused by the changes outside of the top 5 URLs are wasted. This CL trims out that waste. BUG=40407, 179576, 715902,717236 ==========
Description was changed from ========== Cancel the JumpList update if top 5 most visited URLs are unchanged Previously, if TopSites service detects any changes in the top 24 most visited URLs, a JumpList update is scheduled. However, since only the top 5 URLs are displayed in JumpList, updates caused by the changes outside of the top 5 URLs are wasted. This CL trims out that waste. BUG=40407, 179576, 715902,717236 ========== to ========== Cancel the JumpList update if top 5 most visited URLs are unchanged Previously, if TopSites service detects any changes in the top 24 most visited URLs, a JumpList update is scheduled. However, since only the top 5 URLs are displayed in JumpList, updates caused by the changes outside of the top 5 URLs are wasted. This CL trims out that waste. BUG=40407, 179576, 715902, 717236 ==========
The CQ bit was checked by chengx@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 checked by chengx@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.
chengx@chromium.org changed reviewers: + grt@chromium.org
PTAL, thanks!
https://codereview.chromium.org/2896233003/diff/100001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2896233003/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:296: bool need_update = false; i think it makes sense to make a helper function for this new code. this here could then be: // There is nothing to do if the top most visited sites have not changed. if (!HasNewMostVisitedItems(data->most_visited_pages_, urls)) return; then you could also write a unit test for this function that would catch the out-of-bounds read below. https://codereview.chromium.org/2896233003/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:298: // If the number of the top sites going to be displayed is larger than the although it's unexpected, i think you also want to schedule an update if urls.size() < data->most_visited_pages_.size(), no? otherwise the loop below will run off the end of |urls|. https://codereview.chromium.org/2896233003/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:306: if (!need_update) { how about something like this: #include <algorithm> #include <iterator> if (std::equal(std::begin(data->most_visited_pages_), std::end(data->most_visited_pages_), std::begin(urls), [](const auto& item_ptr, const auto& most_visited_url) { return item_ptr->url() == most_visited_url.url.spec(); })) { return; }
The CQ bit was checked by chengx@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.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Patchset #3 (id:120001) 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: 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 chengx@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.
PTAL, thanks~ https://codereview.chromium.org/2896233003/diff/100001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2896233003/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:296: bool need_update = false; On 2017/05/24 07:22:12, grt (UTC plus 2) wrote: > i think it makes sense to make a helper function for this new code. this here > could then be: > > // There is nothing to do if the top most visited sites have not changed. > if (!HasNewMostVisitedItems(data->most_visited_pages_, urls)) > return; > > then you could also write a unit test for this function that would catch the > out-of-bounds read below. I've added the helper function. I think it's better to put this method in the anonymous namespace, so it's very tricky to make a unit test for it. There's also a bug filed for adding a unit test suite for JumpList class. Honestly, I have put it aside now as it's more complicated. https://codereview.chromium.org/2896233003/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:298: // If the number of the top sites going to be displayed is larger than the On 2017/05/24 07:22:12, grt (UTC plus 2) wrote: > although it's unexpected, i think you also want to schedule an update if > urls.size() < data->most_visited_pages_.size(), no? otherwise the loop below > will run off the end of |urls|. Done. You're right. https://codereview.chromium.org/2896233003/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:306: if (!need_update) { On 2017/05/24 07:22:12, grt (UTC plus 2) wrote: > how about something like this: > > #include <algorithm> > #include <iterator> > > if (std::equal(std::begin(data->most_visited_pages_), > std::end(data->most_visited_pages_), > std::begin(urls), > [](const auto& item_ptr, const auto& most_visited_url) { > return item_ptr->url() == most_visited_url.url.spec(); > })) { > return; > } Done. This is really impressive!
not lgtm. i understand that making unit tests for the entire jumplist machine is challenging. making tests for the new code should be a smaller undertaking and is worthwhile. human code review skills are only so good. additionally, a one-time code review does not prevent accidental regressions from being introduced in future CLs. https://codereview.chromium.org/2896233003/diff/140001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2896233003/diff/140001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:212: return std::equal(std::begin(items), std::end(items), std::begin(urls), shouldn't this return false if the ranges are equal since that indicates that the top N urls haven't changed?
Patchset #4 (id:160001) has been deleted
The CQ bit was checked by chengx@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.
I was naively thinking I need to instantiate the JumpList class and construct
many things to test the added method. So I hesitated to add the unittest. But
obviously I was wrong. Making unittests for the newly added method is fairly
easy. I've added the unittest in the new patch set. PTAL, thanks!
I don't think the new method "MostVisitedItemsUnchanged()" fits the existing
jumplist_file_util.{h,cc} which is all about file operation on disk. Therefore,
I created jumplist_update_util.{h,cc}. It's a little sad that it only has one
method now, but it'll have more in the future once I create new util methods.
On 2017/05/29 07:14:44, grt (UTC plus 2) wrote:
> not lgtm. i understand that making unit tests for the entire jumplist machine
is
> challenging. making tests for the new code should be a smaller undertaking and
> is worthwhile. human code review skills are only so good. additionally, a
> one-time code review does not prevent accidental regressions from being
> introduced in future CLs.
https://codereview.chromium.org/2896233003/diff/140001/chrome/browser/win/jum...
File chrome/browser/win/jumplist.cc (right):
https://codereview.chromium.org/2896233003/diff/140001/chrome/browser/win/jum...
chrome/browser/win/jumplist.cc:212: return std::equal(std::begin(items),
std::end(items), std::begin(urls),
On 2017/05/29 07:14:44, grt (UTC plus 2) wrote:
> shouldn't this return false if the ranges are equal since that indicates that
> the top N urls haven't changed
You're right. I've fixed this in the new patch set.
The CQ bit was checked by chengx@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.
Patchset #4 (id:180001) has been deleted
great! https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:300: kMostVisitedItems)) nit: add braces for multi-line conditional https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... File chrome/browser/win/jumplist_update_util.cc (right): https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util.cc:8: #include "chrome/browser/win/jumplist_update_util.h" nit: move up above other includes (https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes) https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util.cc:12: size_t most_visited_items) { nit: max_item_count? https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util.cc:19: if (size_t(std::min(urls.size(), most_visited_items)) != items.size()) urls.size() and most_visited_items are both size_t. why is the cast necessary? https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... File chrome/browser/win/jumplist_update_util.h (right): https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. nit: omit "(c)" (https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...) https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... File chrome/browser/win/jumplist_update_util_unittest.cc (right): https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:7: #include "algorithm" <algorithm> https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:9: #include "base/containers/hash_tables.h" a comment in this file says: // This header file is deprecated. Use the corresponding C++11 type // instead. https://crbug.com/576864 https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:24: scoped_refptr<ShellLinkItem> item(new ShellLinkItem); auto item = base::MakeRefCounted<ShellLinkItem>(); https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:31: class JumpListUpdateUtilTest : public testing::Test { i don't think this test fixture adds any value as-is. how about removing it and using TEST below? https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:41: // Initialize item_list and url_list using the url_title map. replace url_title in the unnamed namespace with the following here: static constexpr struct { const char* url; const wchar_t* title; } kTestData[] = { {"https://www.google.com/", L"Google"}, {"https://www.youtube.com/", L"Youtube"}, {"https://www.gmail.com/", L"Gmail"}}; https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:45: GURL url(base::UTF8ToUTF16(url_title_pair.first)); this conversion isn't needed -- GURL has a ctor that takes a narrow string https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:47: url, base::UTF8ToUTF16(url_title_pair.second)); remove this conversion in favor of using a wchar_t literal in the test data (as above). https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:70: item_list.pop_back(); i'm finding it hard to follow what "url_list" and "item_list" are. i think it would be more clear if they were called "history_items" and "jumplist_items" (or "shell_links" or something).
The CQ bit was checked by chengx@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've addressed the comments in the new patch set. PTAL, thanks! https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:300: kMostVisitedItems)) On 2017/05/30 08:51:41, grt (UTC plus 2) wrote: > nit: add braces for multi-line conditional Done. https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... File chrome/browser/win/jumplist_update_util.cc (right): https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util.cc:8: #include "chrome/browser/win/jumplist_update_util.h" On 2017/05/30 08:51:41, grt (UTC plus 2) wrote: > nit: move up above other includes > (https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes) Done. https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util.cc:12: size_t most_visited_items) { On 2017/05/30 08:51:41, grt (UTC plus 2) wrote: > nit: max_item_count? Done. https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util.cc:19: if (size_t(std::min(urls.size(), most_visited_items)) != items.size()) On 2017/05/30 08:51:41, grt (UTC plus 2) wrote: > urls.size() and most_visited_items are both size_t. why is the cast necessary? Yes, it should be removed. https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... File chrome/browser/win/jumplist_update_util.h (right): https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/05/30 08:51:42, grt (UTC plus 2) wrote: > nit: omit "(c)" > (https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...) Done. I also removed "(c)" for other header files. https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... File chrome/browser/win/jumplist_update_util_unittest.cc (right): https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:7: #include "algorithm" On 2017/05/30 08:51:42, grt (UTC plus 2) wrote: > <algorithm> Done. https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:9: #include "base/containers/hash_tables.h" On 2017/05/30 08:51:42, grt (UTC plus 2) wrote: > a comment in this file says: > > // This header file is deprecated. Use the corresponding C++11 type > // instead. https://crbug.com/576864 I've removed this header file since kTestData doesn't use a map anymore. https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:24: scoped_refptr<ShellLinkItem> item(new ShellLinkItem); On 2017/05/30 08:51:42, grt (UTC plus 2) wrote: > auto item = base::MakeRefCounted<ShellLinkItem>(); Done. https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:31: class JumpListUpdateUtilTest : public testing::Test { On 2017/05/30 08:51:42, grt (UTC plus 2) wrote: > i don't think this test fixture adds any value as-is. how about removing it and > using TEST below? Done. https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:41: // Initialize item_list and url_list using the url_title map. On 2017/05/30 08:51:42, grt (UTC plus 2) wrote: > replace url_title in the unnamed namespace with the following here: > > static constexpr struct { > const char* url; > const wchar_t* title; > } kTestData[] = { > {"https://www.google.com/", L"Google"}, > {"https://www.youtube.com/", L"Youtube"}, > {"https://www.gmail.com/", L"Gmail"}}; Done. https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:45: GURL url(base::UTF8ToUTF16(url_title_pair.first)); On 2017/05/30 08:51:42, grt (UTC plus 2) wrote: > this conversion isn't needed -- GURL has a ctor that takes a narrow string Done. https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:47: url, base::UTF8ToUTF16(url_title_pair.second)); On 2017/05/30 08:51:42, grt (UTC plus 2) wrote: > remove this conversion in favor of using a wchar_t literal in the test data (as > above). Done. https://codereview.chromium.org/2896233003/diff/200001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:70: item_list.pop_back(); On 2017/05/30 08:51:42, grt (UTC plus 2) wrote: > i'm finding it hard to follow what "url_list" and "item_list" are. i think it > would be more clear if they were called "history_items" and "jumplist_items" (or > "shell_links" or something). I changed to history_items and jumplist_items.
Description was changed from ========== Cancel the JumpList update if top 5 most visited URLs are unchanged Previously, if TopSites service detects any changes in the top 24 most visited URLs, a JumpList update is scheduled. However, since only the top 5 URLs are displayed in JumpList, updates caused by the changes outside of the top 5 URLs are wasted. This CL trims out that waste. BUG=40407, 179576, 715902, 717236 ========== to ========== Cancel the JumpList update if top 5 most visited URLs are unchanged Previously, if TopSites service detects any changes in the top 24 most visited URLs, a JumpList update is scheduled. However, since only the top 5 URLs are displayed in JumpList, updates caused by the changes outside of the top 5 URLs are wasted. This CL trims out that waste. BUG=40407, 179576, 715902, 717236, 498987 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by chengx@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by chengx@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.
lgtm with a few nits. https://codereview.chromium.org/2896233003/diff/220001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (left): https://codereview.chromium.org/2896233003/diff/220001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. don't update old headers like this as per https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2896233003/diff/220001/chrome/browser/win/jum... File chrome/browser/win/jumplist_update_util_unittest.cc (right): https://codereview.chromium.org/2896233003/diff/220001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:15: static constexpr struct { move this into the test function as per https://google.github.io/styleguide/cppguide.html#Local_Variables https://codereview.chromium.org/2896233003/diff/220001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:35: for (const auto& pair : kTestData) { nit: pair -> test_data or something https://codereview.chromium.org/2896233003/diff/220001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:37: GURL url(pair.url); replace these next three lines with: history_items.emplace_back(GURL(pair.url), pair.title);
The CQ bit was checked by chengx@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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) 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...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) 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_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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_headless_rel 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_...) linux_chromium_tsan_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 chengx@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.
I've addressed all the comments in the new patch set. Thanks! https://codereview.chromium.org/2896233003/diff/220001/chrome/browser/win/jum... File chrome/browser/win/jumplist.h (left): https://codereview.chromium.org/2896233003/diff/220001/chrome/browser/win/jum... chrome/browser/win/jumplist.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/05/31 06:59:53, grt (UTC plus 2) wrote: > don't update old headers like this as per > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2896233003/diff/220001/chrome/browser/win/jum... File chrome/browser/win/jumplist_update_util_unittest.cc (right): https://codereview.chromium.org/2896233003/diff/220001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:15: static constexpr struct { On 2017/05/31 06:59:53, grt (UTC plus 2) wrote: > move this into the test function as per > https://google.github.io/styleguide/cppguide.html#Local_Variables Done. https://codereview.chromium.org/2896233003/diff/220001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:35: for (const auto& pair : kTestData) { On 2017/05/31 06:59:53, grt (UTC plus 2) wrote: > nit: pair -> test_data or something Done. https://codereview.chromium.org/2896233003/diff/220001/chrome/browser/win/jum... chrome/browser/win/jumplist_update_util_unittest.cc:37: GURL url(pair.url); On 2017/05/31 06:59:53, grt (UTC plus 2) wrote: > replace these next three lines with: > history_items.emplace_back(GURL(pair.url), pair.title); This is super handy! Thanks!
The CQ bit was checked by chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org Link to the patchset: https://codereview.chromium.org/2896233003/#ps260001 (title: "Fix 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": 260001, "attempt_start_ts": 1496263588847100,
"parent_rev": "027cc2dcd09b534729547c2b234238a61a05cee3", "commit_rev":
"61e622b8b398999d4884231999375245c07dad01"}
Message was sent while issue was closed.
Description was changed from ========== Cancel the JumpList update if top 5 most visited URLs are unchanged Previously, if TopSites service detects any changes in the top 24 most visited URLs, a JumpList update is scheduled. However, since only the top 5 URLs are displayed in JumpList, updates caused by the changes outside of the top 5 URLs are wasted. This CL trims out that waste. BUG=40407, 179576, 715902, 717236, 498987 ========== to ========== Cancel the JumpList update if top 5 most visited URLs are unchanged Previously, if TopSites service detects any changes in the top 24 most visited URLs, a JumpList update is scheduled. However, since only the top 5 URLs are displayed in JumpList, updates caused by the changes outside of the top 5 URLs are wasted. This CL trims out that waste. BUG=40407, 179576, 715902, 717236, 498987 Review-Url: https://codereview.chromium.org/2896233003 Cr-Commit-Position: refs/heads/master@{#476017} Committed: https://chromium.googlesource.com/chromium/src/+/61e622b8b398999d488423199937... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:260001) as https://chromium.googlesource.com/chromium/src/+/61e622b8b398999d488423199937... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
