Disable baked-in popular sites for M60 iOS only. |
||||||||||||||
Issue descriptionThe launch of baked-in popular sites was postponed to M61 (see issue 725961). 1. Therefore, disable the feature on iOS only. 2. Merge it into M60. 3. Reenable the feature, so it's present on Dev/Canay M61.
,
Jun 29 2017
,
Jun 29 2017
Just to avoid ambiguity: Please disable this client-side (hardcoded) as opposed to just via a Finch config change :).
,
Jun 29 2017
Are we tracking disabling it for M59 somewhere else?
,
Jun 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/61b617a08bebd805857faae0eaefed7315b5fdcd commit 61b617a08bebd805857faae0eaefed7315b5fdcd Author: fhorschig <fhorschig@chromium.org> Date: Fri Jun 30 11:33:47 2017 Disable baked-in popular sites for iOS. The launch of this feature has been postponed. As Finch does not guarantee that this feature is disabled, disable it in the code. (Reenabling this feature will happen after disabling was merged to M60). BUG= 737607 Review-Url: https://codereview.chromium.org/2959193002 Cr-Commit-Position: refs/heads/master@{#483677} [modify] https://crrev.com/61b617a08bebd805857faae0eaefed7315b5fdcd/components/ntp_tiles/popular_sites_impl.cc [modify] https://crrev.com/61b617a08bebd805857faae0eaefed7315b5fdcd/components/ntp_tiles/popular_sites_impl_unittest.cc
,
Jun 30 2017
We won't respin 59 for this, which would be the only way to fully turn this off in 59. The finch for 59 is set to 0% so as soon as user gets the new finch configs they won't see this feature on M59. @fhorschig please correct if anything here is not your understanding as well.
,
Jun 30 2017
Absolutely correct. I added you and cmasso@ to the conversation.
,
Jun 30 2017
Requesting merge to disable this feature in M60.
,
Jun 30 2017
This bug requires manual review: Less than 21 days to go before AppStore submit on M60 Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0380078f4a760a0fe2725d4c84d3edbe3987d99e commit 0380078f4a760a0fe2725d4c84d3edbe3987d99e Author: fhorschig <fhorschig@chromium.org> Date: Mon Jul 03 08:45:23 2017 Revert of Disable baked-in popular sites for iOS. (patchset #1 id:1 of https://codereview.chromium.org/2959193002/ ) Reason for revert: Reverting the disabling makes baked-in popular sites available on Dev/Canary M61+ again. Disabling baked-in popular sites is only needed for M60. (50% Beta Approval is given for M61 in issue 725961) Original issue's description: > Disable baked-in popular sites for iOS. > > The launch of this feature has been postponed. > As Finch does not guarantee that this feature is disabled, > disable it in the code. > (Reenabling this feature will happen after disabling was merged to M60). > > BUG= 737607 > > Review-Url: https://codereview.chromium.org/2959193002 > Cr-Commit-Position: refs/heads/master@{#483677} > Committed: https://chromium.googlesource.com/chromium/src/+/61b617a08bebd805857faae0eaefed7315b5fdcd TBR=sfiera@chromium.org BUG= 737607 Review-Url: https://codereview.chromium.org/2962363002 Cr-Commit-Position: refs/heads/master@{#483963} [modify] https://crrev.com/0380078f4a760a0fe2725d4c84d3edbe3987d99e/components/ntp_tiles/popular_sites_impl.cc [modify] https://crrev.com/0380078f4a760a0fe2725d4c84d3edbe3987d99e/components/ntp_tiles/popular_sites_impl_unittest.cc
,
Jul 5 2017
,
Jul 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c5233b020d1c6a554d55b1623ea6ce6f4cc4366b commit c5233b020d1c6a554d55b1623ea6ce6f4cc4366b Author: Friedrich Horschig <fhorschig@chromium.org> Date: Thu Jul 06 09:51:06 2017 Disable baked-in popular sites for iOS. The launch of this feature has been postponed. As Finch does not guarantee that this feature is disabled, disable it in the code. (Reenabling this feature will happen after disabling was merged to M60). BUG= 737607 TBR=fhorschig@chromium.org (cherry picked from commit 61b617a08bebd805857faae0eaefed7315b5fdcd) Review-Url: https://codereview.chromium.org/2959193002 Cr-Original-Commit-Position: refs/heads/master@{#483677} Change-Id: I2ed7ca0b59550689cb2ebbac4ffbcd01748e5b9e Reviewed-on: https://chromium-review.googlesource.com/561148 Reviewed-by: Friedrich Horschig <fhorschig@chromium.org> Cr-Commit-Position: refs/branch-heads/3112@{#526} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/c5233b020d1c6a554d55b1623ea6ce6f4cc4366b/components/ntp_tiles/popular_sites_impl.cc [modify] https://crrev.com/c5233b020d1c6a554d55b1623ea6ce6f4cc4366b/components/ntp_tiles/popular_sites_impl_unittest.cc
,
Jul 6 2017
,
Jul 6 2017
,
Jul 7 2017
Lindsay, can you pls verify that the feature was properly disabled in M60 now?
,
Jul 7 2017
M60 builds are currently failing to build. http://crbug/739706. Will verify once we get a new build. fhorschig@ Can you please let me know if the below steps would be good to verify this bug. 1. Fresh Install Chrome M60 2. Turn ON Airplane Mode 3. Launch Chrome, Skip Sign in in Welcome Screen Expected result: NTP will be displayed with Popular sites tiles but NO favicons. Only Grey tiles with letters should be displayed.
,
Jul 10 2017
The error makes sense to me. I find it very strange, that it did not appear locally or on trybots before. Although it's a one-liner, I would suggest to merge the revert and reland disabling the feature including the fix.
,
Jul 10 2017
,
Jul 10 2017
Adding label to request merge for the revert.
,
Jul 10 2017
This bug requires manual review: Less than 11 days to go before AppStore submit on M60 Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2011ecdf7883b89225cecad8e681241ec64342c9 commit 2011ecdf7883b89225cecad8e681241ec64342c9 Author: Friedrich Horschig <fhorschig@chromium.org> Date: Mon Jul 10 17:03:38 2017 [Respin] Disable baked-in popular sites for iOS After discovering that the unused |SetDefaultResourceForSite| breaks the current Beta build, this CL redoes https://crrev.com/2959193002/ and disables the baked-in popular sites for iOS. Bug: 737607 Change-Id: I6ba17e95a2a81d9ec9e06580ab1248b2fc24faac Reviewed-on: https://chromium-review.googlesource.com/564614 Reviewed-by: Chris Pickel <sfiera@chromium.org> Commit-Queue: Friedrich Horschig <fhorschig@chromium.org> Cr-Commit-Position: refs/heads/master@{#485299} [modify] https://crrev.com/2011ecdf7883b89225cecad8e681241ec64342c9/components/ntp_tiles/popular_sites_impl.cc [modify] https://crrev.com/2011ecdf7883b89225cecad8e681241ec64342c9/components/ntp_tiles/popular_sites_impl_unittest.cc
,
Jul 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7f4a97fb108cbdb7998584b8c88963078e401385 commit 7f4a97fb108cbdb7998584b8c88963078e401385 Author: Friedrich Horschig <fhorschig@chromium.org> Date: Mon Jul 10 17:50:57 2017 [Respin] Reenable baked-in popular sites This cl complements https://chromium-review.googlesource.com/c/564614/ in that it reenables the baked-in popular sites for iOS. The reason for not reverting the old CL is the includes that were needed to make the presubmit happy. Bug: 737607 Change-Id: I24020f6949aff679bc80a918f61610d32056ceda Reviewed-on: https://chromium-review.googlesource.com/565565 Reviewed-by: Chris Pickel <sfiera@chromium.org> Commit-Queue: Friedrich Horschig <fhorschig@chromium.org> Cr-Commit-Position: refs/heads/master@{#485309} [modify] https://crrev.com/7f4a97fb108cbdb7998584b8c88963078e401385/components/ntp_tiles/popular_sites_impl.cc [modify] https://crrev.com/7f4a97fb108cbdb7998584b8c88963078e401385/components/ntp_tiles/popular_sites_impl_unittest.cc
,
Jul 10 2017
,
Jul 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4459d4fac0fe38e10de783241e7f6486bbb31b1b commit 4459d4fac0fe38e10de783241e7f6486bbb31b1b Author: Friedrich Horschig <fhorschig@chromium.org> Date: Tue Jul 11 08:46:32 2017 Revert of Disable baked-in popular sites for iOS. (patchset #1 id:1 of https://codereview.chromium.org/2959193002/ ) Reason for revert: One platform check was still in place which causes the current Beta to fail compilation. In order to keep the history intact, we revert the disabling, fix the bug and disable again. Original issue's description: > Disable baked-in popular sites for iOS. > > The launch of this feature has been postponed. > As Finch does not guarantee that this feature is disabled, > disable it in the code. > (Reenabling this feature will happen after disabling was merged to M60). > > BUG= 737607 > > Review-Url: https://codereview.chromium.org/2959193002 > Cr-Commit-Position: refs/heads/master@{#483677} > Committed: https://chromium.googlesource.com/chromium/src/+/61b617a08bebd805857faae0eaefed7315b5fdcd TBR=fhorschig@chromium.org, sfiera@chromium.org BUG= 737607 (cherry picked from commit 0380078f4a760a0fe2725d4c84d3edbe3987d99e) Review-Url: https://codereview.chromium.org/2962363002 Cr-Original-Commit-Position: refs/heads/master@{#483963} Change-Id: I8b5baa7629c87d5c4bded8d1e70a4eb60b41fe20 Reviewed-on: https://chromium-review.googlesource.com/566858 Reviewed-by: Friedrich Horschig <fhorschig@chromium.org> Cr-Commit-Position: refs/branch-heads/3112@{#577} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/4459d4fac0fe38e10de783241e7f6486bbb31b1b/components/ntp_tiles/popular_sites_impl.cc [modify] https://crrev.com/4459d4fac0fe38e10de783241e7f6486bbb31b1b/components/ntp_tiles/popular_sites_impl_unittest.cc
,
Jul 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5f8053608b28025b86de489cc24ffd9996b98431 commit 5f8053608b28025b86de489cc24ffd9996b98431 Author: Friedrich Horschig <fhorschig@chromium.org> Date: Tue Jul 11 08:48:08 2017 [Respin] Disable baked-in popular sites for iOS After discovering that the unused |SetDefaultResourceForSite| breaks the current Beta build, this CL redoes https://crrev.com/2959193002/ and disables the baked-in popular sites for iOS. TBR=fhorschig@chromium.org (cherry picked from commit 2011ecdf7883b89225cecad8e681241ec64342c9) Bug: 737607 Change-Id: I6ba17e95a2a81d9ec9e06580ab1248b2fc24faac Reviewed-on: https://chromium-review.googlesource.com/564614 Reviewed-by: Chris Pickel <sfiera@chromium.org> Commit-Queue: Friedrich Horschig <fhorschig@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#485299} Reviewed-on: https://chromium-review.googlesource.com/566819 Reviewed-by: Friedrich Horschig <fhorschig@chromium.org> Cr-Commit-Position: refs/branch-heads/3112@{#578} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/5f8053608b28025b86de489cc24ffd9996b98431/components/ntp_tiles/popular_sites_impl.cc [modify] https://crrev.com/5f8053608b28025b86de489cc24ffd9996b98431/components/ntp_tiles/popular_sites_impl_unittest.cc
,
Jul 11 2017
,
Jul 11 2017
Hi Claude, Can you please kick off another M60 build with this fix included so we can verify that 60 is building now and verify this fix? This is presumably blocking beta this week as well (see issue 739706). Thanks, |
||||||||||||||
►
Sign in to add a comment |
||||||||||||||
Comment 1 by mard...@chromium.org
, Jun 29 2017