extensions: Refactor NavigateToURL to take disposition param [chromium/src : main]
0 views
Skip to first unread message
Zhengzheng Liu (Gerrit)
Sep 4, 2025, 7:16:59 PM (3 days ago) Sep 4
to James Cook, Achuith Bhandarkar, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from James Cook
Zhengzheng Liu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Zhengzheng Liu
| Commit-Queue | +1 |
1 comment
Patchset-level comments
File-level comment, Patchset 5 (Latest):
Zhengzheng Liu . resolved
Hi James, I added CHECK for the disposition support as well as very simple browsertests for the functionality. Could you please help take a look? Thanks
Related details
Attention is currently required from:
- James Cook
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
Sep 4, 2025, 7:21:03 PM (3 days ago) Sep 4
to Zhengzheng Liu, Achuith Bhandarkar, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Zhengzheng Liu
James Cook voted and added 4 comments![]( "Open in Gerrit")
Votes added by James Cook
| Code-Review | +1 |
4 comments
Patchset-level comments
James Cook . resolved
LGTM with nit
File chrome/browser/extensions/extension_tab_util.cc
Line 1273, Patchset 5 (Latest):
CHECK(disposition == WindowOpenDisposition::NEW_FOREGROUND_TAB);James Cook . unresolved
nit: use CHECK_EQ and you'll get a better error message if it trips
Line 1285, Patchset 5 (Latest):
#endifJames Cook . unresolved
Not for this CL, out of curiosity I wonder if content::WebContents::OpenURL() could be useful here? It seems to understand disposition and transition type.
File chrome/test/BUILD.gn
Line 9297, Patchset 5 (Latest):
"../browser/extensions/extension_tab_util_unittest.cc",James Cook . unresolved
Thanks for adding this!
Related details
Attention is currently required from:
- Zhengzheng Liu
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zhengzheng Liu (Gerrit)
Sep 4, 2025, 7:33:19 PM (3 days ago) Sep 4
to James Cook, Achuith Bhandarkar, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Zhengzheng Liu added 3 comments![]( "Open in Gerrit")
File chrome/browser/extensions/extension_tab_util.cc
Line 1273, Patchset 5:
CHECK(disposition == WindowOpenDisposition::NEW_FOREGROUND_TAB);James Cook . resolved
nit: use CHECK_EQ and you'll get a better error message if it trips
Zhengzheng Liu
Done
Not for this CL, out of curiosity I wonder if content::WebContents::OpenURL() could be useful here? It seems to understand disposition and transition type.
Zhengzheng Liu
Thanks for your suggestion. I will further investigate and wrap up a follow up CL if `content::WebContents::OpenURL()` works better here. ❤️
File chrome/test/BUILD.gn
Line 9297, Patchset 5:
"../browser/extensions/extension_tab_util_unittest.cc",James Cook . resolved
Zhengzheng LiuThanks for adding this!
😊
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zhengzheng Liu (Gerrit)
Sep 4, 2025, 7:33:22 PM (3 days ago) Sep 4
to James Cook, Achuith Bhandarkar, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Zhengzheng Liu voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zhengzheng Liu (Gerrit)
Sep 4, 2025, 8:11:29 PM (3 days ago) Sep 4
to James Cook, Achuith Bhandarkar, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Zhengzheng Liu added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 6 (Latest):
Zhengzheng Liu . resolved
The CQ failure is not related to this change. A revert to fix this failure has been submitted. https://chromium-review.googlesource.com/c/chromium/src/+/6917491
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zhengzheng Liu (Gerrit)
Sep 4, 2025, 8:11:42 PM (3 days ago) Sep 4
to James Cook, Achuith Bhandarkar, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Zhengzheng Liu voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Achuith Bhandarkar (Gerrit)
Sep 4, 2025, 8:37:58 PM (3 days ago) Sep 4
to Zhengzheng Liu, James Cook, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Zhengzheng Liu
Achuith Bhandarkar voted and added 2 comments![]( "Open in Gerrit")
Votes added by Achuith Bhandarkar
| Code-Review | -1 |
2 comments
File chrome/browser/extensions/extension_tab_util.cc
Line 1272, Patchset 6 (Latest):
// TODO(crbug.com//440173000): Support new window disposition for Android.Achuith Bhandarkar . unresolved
what do you mean by "new" here? Is this supposed to be "other"?
File chrome/browser/extensions/extension_tab_util_browsertest.cc
Line 148, Patchset 6 (Latest):
web_contents->GetBrowserContext(), GURL("http://www.google.com"));Achuith Bhandarkar . unresolved
Wonder if we should use an internal URL like chrome://version instead
Related details
Attention is currently required from:
- Zhengzheng Liu
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Achuith Bhandarkar (Gerrit)
Sep 4, 2025, 8:41:42 PM (3 days ago) Sep 4
to Zhengzheng Liu, James Cook, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Zhengzheng Liu
Achuith Bhandarkar voted Code-Review+0![]( "Open in Gerrit")
| Code-Review | +0 |
Related details
Attention is currently required from:
- Zhengzheng Liu
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zhengzheng Liu (Gerrit)
Sep 4, 2025, 9:16:32 PM (3 days ago) Sep 4
to Achuith Bhandarkar, James Cook, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Achuith Bhandarkar
Zhengzheng Liu added 3 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 8 (Latest):
Zhengzheng Liu . resolved
Hi Achuith, please help take another look. Thanks.
File chrome/browser/extensions/extension_tab_util.cc
Line 1272, Patchset 6:
// TODO(crbug.com//440173000): Support new window disposition for Android.Achuith Bhandarkar . resolved
what do you mean by "new" here? Is this supposed to be "other"?
Zhengzheng Liu
Done
File chrome/browser/extensions/extension_tab_util_browsertest.cc
Line 148, Patchset 6:
web_contents->GetBrowserContext(), GURL("http://www.google.com"));Achuith Bhandarkar . resolved
Wonder if we should use an internal URL like chrome://version instead
Zhengzheng Liu
Done. "www.google.com" is also used in other places. I will create a bug to clean this usage up?
Related details
Attention is currently required from:
- Achuith Bhandarkar
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zhengzheng Liu (Gerrit)
Sep 5, 2025, 2:17:54 PM (2 days ago) Sep 5
to Achuith Bhandarkar, James Cook, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Achuith Bhandarkar
Zhengzheng Liu added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 9 (Latest):
Zhengzheng Liu . resolved
FYI: The ClangTidy warning is related to b/443228626, which is not caused by this CL.
Related details
Attention is currently required from:
- Achuith Bhandarkar
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Achuith Bhandarkar (Gerrit)
Sep 5, 2025, 2:59:12 PM (2 days ago) Sep 5
to Zhengzheng Liu, James Cook, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Zhengzheng Liu
Achuith Bhandarkar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Achuith Bhandarkar
| Code-Review | +1 |
1 comment
File chrome/browser/extensions/extension_tab_util_browsertest.cc
Line 148, Patchset 6:
web_contents->GetBrowserContext(), GURL("http://www.google.com"));Achuith Bhandarkar . resolved
Zhengzheng LiuWonder if we should use an internal URL like chrome://version instead
Done. "www.google.com" is also used in other places. I will create a bug to clean this usage up?
Achuith Bhandarkar
Let's not bother with this. I thought we'd want to avoid making calls to external sites in these tests (in theory the call could fail).
Related details
Attention is currently required from:
- Zhengzheng Liu
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zhengzheng Liu (Gerrit)
Sep 5, 2025, 3:49:32 PM (2 days ago) Sep 5
to Achuith Bhandarkar, James Cook, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Zhengzheng Liu added 1 comment![]( "Open in Gerrit")
File chrome/browser/extensions/extension_tab_util_browsertest.cc
Line 148, Patchset 6:
web_contents->GetBrowserContext(), GURL("http://www.google.com"));Achuith Bhandarkar . resolved
Zhengzheng LiuWonder if we should use an internal URL like chrome://version instead
Achuith BhandarkarDone. "www.google.com" is also used in other places. I will create a bug to clean this usage up?
Let's not bother with this. I thought we'd want to avoid making calls to external sites in these tests (in theory the call could fail).
Zhengzheng Liu
OK.Thanks for the suggestions and reviews.
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zhengzheng Liu (Gerrit)
Sep 5, 2025, 3:49:38 PM (2 days ago) Sep 5
to Achuith Bhandarkar, James Cook, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Zhengzheng Liu voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Sep 5, 2025, 4:03:22 PM (2 days ago) Sep 5
to Zhengzheng Liu, Achuith Bhandarkar, James Cook, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
extensions: Refactor NavigateToURL to take disposition param
-- Add CHECK of the disposition param for NavigateToURL to make sure it
is supported on the specific platform.
-- Add simple browsertest for NavigateToURL method.
-- Enable extension_tab_util_unittest on desktop Android.
Bug: 440173000
Change-Id: If93303e0d781d6cf2e19072822578c8d60225fa9
Reviewed-by: Achuith Bhandarkar <ach...@chromium.org>
Reviewed-by: James Cook <jame...@chromium.org>
Commit-Queue: Zhengzheng Liu <zhz...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1511813}
Files:
- M chrome/browser/extensions/extension_tab_util.cc
- M chrome/browser/extensions/extension_tab_util_browsertest.cc
- M chrome/browser/extensions/extension_tab_util_unittest.cc
- M chrome/test/BUILD.gn
Change size: S
Delta: 4 files changed, 38 insertions(+), 1 deletion(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by James Cook, +1 by Achuith Bhandarkar
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages