FIXUP: URLs are not fixed when bookmarks are created or updated using the bookmark extension API [chromium/src : main]
YISI YU (Gerrit)
YISI YU voted and added 1 comment![]( "Open in Gerrit")
Votes added by YISI YU
| Auto-Submit | +1 |
1 comment
Hello James. Hello Tom. The change is ready, PTAL, thanks!
The root cause of [this issue](https://issues.chromium.org/issues/402056130) is that `url_formatter::FixupURL()` is not called to fix the URL when navigating to an `about://` URL from bookmark, which triggers a DCHECK assertion failure [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/browser_about_handler.cc;l=51). (The OmniBox does not have this issue because it uses `url_formatter::FixupURL()` to correct the URL [here](https://source.chromium.org/chromium/chromium/src/+/main:components/omnibox/browser/autocomplete_input.cc;l=273).)
There are two ways to resolving this issue: the first is to call `url_formatter::FixupURL()` when a bookmark navigates [before this line](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc;l=161?q=chrome%2Fbrowser%2Fui%2Fbookmarks%2Fbookmark_utils_desktop.cc&ss=chromium%2Fchromium%2Fsrc), and the second is to call `url_formatter::FixupURL()` when creating or updating URLs via the bookmarks extension API.
I chose to fix the issue by adjusting the bookmarks extension API. This is because `FixupURL()` is called when creating or updating bookmarks in the bookmark bar [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc;l=446;bpv=1;bpt=0?q=bookmark_editor_view.&ss=chromium%2Fchromium%2Fsrc), whereas the bookmarks extension API does not perform such correction. Aligning behavior with the bookmark bar is likely the best option (and can prevent potential issues caused by unexpected input).
Related details
- James Cook
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Lukaszewicz (Gerrit)
Tom Lukaszewicz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tom Lukaszewicz
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
- James Cook
- YISI YU
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
James Cook voted and added 4 comments![]( "Open in Gerrit")
Votes added by James Cook
| Code-Review | +1 |
4 comments
LGTM with nits
return url_formatter::FixupURL(url_string, std::string());nit: I would leave off the std::string() part, since there's a default for that argument.
optional: Consider just inlining calls to url_formatter::FixupURL() instead of adding this helper function.
extensions::api::bookmarks::BookmarkTreeNode::FromValue(create_result)nit: "extensions::" not needed
extensions::api::bookmarks::BookmarkTreeNode::FromValue(update_result)ditto
Related details
- Tom Lukaszewicz
- YISI YU
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YISI YU (Gerrit)
YISI YU voted and added 4 comments![]( "Open in Gerrit")
Votes added by YISI YU
| Auto-Submit | +1 |
| Commit-Queue | +1 |
4 comments
return url_formatter::FixupURL(url_string, std::string());nit: I would leave off the std::string() part, since there's a default for that argument.
optional: Consider just inlining calls to url_formatter::FixupURL() instead of adding this helper function.
Aha, it looks like this default parameter was just added yesterday. Done.
I made this helper function a separate one because I wrote a big comment explaining why we need to do it (and it has 2 references in this file). This probably makes the code a bit easier to read.
extensions::api::bookmarks::BookmarkTreeNode::FromValue(create_result)YISI YUnit: "extensions::" not needed
Done. I have removed all redundant `extension::`
extensions::api::bookmarks::BookmarkTreeNode::FromValue(update_result)YISI YUditto
Done
Related details
- James Cook
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
James Cook voted and added 1 comment![]( "Open in Gerrit")
Votes added by James Cook
| Code-Review | +1 |
1 comment
LGTM. Tom, I think you'll need to re-stamp this, it lost your +1.
Related details
- Tom Lukaszewicz
- YISI YU
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Lukaszewicz (Gerrit)
Tom Lukaszewicz voted Code-Review+1![]( "Open in Gerrit")
- YISI YU
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
James Cook voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- YISI YU
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
FIXUP: URLs are not fixed when bookmarks are created or updated using the bookmark extension API
`chrome.bookmarks` does not fix URLs during creation or updates. This
causes inconsistent behavior with the bookmark bar and also leads to
potential issues. Users can add URLs with the `about://` scheme to
bookmarks via the extension API, which triggers a `DCHECK` when the
bookmarks are opened. Use `url_formatter::FixupURL()` to correct the
URLs, e.g., converting `about://` to `chrome://`
- M chrome/browser/extensions/api/bookmarks/bookmarks_api.cc
- M chrome/browser/extensions/api/bookmarks/bookmarks_api_unittest.cc
Code-Review: +1 by James Cook, +1 by Tom Lukaszewicz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YISI YU (Gerrit)
YISI YU added 1 comment![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |