Regression:The text on the page info bubble reads Bookmark manager is an extension page. |
||||
Issue descriptionChrome Version : 52.0.2740.0 (Official Build) OS Version : Ubuntu 14.04 What steps will reproduce the problem? (1)Launch chrome and hit 'ctrl+shift+o' to open bookmarks page. (2)Click on 'View site info' button in Omnibox and observe the bubble. What is the expected result? The text on the bubble should read 'You are reading a secure Google Chrome page'. What happens instead? The text on the bubble says, Bookmark manager is an extension page. This is a regression issue broken in M-52. Good Build:52.0.2718.0 Bad Build :52.0.2719.0 CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/44e1d5dc7a9bac1aa8d7ec01dce4ba8a2338d312..50666ea5e8701bbcb3eb25245632a0f7b6371661 Suspecting https://codereview.chromium.org/1917013002 from changelog. @meacer: please confirm whether this is working as intended or an issue.
,
May 18 2016
This is odd, looking at it now.
,
May 18 2016
This is because bookmark page is generated by the bookmark manager extension, so in a way it's working as intended. But I'll prepare a fix.
,
May 18 2016
There seems to be an existing bug on stable channel: Website settings dialog gets the extension URL on chrome://bookmarks, but it also treats extension pages as secure origins. Two bugs cancel each other and the user sees "you are viewing a secure Chrome page" string. This is because page info bubble uses GetVisibleEntry()->GetURL(). Should it use GetVirtualURL() instead?
,
May 26 2016
Navigation folks, ping for the question in comment #4 :)
,
May 26 2016
Sorry, I don't have enough context to answer-- it depends on how the URL is being used. Where is the code?
,
May 26 2016
Sorry about the lack of context. Code's here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/views/frame/web_app_left_header_view_ash.cc&l=97 For chrome://bookmarks page, nav_entry()->GetURL() returns the chrome-extension:// URL for the extension that implements the page, while nav_entry()->GetVirtualURL() returns chrome://bookmarks.
,
May 26 2016
Thanks. I've looked into how it's used (goes through quite a bit of plumbing!), and it does seem like all of that code cares about what's showing in the omnibox, which would indeed be GetVirtualURL(). (That's actually how WebContents::GetURL() is implemented as well.) I wanted to be sure that we weren't making other access control decisions based on it, but it seems safe to change. I might recommend renaming it from |url| to |virtual_url| to make it clear what is being passed, though.
,
May 27 2016
Thanks Charlie. Yes it seems to be only used for display but I'll double check in any case.
,
May 27 2016
,
Jun 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/903170ee962abea0e5b71fc3836ef5fad8c2ea27 commit 903170ee962abea0e5b71fc3836ef5fad8c2ea27 Author: meacer <meacer@chromium.org> Date: Wed Jun 01 01:46:25 2016 Use the virtual URL of the page in the Page Info Bubble BUG= 612710 Review-Url: https://codereview.chromium.org/1995813002 Cr-Commit-Position: refs/heads/master@{#397020} [modify] https://crrev.com/903170ee962abea0e5b71fc3836ef5fad8c2ea27/chrome/browser/chromeos/login/ui/simple_web_view_dialog.cc [modify] https://crrev.com/903170ee962abea0e5b71fc3836ef5fad8c2ea27/chrome/browser/chromeos/login/ui/simple_web_view_dialog.h [modify] https://crrev.com/903170ee962abea0e5b71fc3836ef5fad8c2ea27/chrome/browser/ui/browser_dialogs.h [modify] https://crrev.com/903170ee962abea0e5b71fc3836ef5fad8c2ea27/chrome/browser/ui/browser_window.h [modify] https://crrev.com/903170ee962abea0e5b71fc3836ef5fad8c2ea27/chrome/browser/ui/cocoa/browser_window_cocoa.h [modify] https://crrev.com/903170ee962abea0e5b71fc3836ef5fad8c2ea27/chrome/browser/ui/cocoa/browser_window_cocoa.mm [modify] https://crrev.com/903170ee962abea0e5b71fc3836ef5fad8c2ea27/chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm [modify] https://crrev.com/903170ee962abea0e5b71fc3836ef5fad8c2ea27/chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.h [modify] https://crrev.com/903170ee962abea0e5b71fc3836ef5fad8c2ea27/chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm [modify] https://crrev.com/903170ee962abea0e5b71fc3836ef5fad8c2ea27/chrome/browser/ui/views/browser_dialogs_views_mac.cc [modify] https://crrev.com/903170ee962abea0e5b71fc3836ef5fad8c2ea27/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/903170ee962abea0e5b71fc3836ef5fad8c2ea27/chrome/browser/ui/views/frame/browser_view.h [modify] https://crrev.com/903170ee962abea0e5b71fc3836ef5fad8c2ea27/chrome/browser/ui/views/frame/web_app_left_header_view_ash.cc [modify] https://crrev.com/903170ee962abea0e5b71fc3836ef5fad8c2ea27/chrome/browser/ui/views/location_bar/location_bar_view.h [modify] https://crrev.com/903170ee962abea0e5b71fc3836ef5fad8c2ea27/chrome/browser/ui/views/location_bar/location_icon_view.cc [modify] https://crrev.com/903170ee962abea0e5b71fc3836ef5fad8c2ea27/chrome/browser/ui/views/toolbar/toolbar_view.cc [modify] https://crrev.com/903170ee962abea0e5b71fc3836ef5fad8c2ea27/chrome/browser/ui/views/toolbar/toolbar_view.h [modify] https://crrev.com/903170ee962abea0e5b71fc3836ef5fad8c2ea27/chrome/test/base/test_browser_window.h
,
Jun 1 2016
|
||||
►
Sign in to add a comment |
||||
Comment 1 by kavvaru@chromium.org
, May 18 2016