New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 919148 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jan 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug-Regression



Sign in to add a comment

"Site Information" tools menu item is enabled for NTP

Project Member Reported by eugene...@chromium.org, Jan 4

Issue description

App 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
 
Owner: eugene...@chromium.org
Status: Started (was: Untriaged)
I know how to fix this and will send a CL soon.
Labels: ReleaseBlock-Stable

Comment 3 Deleted

Project Member

Comment 4 by bugdroid1@chromium.org, 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

Cc: vbhatso...@chromium.org
Labels: -Pri-2 Merge-Request-72 Pri-1
Status: Fixed (was: Started)
Vinutha, could you please verify the fix with the next Canary build.
Project Member

Comment 6 by sheriffbot@chromium.org, Jan 7

Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
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
Status: Verified (was: Fixed)
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

Labels: -Hotlist-Merge-Review -Merge-Review-72 Merge-Approved-72
Approved.
Project Member

Comment 9 by sheriffbot@chromium.org, Jan 14

Cc: kariahda@chromium.org
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
Project Member

Comment 10 by bugdroid1@chromium.org, Jan 14

Labels: -merge-approved-72 merge-merged-3626
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

Labels: Merge-Merged-72-3626
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}

Comment 12 by vbarig...@chromium.org, 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.  
Project Member

Comment 13 by bugdroid1@chromium.org, 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