Regression: Functionality of 'Restore Default shortcuts' is working when the option is disabled on NTP
Reported by
sanyam.g...@etouch.net,
Aug 30
|
||||||||||
Issue descriptionChrome Version: 70.0.3537.0 (Official Build)Revision 57f65e033d6d9160457a612f5033171bdfeaca42-refs/branch-heads/3537@{#1}(32/64-bit) OS: Windows(7,8,8.1,10), Mac(10.12.6 , 10.13.1 , 10.13.6, 10.14) & Linux(14.04 LTS) Pre-condition: Enable "Enable using the Google local NTP" ,"New Tab Page Background Selection" and "New Tab Page Custom Links" flags under chrome://flags. What steps will reproduce the problem? (1) Launch chrome and navigate to NTP. (2) Remove the shortcuts then click on 'Gear' icon to open the 'Customize this page' overlay. (3) Now click on 'Restore Default shortcuts' and observe. Actual Result : Functionality of 'Restore Default shortcuts' is working when the option is disabled. Expected Result: Functionality of 'Restore Default shortcuts' should not work when the option is disabled.. This is a regression issue, broken in 'M-70', below is per-revision bisect info: Good Build:70.0.3508.0(Revision: 579242) Bad Build: 70.0.3509.0(Revision: 579853) You are probably looking for a change made after 579298 (known good), but no later than 579299 (first known bad). CHANGE-LOG URL: The script might not always return single CL as suspect as some perf builds might get missing due to failure. https://chromium.googlesource.com/chromium/src/+log/fdd14161e14fe8d2f0578ebddd4578a4e1f674cb..7a26d4f6d9ab30f8550e8af4bb9ef65ca5740a6f Suspect: https://chromium.googlesource.com/chromium/src/+/7a26d4f6d9ab30f8550e8af4bb9ef65ca5740a6f @kristipark: Could you please help to reassign if your change is not the cause for this change. Kindly review the attached screen-cast for reference. Thank You..!!
,
Aug 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37 commit 9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37 Author: kristipark <kristipark@chromium.org> Date: Thu Aug 30 23:21:14 2018 [NTP] Add isCustomLink property to EmbeddedSearchAPI The property is used to determine if we need to show the "Restore default shortcuts" option. Previously this was calculated by checking the tile source of the most visited items, but this did not account for the case where there are no available items. As such, if there were no custom links, the restore default option was disabled. Bug: 879082 Change-Id: Ia15f4fc863642b520234c061ee5ecf6f7a54c50e Reviewed-on: https://chromium-review.googlesource.com/1197465 Commit-Queue: Kristi Park <kristipark@chromium.org> Reviewed-by: Mustafa Emre Acer <meacer@chromium.org> Reviewed-by: Mathieu Perreault <mathp@chromium.org> Cr-Commit-Position: refs/heads/master@{#587835} [modify] https://crrev.com/9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37/chrome/browser/resources/local_ntp/most_visited_single.js [modify] https://crrev.com/9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37/chrome/browser/search/instant_service.cc [modify] https://crrev.com/9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37/chrome/browser/search/instant_service_observer.cc [modify] https://crrev.com/9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37/chrome/browser/search/instant_service_observer.h [modify] https://crrev.com/9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37/chrome/browser/ui/search/local_ntp_backgrounds_browsertest.cc [modify] https://crrev.com/9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37/chrome/browser/ui/search/local_ntp_browsertest.cc [modify] https://crrev.com/9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37/chrome/browser/ui/search/search_ipc_router.cc [modify] https://crrev.com/9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37/chrome/browser/ui/search/search_ipc_router.h [modify] https://crrev.com/9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37/chrome/browser/ui/search/search_ipc_router_unittest.cc [modify] https://crrev.com/9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37/chrome/browser/ui/search/search_tab_helper.cc [modify] https://crrev.com/9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37/chrome/browser/ui/search/search_tab_helper.h [modify] https://crrev.com/9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37/chrome/common/search.mojom [modify] https://crrev.com/9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37/chrome/common/search/mock_embedded_search_client.h [modify] https://crrev.com/9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37/chrome/renderer/searchbox/searchbox.cc [modify] https://crrev.com/9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37/chrome/renderer/searchbox/searchbox.h [modify] https://crrev.com/9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37/chrome/renderer/searchbox/searchbox_extension.cc
,
Aug 30
,
Aug 30
[Auto-generated comment by a script] We noticed that this issue is targeted for M-70; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-70 label, otherwise remove Merge-TBD label. Thanks.
,
Sep 4
Verified that this is fixed in Canary. Requesting merge to M70.
,
Sep 5
Your change meets the bar and is auto-approved for M70. Please go ahead and merge the CL to branch 3538 manually. Please contact milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 5
Merged to M70 in https://chromium-review.googlesource.com/c/chromium/src/+/1208130 (bugdroid appears to be disabled)
,
Sep 5
,
Sep 5
Adding merge label
,
Sep 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ea0517fb85b6832025561e274f6346878abf92f2 commit ea0517fb85b6832025561e274f6346878abf92f2 Author: kristipark <kristipark@chromium.org> Date: Wed Sep 05 18:31:05 2018 [Merge M70] [NTP] Add isCustomLink property to EmbeddedSearchAPI The property is used to determine if we need to show the "Restore default shortcuts" option. Previously this was calculated by checking the tile source of the most visited items, but this did not account for the case where there are no available items. As such, if there were no custom links, the restore default option was disabled. Bug: 879082 Change-Id: Ia15f4fc863642b520234c061ee5ecf6f7a54c50e Reviewed-on: https://chromium-review.googlesource.com/1197465 Commit-Queue: Kristi Park <kristipark@chromium.org> Reviewed-by: Mustafa Emre Acer <meacer@chromium.org> Reviewed-by: Mathieu Perreault <mathp@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#587835}(cherry picked from commit 9a51cfb2f0128ad1e7758586c5cbaba88e5a4a37) Reviewed-on: https://chromium-review.googlesource.com/1208130 Reviewed-by: Kristi Park <kristipark@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#59} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/ea0517fb85b6832025561e274f6346878abf92f2/chrome/browser/resources/local_ntp/most_visited_single.js [modify] https://crrev.com/ea0517fb85b6832025561e274f6346878abf92f2/chrome/browser/search/instant_service.cc [modify] https://crrev.com/ea0517fb85b6832025561e274f6346878abf92f2/chrome/browser/search/instant_service_observer.cc [modify] https://crrev.com/ea0517fb85b6832025561e274f6346878abf92f2/chrome/browser/search/instant_service_observer.h [modify] https://crrev.com/ea0517fb85b6832025561e274f6346878abf92f2/chrome/browser/ui/search/local_ntp_backgrounds_browsertest.cc [modify] https://crrev.com/ea0517fb85b6832025561e274f6346878abf92f2/chrome/browser/ui/search/local_ntp_browsertest.cc [modify] https://crrev.com/ea0517fb85b6832025561e274f6346878abf92f2/chrome/browser/ui/search/search_ipc_router.cc [modify] https://crrev.com/ea0517fb85b6832025561e274f6346878abf92f2/chrome/browser/ui/search/search_ipc_router.h [modify] https://crrev.com/ea0517fb85b6832025561e274f6346878abf92f2/chrome/browser/ui/search/search_ipc_router_unittest.cc [modify] https://crrev.com/ea0517fb85b6832025561e274f6346878abf92f2/chrome/browser/ui/search/search_tab_helper.cc [modify] https://crrev.com/ea0517fb85b6832025561e274f6346878abf92f2/chrome/browser/ui/search/search_tab_helper.h [modify] https://crrev.com/ea0517fb85b6832025561e274f6346878abf92f2/chrome/common/search.mojom [modify] https://crrev.com/ea0517fb85b6832025561e274f6346878abf92f2/chrome/common/search/mock_embedded_search_client.h [modify] https://crrev.com/ea0517fb85b6832025561e274f6346878abf92f2/chrome/renderer/searchbox/searchbox.cc [modify] https://crrev.com/ea0517fb85b6832025561e274f6346878abf92f2/chrome/renderer/searchbox/searchbox.h [modify] https://crrev.com/ea0517fb85b6832025561e274f6346878abf92f2/chrome/renderer/searchbox/searchbox_extension.cc
,
Sep 10
Kristi - does anything here need to be copied to the remote NTP? (Pre-emptively adding the label so we don't lose track of it).
,
Sep 10
Nope, this should be gtg |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by rbasuvula@chromium.org
, Aug 30Labels: ReleaseBlock-Stable