Issue metadata
Sign in to add a comment
|
"Site Information" tools menu item is enabled for NTP |
||||||||||||||||||||||
Issue descriptionApp Version (from "Chrome Settings > About Chrome"): 73.0.3643 iOS Version: All Device: All Steps to reproduce: 1.) Make sure that browser-container-contains-ntp is enabled 2.) Open NTP 3.) Open Tools menu Observed behavior: "Site Information" tools menu item is enabled Expected behavior: "Site Information" tools menu item is disabled Additional comments: Not reproducible if browser-container-contains-ntp is disabled
,
Jan 4
,
Jan 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6ccb9d4e041ebc8311b489b454449e8f38e84b9e commit 6ccb9d4e041ebc8311b489b454449e8f38e84b9e Author: Eugene But <eugenebut@google.com> Date: Mon Jan 07 20:49:59 2019 Fix currentWebPageSupportsSiteInfo for NTP. Old code checked if URL is app-specific. When browser-container-contains-ntp flag is enabled, NTP has about:newtab URL and chrome:newtab Virtual URL. This CL checks if Virtual URL is app specific. This CL does not add tests, because PopupMenuMediatorTest fixture does not test enabled state for menu items. This CL has to be cherry picked, so ideally it has to be small. Tests will be hopefully added in a separate CL by the owner of this code ;) Bug: 919148 Change-Id: I4068106e2c559cce0852cb57289af9b0e0a5a9c9 Reviewed-on: https://chromium-review.googlesource.com/c/1396334 Commit-Queue: Eugene But <eugenebut@chromium.org> Reviewed-by: Justin Cohen <justincohen@chromium.org> Reviewed-by: Gauthier Ambard <gambard@chromium.org> Cr-Commit-Position: refs/heads/master@{#620461} [modify] https://crrev.com/6ccb9d4e041ebc8311b489b454449e8f38e84b9e/ios/chrome/browser/ui/popup_menu/popup_menu_mediator.mm
,
Jan 7
Vinutha, could you please verify the fix with the next Canary build.
,
Jan 7
This bug requires manual review: Less than 18 days to go before AppStore submit on M72 Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 8
https://drive.google.com/file/d/1xYja2LTSkLyeKNyw6vfh_C1S3JOc9Jwk/view?usp=sharing Verified on 73.0.3665.0 Canary on iPhone XSMAX iOS 12.0.1, iPad Pro 12'9 iOS 11.4.1
,
Jan 9
Approved.
,
Jan 14
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ae90756a69ab200847da0b72ff0b314796766399 commit ae90756a69ab200847da0b72ff0b314796766399 Author: Eugene But <eugenebut@google.com> Date: Mon Jan 14 19:19:48 2019 Fix currentWebPageSupportsSiteInfo for NTP. Old code checked if URL is app-specific. When browser-container-contains-ntp flag is enabled, NTP has about:newtab URL and chrome:newtab Virtual URL. This CL checks if Virtual URL is app specific. This CL does not add tests, because PopupMenuMediatorTest fixture does not test enabled state for menu items. This CL has to be cherry picked, so ideally it has to be small. Tests will be hopefully added in a separate CL by the owner of this code ;) TBR=eugenebut@google.com (cherry picked from commit 6ccb9d4e041ebc8311b489b454449e8f38e84b9e) Bug: 919148 Change-Id: I4068106e2c559cce0852cb57289af9b0e0a5a9c9 Reviewed-on: https://chromium-review.googlesource.com/c/1396334 Commit-Queue: Eugene But <eugenebut@chromium.org> Reviewed-by: Justin Cohen <justincohen@chromium.org> Reviewed-by: Gauthier Ambard <gambard@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#620461} Reviewed-on: https://chromium-review.googlesource.com/c/1409700 Reviewed-by: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#672} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/ae90756a69ab200847da0b72ff0b314796766399/ios/chrome/browser/ui/popup_menu/popup_menu_mediator.mm
,
Jan 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ae90756a69ab200847da0b72ff0b314796766399 Commit: ae90756a69ab200847da0b72ff0b314796766399 Author: eugenebut@google.com Commiter: eugenebut@chromium.org Date: 2019-01-14 19:19:48 +0000 UTC Fix currentWebPageSupportsSiteInfo for NTP. Old code checked if URL is app-specific. When browser-container-contains-ntp flag is enabled, NTP has about:newtab URL and chrome:newtab Virtual URL. This CL checks if Virtual URL is app specific. This CL does not add tests, because PopupMenuMediatorTest fixture does not test enabled state for menu items. This CL has to be cherry picked, so ideally it has to be small. Tests will be hopefully added in a separate CL by the owner of this code ;) TBR=eugenebut@google.com (cherry picked from commit 6ccb9d4e041ebc8311b489b454449e8f38e84b9e) Bug: 919148 Change-Id: I4068106e2c559cce0852cb57289af9b0e0a5a9c9 Reviewed-on: https://chromium-review.googlesource.com/c/1396334 Commit-Queue: Eugene But <eugenebut@chromium.org> Reviewed-by: Justin Cohen <justincohen@chromium.org> Reviewed-by: Gauthier Ambard <gambard@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#620461} Reviewed-on: https://chromium-review.googlesource.com/c/1409700 Reviewed-by: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#672} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
,
Jan 16
(6 days ago)
Verified on chrome beta version 72.0.3626.59 on iPhone XS max 12.1.2, iPhone 8 plus with iOS 12.1.3 beta 4, and iPad 2018 with iOS 11.4.1, 12.1.1. Site information tools menu item is in disabled state with browser-container-contains-ntp flag enabled.
,
Jan 18
(4 days ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/88410c4f9836ef567f34652a448342dff6e404b4 commit 88410c4f9836ef567f34652a448342dff6e404b4 Author: Gauthier Ambard <gambard@chromium.org> Date: Fri Jan 18 15:56:43 2019 [iOS] Add test for PopupMenuMediator This CL adds a test to check the enabled state of the SiteInfo item on the NTP. Bug: 919148 Change-Id: I54ee8131dbe3cea907563a44f5f27c60a1bd537c Reviewed-on: https://chromium-review.googlesource.com/c/1397638 Commit-Queue: Gauthier Ambard <gambard@chromium.org> Reviewed-by: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#624140} [modify] https://crrev.com/88410c4f9836ef567f34652a448342dff6e404b4/ios/chrome/browser/ui/popup_menu/BUILD.gn [modify] https://crrev.com/88410c4f9836ef567f34652a448342dff6e404b4/ios/chrome/browser/ui/popup_menu/popup_menu_mediator_unittest.mm |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by eugene...@chromium.org
, Jan 4Status: Started (was: Untriaged)