Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2568063002:
[Password Manager] Fix icons on chrome://settings/passwords (Closed)
|
Created:
4 years ago by kolos1 Modified:
4 years ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, gcasto+watchlist_chromium.org, michaelpg+watch-options_chromium.org, arv+watch_chromium.org, vabr+watchlistpasswordmanager_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Password Manager] Fix icons on chrome://settings/passwords
Before this CL requests to the favicon service looked like the following:
chrome://favicon/size/16@1x/origin/https://twitter.com/signup
The "origin" parameter means that
a) the path should be removed
b) the "http" scheme should be added, if it is missed
(see details here
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/favicon_source.h).
The favicon service fetches icons from the history backend. If the path has
been removed from the URL, new URL might be absent in the history or even not
exist on the given site. Therefore, we might fail to fetch icon.
So, the removing the path doesn't make sense and the "origin/" parameter should
be removed. The bookmarks manager also doesn't use this parameter.
The parameter was added in vabr@'s CL
(https://codereview.chromium.org/10079024) which has a comment about "origin/".
The comment doesn't match to how the favicon service works.
BUG=672483
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/6178699a5a0ba0c505e336e7caf449a32467cba0
Cr-Commit-Position: refs/heads/master@{#438465}
Patch Set 1 #
Messages
Total messages: 20 (10 generated)
Description was changed from ========== [Password Manager] Fix icons on chrome://settings/passwords Before this CL requests to the favicon service looked like the following: chrome://favicon/size/16@1x/origin/https://twitter.com/signup The "origin" parameter means that a) the path should be removed b) the "http" scheme should be added, if it is missed (see details here https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/favicon_source.h). The favicon service fetches icons from the history backend. If the path has been removed from the URL, new URL might be absent in the history or even not exist on the given site. Therefore, we might fail to fetch icon. So, the removing the path doesn't make sense and the "origin/" parameter should be removed. The bookmarks manager also doesn't use this parameter. The parameter was added in this CL by vabr@ (https://codereview.chromium.org/589353003) which has a comment about "origin/". The comment doesn't match to how the favicon service works. BUG=672483 ========== to ========== [Password Manager] Fix icons on chrome://settings/passwords Before this CL requests to the favicon service looked like the following: chrome://favicon/size/16@1x/origin/https://twitter.com/signup The "origin" parameter means that a) the path should be removed b) the "http" scheme should be added, if it is missed (see details here https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/favicon_source.h). The favicon service fetches icons from the history backend. If the path has been removed from the URL, new URL might be absent in the history or even not exist on the given site. Therefore, we might fail to fetch icon. So, the removing the path doesn't make sense and the "origin/" parameter should be removed. The bookmarks manager also doesn't use this parameter. The parameter was added in this CL by vabr@ (https://codereview.chromium.org/589353003) which has a comment about "origin/". The comment doesn't match to how the favicon service works. BUG=672483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
kolos@chromium.org changed reviewers: + dbeam@chromium.org, pkotwicz@chromium.org, vabr@chromium.org
dbeam@: Please review this tiny CL. Also added vabr@ who added icons to the password page (https://codereview.chromium.org/589353003) and pkotwicz@ who works on favicons in chrome.
Description was changed from ========== [Password Manager] Fix icons on chrome://settings/passwords Before this CL requests to the favicon service looked like the following: chrome://favicon/size/16@1x/origin/https://twitter.com/signup The "origin" parameter means that a) the path should be removed b) the "http" scheme should be added, if it is missed (see details here https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/favicon_source.h). The favicon service fetches icons from the history backend. If the path has been removed from the URL, new URL might be absent in the history or even not exist on the given site. Therefore, we might fail to fetch icon. So, the removing the path doesn't make sense and the "origin/" parameter should be removed. The bookmarks manager also doesn't use this parameter. The parameter was added in this CL by vabr@ (https://codereview.chromium.org/589353003) which has a comment about "origin/". The comment doesn't match to how the favicon service works. BUG=672483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Password Manager] Fix icons on chrome://settings/passwords Before this CL requests to the favicon service looked like the following: chrome://favicon/size/16@1x/origin/https://twitter.com/signup The "origin" parameter means that a) the path should be removed b) the "http" scheme should be added, if it is missed (see details here https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/favicon_source.h). The favicon service fetches icons from the history backend. If the path has been removed from the URL, new URL might be absent in the history or even not exist on the given site. Therefore, we might fail to fetch icon. So, the removing the path doesn't make sense and the "origin/" parameter should be removed. The bookmarks manager also doesn't use this parameter. The parameter was added in vabr@'s CL (https://codereview.chromium.org/589353003) which has a comment about "origin/". The comment doesn't match to how the favicon service works. BUG=672483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
LGTM. Please correct the CL summary to point to https://codereview.chromium.org/10079024 as the CL which added the 'origin' parameter. The one cited now was just a revert of some other changes, which moved the code around. Cheers, Vaclav
Description was changed from ========== [Password Manager] Fix icons on chrome://settings/passwords Before this CL requests to the favicon service looked like the following: chrome://favicon/size/16@1x/origin/https://twitter.com/signup The "origin" parameter means that a) the path should be removed b) the "http" scheme should be added, if it is missed (see details here https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/favicon_source.h). The favicon service fetches icons from the history backend. If the path has been removed from the URL, new URL might be absent in the history or even not exist on the given site. Therefore, we might fail to fetch icon. So, the removing the path doesn't make sense and the "origin/" parameter should be removed. The bookmarks manager also doesn't use this parameter. The parameter was added in vabr@'s CL (https://codereview.chromium.org/589353003) which has a comment about "origin/". The comment doesn't match to how the favicon service works. BUG=672483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Password Manager] Fix icons on chrome://settings/passwords Before this CL requests to the favicon service looked like the following: chrome://favicon/size/16@1x/origin/https://twitter.com/signup The "origin" parameter means that a) the path should be removed b) the "http" scheme should be added, if it is missed (see details here https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/favicon_source.h). The favicon service fetches icons from the history backend. If the path has been removed from the URL, new URL might be absent in the history or even not exist on the given site. Therefore, we might fail to fetch icon. So, the removing the path doesn't make sense and the "origin/" parameter should be removed. The bookmarks manager also doesn't use this parameter. The parameter was added in vabr@'s CL (https://codereview.chromium.org/10079024) which has a comment about "origin/". The comment doesn't match to how the favicon service works. BUG=672483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [Password Manager] Fix icons on chrome://settings/passwords Before this CL requests to the favicon service looked like the following: chrome://favicon/size/16@1x/origin/https://twitter.com/signup The "origin" parameter means that a) the path should be removed b) the "http" scheme should be added, if it is missed (see details here https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/favicon_source.h). The favicon service fetches icons from the history backend. If the path has been removed from the URL, new URL might be absent in the history or even not exist on the given site. Therefore, we might fail to fetch icon. So, the removing the path doesn't make sense and the "origin/" parameter should be removed. The bookmarks manager also doesn't use this parameter. The parameter was added in vabr@'s CL (https://codereview.chromium.org/10079024) which has a comment about "origin/". The comment doesn't match to how the favicon service works. BUG=672483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Password Manager] Fix icons on chrome://settings/passwords Before this CL requests to the favicon service looked like the following: chrome://favicon/size/16@1x/origin/https://twitter.com/signup The "origin" parameter means that a) the path should be removed b) the "http" scheme should be added, if it is missed (see details here https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/favicon_source.h). The favicon service fetches icons from the history backend. If the path has been removed from the URL, new URL might be absent in the history or even not exist on the given site. Therefore, we might fail to fetch icon. So, the removing the path doesn't make sense and the "origin/" parameter should be removed. The bookmarks manager also doesn't use this parameter. The parameter was added in vabr@'s CL (https://codereview.chromium.org/10079024) which has a comment about "origin/". The comment doesn't match to how the favicon service works. BUG=672483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dbeam@: friendly ping. Please review this tiny CL.
dbeam@chromium.org changed reviewers: + dpapad@chromium.org
+dpapad is this right?
On 2016/12/13 at 18:57:43, dbeam wrote: > +dpapad is this right? I am not 100% sure, but based on my own favicon investigation at https://bugs.chromium.org/p/chromium/issues/detail?id=608069#c6, it sounds probably right. The history backend stores favicons as the user navigates around the Web, using the favicon exact URL as the "id" (in a SQLlite database). Querying for a favicon with anything other than the exact "id" fails to fetch the icon. IIUC, even with this CL, a default fallback favicon will be displayed if the user has never visited the corresponding webpage (https://twitter.com/signup in this example). @kolos does this match you are observing?
On 2016/12/13 19:35:27, dpapad wrote: > On 2016/12/13 at 18:57:43, dbeam wrote: > > +dpapad is this right? > > I am not 100% sure, but based on my own favicon investigation at > https://bugs.chromium.org/p/chromium/issues/detail?id=608069#c6, it sounds > probably right. The history backend stores favicons as the user navigates around > the Web, using the favicon exact URL as the "id" (in a SQLlite database). > Querying for a favicon with anything other than the exact "id" fails to fetch > the icon. > > IIUC, even with this CL, a default fallback favicon will be displayed if the > user has never visited the corresponding webpage (https://twitter.com/signup in > this example). @kolos does this match you are observing? Yes, the default favicon will be displayed, but we are not happy with that. It is why I need to fix that. This CL will show icons if the user has visited the URL. If an icon will be available even without a visit, it would be perfect. But this is out of scope of this CL. It is a question to the favicon service. Anyway, using "origin/" parameter doesn't make sense, because it removes the path of URLs. So that, some requests to the favicon service point to nonexistent URLs which of course hasn't been visited by the user. dpapad@, dbeam@: please let me your concerns, if any.
lgtm
The CQ bit was checked by kolos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1481702029917990, "parent_rev":
"7f63c7f52091f47f2399048a76db41053c85c98c", "commit_rev":
"45be5e9f18e516b983270395aab059af6af3602f"}
Message was sent while issue was closed.
Description was changed from ========== [Password Manager] Fix icons on chrome://settings/passwords Before this CL requests to the favicon service looked like the following: chrome://favicon/size/16@1x/origin/https://twitter.com/signup The "origin" parameter means that a) the path should be removed b) the "http" scheme should be added, if it is missed (see details here https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/favicon_source.h). The favicon service fetches icons from the history backend. If the path has been removed from the URL, new URL might be absent in the history or even not exist on the given site. Therefore, we might fail to fetch icon. So, the removing the path doesn't make sense and the "origin/" parameter should be removed. The bookmarks manager also doesn't use this parameter. The parameter was added in vabr@'s CL (https://codereview.chromium.org/10079024) which has a comment about "origin/". The comment doesn't match to how the favicon service works. BUG=672483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Password Manager] Fix icons on chrome://settings/passwords Before this CL requests to the favicon service looked like the following: chrome://favicon/size/16@1x/origin/https://twitter.com/signup The "origin" parameter means that a) the path should be removed b) the "http" scheme should be added, if it is missed (see details here https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/favicon_source.h). The favicon service fetches icons from the history backend. If the path has been removed from the URL, new URL might be absent in the history or even not exist on the given site. Therefore, we might fail to fetch icon. So, the removing the path doesn't make sense and the "origin/" parameter should be removed. The bookmarks manager also doesn't use this parameter. The parameter was added in vabr@'s CL (https://codereview.chromium.org/10079024) which has a comment about "origin/". The comment doesn't match to how the favicon service works. BUG=672483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2568063002 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [Password Manager] Fix icons on chrome://settings/passwords Before this CL requests to the favicon service looked like the following: chrome://favicon/size/16@1x/origin/https://twitter.com/signup The "origin" parameter means that a) the path should be removed b) the "http" scheme should be added, if it is missed (see details here https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/favicon_source.h). The favicon service fetches icons from the history backend. If the path has been removed from the URL, new URL might be absent in the history or even not exist on the given site. Therefore, we might fail to fetch icon. So, the removing the path doesn't make sense and the "origin/" parameter should be removed. The bookmarks manager also doesn't use this parameter. The parameter was added in vabr@'s CL (https://codereview.chromium.org/10079024) which has a comment about "origin/". The comment doesn't match to how the favicon service works. BUG=672483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2568063002 ========== to ========== [Password Manager] Fix icons on chrome://settings/passwords Before this CL requests to the favicon service looked like the following: chrome://favicon/size/16@1x/origin/https://twitter.com/signup The "origin" parameter means that a) the path should be removed b) the "http" scheme should be added, if it is missed (see details here https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/favicon_source.h). The favicon service fetches icons from the history backend. If the path has been removed from the URL, new URL might be absent in the history or even not exist on the given site. Therefore, we might fail to fetch icon. So, the removing the path doesn't make sense and the "origin/" parameter should be removed. The bookmarks manager also doesn't use this parameter. The parameter was added in vabr@'s CL (https://codereview.chromium.org/10079024) which has a comment about "origin/". The comment doesn't match to how the favicon service works. BUG=672483 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/6178699a5a0ba0c505e336e7caf449a32467cba0 Cr-Commit-Position: refs/heads/master@{#438465} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/6178699a5a0ba0c505e336e7caf449a32467cba0 Cr-Commit-Position: refs/heads/master@{#438465} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
