Move omnibox base classes that are no longer used on iOS to //chrome. [chromium/src : main]
Josiah Kiehl (Gerrit)
Josiah Kiehl voted and added 3 comments![]( "Open in Gerrit")
Votes added by Josiah Kiehl
| Commit-Queue | +1 |
3 comments
Bypass-Check-License: No new filesJosiah KiehlI've rarely seen this used. What was failing?
The presubmit check was upset that the license date was not 2025.
source_set("omnibox_base") {I'm unsure if this name follows naming conventions and if this is a reasonable location in the file to put this declaration.
constexpr bool kIsDesktop = !BUILDFLAG(IS_ANDROID);I'm not sure about this change, either.
Given we don't have any IOS code in //chrome, I figured we can get rid of all IS_IOS checks, however this variable name kinda suggests that we should still assert !IS_IOS even though it's not necessary.
Related details
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl added 3 comments![]( "Open in Gerrit")
Josiah KiehlBeyond the subject line, add a brief description to explain why.
Done
Move omnibox base classes that are no longer cross platform to //chromeJosiah Kiehl"used on iOS" ?
Done
#include <uiautomation.h>Josiah KiehlCan this remain where it was?
Already fixed. I didn't realize you'd get to the review before I added you to the attention set. :P
Josiah Kiehl (Gerrit)
Josiah Kiehl added 1 comment![]( "Open in Gerrit")
#include <uiautomation.h>Josiah KiehlCan this remain where it was?
Already fixed. I didn't realize you'd get to the review before I added you to the attention set. :P
Done
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
This also creates a new build target "//chrome/browser/ui:omnibox_base" to avoid circular dependencies for "//chrome/browser/ui"Wrap at 72 columns. Gerrit's CL description editor UI has a format button that usually does it right.
constexpr bool kIsDesktop = !BUILDFLAG(IS_ANDROID);I'm not sure about this change, either.
Lei ZhangGiven we don't have any IOS code in //chrome, I figured we can get rid of all IS_IOS checks, however this variable name kinda suggests that we should still assert !IS_IOS even though it's not necessary.
This is ok IMO if you get in the mindset that //chrome only contains Android as a mobile platform.
The other possibility is to change this to explicitly list out the known desktop platforms.
Related details
- Josiah Kiehl
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
Bypass-Check-License: No new filesJosiah KiehlI've rarely seen this used. What was failing?
The presubmit check was upset that the license date was not 2025.
I patched in this CL and ran the presubmit locally. I got several warnings, but not this one.
Josiah Kiehl (Gerrit)
Josiah Kiehl added 7 comments![]( "Open in Gerrit")
Bypass-Check-License: No new filesJosiah KiehlI've rarely seen this used. What was failing?
Lei ZhangThe presubmit check was upset that the license date was not 2025.
I patched in this CL and ran the presubmit locally. I got several warnings, but not this one.
It could be that the reason I triggered it was in an earlier version and now I don't need it. I'll remove it.
This also creates a new build target "//chrome/browser/ui:omnibox_base" to avoid circular dependencies for "//chrome/browser/ui"Wrap at 72 columns. Gerrit's CL description editor UI has a format button that usually does it right.
Oh that's neat, ty!
source_set("omnibox_base") {Josiah KiehlCan this go in chrome/browser/ui/views/omnibox/BUILD.gn instead? In //chrome, there is a glacially slow transition from 10K line BUILD.gn files to per-directory BUILD.gn files.
That is what I wanted to do, but there isn't such a file, so I hesitated to add it without knowing what the intent was. I'll create it for this purpose.
source_set("omnibox_base") {I'm unsure if this name follows naming conventions and if this is a reasonable location in the file to put this declaration.
Acknowledged
"//components/resources:components_scaled_resources_grit",Josiah KiehlMaybe for another CL, but are there resources that should move into //chrome as well?
They probably can. I tried to move as few files as I could at once while still being buildable. I agree this is for another CL.
constexpr bool kIsDesktop = !BUILDFLAG(IS_ANDROID);Lei ZhangI'm not sure about this change, either.
Given we don't have any IOS code in //chrome, I figured we can get rid of all IS_IOS checks, however this variable name kinda suggests that we should still assert !IS_IOS even though it's not necessary.
This is ok IMO if you get in the mindset that //chrome only contains Android as a mobile platform.
The other possibility is to change this to explicitly list out the known desktop platforms.
Acknowledged
"../browser/ui/views/omnibox/omnibox_controller_unittest.cc",Josiah Kiehl1) Can these go in chrome/browser/ui/views/omnibox/BUILD.gn.
2) Unit tests don't belong inside the "test_support" target.
Sure, that makes sense now that I've created that file. Ty!
Related details
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl added 1 comment![]( "Open in Gerrit")
Bypass-Check-License: No new filesJosiah KiehlI've rarely seen this used. What was failing?
Lei ZhangThe presubmit check was upset that the license date was not 2025.
Josiah KiehlI patched in this CL and ran the presubmit locally. I got several warnings, but not this one.
It could be that the reason I triggered it was in an earlier version and now I don't need it. I'll remove it.
- Justin Donnelly
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
Bypass-Check-License: No new filesJosiah KiehlI've rarely seen this used. What was failing?
Lei ZhangThe presubmit check was upset that the license date was not 2025.
Josiah KiehlI patched in this CL and ran the presubmit locally. I got several warnings, but not this one.
Josiah KiehlIt could be that the reason I triggered it was in an earlier version and now I don't need it. I'll remove it.
The error returned:
https://ci.chromium.org/ui/p/chromium/builders/try/chromium_presubmit/3504033/overview
Thanks for the reference. I guess we'll follow the presubmit's advice.
Related details
- Justin Donnelly
Josiah Kiehl (Gerrit)
Josiah Kiehl added 1 comment![]( "Open in Gerrit")
Bypass-Check-License: No new filesJosiah KiehlI've rarely seen this used. What was failing?
Lei ZhangThe presubmit check was upset that the license date was not 2025.
Josiah KiehlI patched in this CL and ran the presubmit locally. I got several warnings, but not this one.
Josiah KiehlIt could be that the reason I triggered it was in an earlier version and now I don't need it. I'll remove it.
Lei ZhangThe error returned:
https://ci.chromium.org/ui/p/chromium/builders/try/chromium_presubmit/3504033/overview
Thanks for the reference. I guess we'll follow the presubmit's advice.
- Justin Donnelly
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 7 comments![]( "Open in Gerrit")
}There's just a random newline deletion here. Revert and maybe separately get your text editor to keep the newline at the end of the file?
"//chrome/browser/ui/views/omnibox:base",Curious if this needs to be in `public_deps`, or if it can go in `deps` below. Either way, please use GN's auto-formatter to keep the list sorted.
"//chrome/browser/ui/views/omnibox:base",Same here and elsewhere. Keep the list sorted. If you don't, everyone else who touches this file in the future gets a complain about this in their presubmits.
"omnibox_controller.h",Split the headers into a `public` list.
visibility = [https://gn.googlesource.com/gn/+/main/docs/style_guide.md#ordering-within-a-target prefers tho have this first, but I also wonder if it's necessary.
"../browser/ui/views/omnibox/omnibox_controller_unittest.cc",Josiah Kiehl1) Can these go in chrome/browser/ui/views/omnibox/BUILD.gn.
2) Unit tests don't belong inside the "test_support" target.
Sure, that makes sense now that I've created that file. Ty!
Please move things over. Right now, some of these files have entries in 2 places.
Related details
- Josiah Kiehl
- Justin Donnelly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
visibility = [https://gn.googlesource.com/gn/+/main/docs/style_guide.md#ordering-within-a-target prefers tho have this first, but I also wonder if it's necessary.
"to have this first"
Josiah Kiehl (Gerrit)
Josiah Kiehl added 8 comments![]( "Open in Gerrit")
There's just a random newline deletion here. Revert and maybe separately get your text editor to keep the newline at the end of the file?
Done
Curious if this needs to be in `public_deps`, or if it can go in `deps` below. Either way, please use GN's auto-formatter to keep the list sorted.
It can. I added more direct dependencies down the tree to allow me to move it.
Same here and elsewhere. Keep the list sorted. If you don't, everyone else who touches this file in the future gets a complain about this in their presubmits.
Done
"lens_search_feature_flag_utils_browsertest.cc",This was residual from the format run... I left it in, thinking it's benign, but if I should revert lmk.
Josiah KiehlFile needs copyright boilerplate.
Done
Split the headers into a `public` list.
Done
Lei Zhanghttps://gn.googlesource.com/gn/+/main/docs/style_guide.md#ordering-within-a-target prefers tho have this first, but I also wonder if it's necessary.
"to have this first"
It seems like visibility first is significantly more common than last, so I moved it.
"../browser/ui/views/omnibox/omnibox_controller_unittest.cc",Josiah Kiehl1) Can these go in chrome/browser/ui/views/omnibox/BUILD.gn.
2) Unit tests don't belong inside the "test_support" target.
Lei ZhangSure, that makes sense now that I've created that file. Ty!
Please move things over. Right now, some of these files have entries in 2 places.
Neglected to save the file... oops. I suppose I can also move the test_* classes, too, tho, so I'll do that too.
Related details
- Justin Donnelly
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 5 comments![]( "Open in Gerrit")
Saw some red bots. Please make sure `gn check out/foo` passes.
"//chrome/browser/ui/views/omnibox:base",Actually should be below line 4447. Yes, the list is already slightly out of order, sigh.
"lens_search_feature_flag_utils_browsertest.cc",This was residual from the format run... I left it in, thinking it's benign, but if I should revert lmk.
It's better to have this sorted, so please leave this here. This is exactly what I was referring to. Someone else somehow bypasses presubmit, left the list out of order, and now it becomes your problem.
"//chrome/browser/ui/views/omnibox:test_support",Can this be in `deps`? If it needs to be in `public_deps`, can you mention why in the reply CL comment?
"//chrome/browser/ui/views/omnibox:test_support",Ditto re: public_deps vs. deps.
Related details
- Josiah Kiehl
- Justin Donnelly
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl voted and added 1 comment![]( "Open in Gerrit")
Votes added by Josiah Kiehl
| Commit-Queue | +1 |
1 comment
"//chrome/browser/ui/views/omnibox:base",Actually should be below line 4447. Yes, the list is already slightly out of order, sigh.
Ah, it seems the autoformatter tries to respect the empty line as a logical break.. and if you remove the empty line, the formatter sees the comment and adds it line back. This means all the empty lines before comments need to be manually deleted to get the formatter to work as expected.
I moved my new entry for now and left the others unsorted.
Related details
- Justin Donnelly
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl added 2 comments![]( "Open in Gerrit")
"//chrome/browser/ui/views/omnibox:test_support",Can this be in `deps`? If it needs to be in `public_deps`, can you mention why in the reply CL comment?
Moved!
"//chrome/browser/ui/views/omnibox:test_support",Ditto re: public_deps vs. deps.
Moved!
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
"//chrome/browser/ui/views/omnibox:base",Actually should be below line 4447. Yes, the list is already slightly out of order, sigh.
Ah, it seems the autoformatter tries to respect the empty line as a logical break.. and if you remove the empty line, the formatter sees the comment and adds it line back. This means all the empty lines before comments need to be manually deleted to get the formatter to work as expected.
I moved my new entry for now and left the others unsorted.
Ya, ok, no worries. I can do a follow-up and fix this. This CL is large enough as-is.
"//chrome/browser/ui/views/omnibox:base",Looking at the adjacent entries, I suspect this should be omnibox:unit_tests. I suspect in the current patch set, the tests in omnibox:unit_tests aren't being compiled.
Related details
- Josiah Kiehl
- Justin Donnelly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl added 1 comment![]( "Open in Gerrit")
"//chrome/browser/ui/views/omnibox:base",Josiah KiehlLooking at the adjacent entries, I suspect this should be omnibox:unit_tests. I suspect in the current patch set, the tests in omnibox:unit_tests aren't being compiled.
Fixed.......
Related details
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
public_deps = []This doesn't smell right. The android_browsertests target has existed for some time without needing `public_deps`, so why does it need it now? Who are the dependents of android_browsertests that need the exported `public_deps`?
Related details
- Josiah Kiehl
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl added 1 comment![]( "Open in Gerrit")
public_deps = []This doesn't smell right. The android_browsertests target has existed for some time without needing `public_deps`, so why does it need it now? Who are the dependents of android_browsertests that need the exported `public_deps`?
I agree... I originally had it in deps, but I got this error message:
https://ci.chromium.org/ui/p/chromium/builders/try/android-desktop-x64-rel/248217/overview
When I moved it to public_deps, it resolved. I don't fully understand why this is, tho, so if you have insight, I'd appreciate it.
Related details
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
public_deps = []Josiah KiehlThis doesn't smell right. The android_browsertests target has existed for some time without needing `public_deps`, so why does it need it now? Who are the dependents of android_browsertests that need the exported `public_deps`?
I agree... I originally had it in deps, but I got this error message:
https://ci.chromium.org/ui/p/chromium/builders/try/android-desktop-x64-rel/248217/overviewWhen I moved it to public_deps, it resolved. I don't fully understand why this is, tho, so if you have insight, I'd appreciate it.
```
//chrome/test:android_browsertests__library -->
//chrome/test:platform_browser_tests --[private]-->
//chrome/browser/preloading/prefetch:platform_browser_tests --[private]-->
//chrome/browser/ui/views/omnibox:base
```
means you want a deps entry from `//chrome/test:android_browsertests__library` (maybe that's what android_browsertests expands out to?) to `//chrome/browser/ui/views/omnibox:base`.
Related details
- Josiah Kiehl
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl added 1 comment![]( "Open in Gerrit")
public_deps = []Josiah KiehlThis doesn't smell right. The android_browsertests target has existed for some time without needing `public_deps`, so why does it need it now? Who are the dependents of android_browsertests that need the exported `public_deps`?
Lei ZhangI agree... I originally had it in deps, but I got this error message:
https://ci.chromium.org/ui/p/chromium/builders/try/android-desktop-x64-rel/248217/overviewWhen I moved it to public_deps, it resolved. I don't fully understand why this is, tho, so if you have insight, I'd appreciate it.
```
//chrome/test:android_browsertests__library -->
//chrome/test:platform_browser_tests --[private]-->
//chrome/browser/preloading/prefetch:platform_browser_tests --[private]-->
//chrome/browser/ui/views/omnibox:base
```means you want a deps entry from `//chrome/test:android_browsertests__library` (maybe that's what android_browsertests expands out to?) to `//chrome/browser/ui/views/omnibox:base`.
This bit is in `test("android_browsertests") {`. I suppose `test("android_browsertests")` might include a `android_browsertests__library`.
There isn't another location for `android_browsertests` other than this one (which apparently would need to use public_deps), but it looks like adding `...omnibox:base` to `platform_browser_tests` is the trick.
Related details
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl voted and added 2 comments![]( "Open in Gerrit")
Votes added by Josiah Kiehl
| Commit-Queue | +1 |
2 comments
public_deps = []Josiah KiehlThis doesn't smell right. The android_browsertests target has existed for some time without needing `public_deps`, so why does it need it now? Who are the dependents of android_browsertests that need the exported `public_deps`?
Lei ZhangI agree... I originally had it in deps, but I got this error message:
https://ci.chromium.org/ui/p/chromium/builders/try/android-desktop-x64-rel/248217/overviewWhen I moved it to public_deps, it resolved. I don't fully understand why this is, tho, so if you have insight, I'd appreciate it.
Josiah Kiehl```
//chrome/test:android_browsertests__library -->
//chrome/test:platform_browser_tests --[private]-->
//chrome/browser/preloading/prefetch:platform_browser_tests --[private]-->
//chrome/browser/ui/views/omnibox:base
```means you want a deps entry from `//chrome/test:android_browsertests__library` (maybe that's what android_browsertests expands out to?) to `//chrome/browser/ui/views/omnibox:base`.
This bit is in `test("android_browsertests") {`. I suppose `test("android_browsertests")` might include a `android_browsertests__library`.
There isn't another location for `android_browsertests` other than this one (which apparently would need to use public_deps), but it looks like adding `...omnibox:base` to `platform_browser_tests` is the trick.
What I don't know is if there's a reasonable way to capture all the generated targets from `test("...")`... each one needs to be added to `visibility` for this to work
Related details
- Josiah Kiehl
- Lei Zhang
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
public_deps = []Josiah KiehlThis doesn't smell right. The android_browsertests target has existed for some time without needing `public_deps`, so why does it need it now? Who are the dependents of android_browsertests that need the exported `public_deps`?
Lei ZhangI agree... I originally had it in deps, but I got this error message:
https://ci.chromium.org/ui/p/chromium/builders/try/android-desktop-x64-rel/248217/overviewWhen I moved it to public_deps, it resolved. I don't fully understand why this is, tho, so if you have insight, I'd appreciate it.
Josiah Kiehl```
//chrome/test:android_browsertests__library -->
//chrome/test:platform_browser_tests --[private]-->
//chrome/browser/preloading/prefetch:platform_browser_tests --[private]-->
//chrome/browser/ui/views/omnibox:base
```means you want a deps entry from `//chrome/test:android_browsertests__library` (maybe that's what android_browsertests expands out to?) to `//chrome/browser/ui/views/omnibox:base`.
Josiah KiehlThis bit is in `test("android_browsertests") {`. I suppose `test("android_browsertests")` might include a `android_browsertests__library`.
There isn't another location for `android_browsertests` other than this one (which apparently would need to use public_deps), but it looks like adding `...omnibox:base` to `platform_browser_tests` is the trick.
What I don't know is if there's a reasonable way to capture all the generated targets from `test("...")`... each one needs to be added to `visibility` for this to work
Maybe just don't bother with `visibility`? i.e. Omitting it means anyone can depend on it.
Related details
- Josiah Kiehl
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl added 1 comment![]( "Open in Gerrit")
public_deps = []Josiah KiehlThis doesn't smell right. The android_browsertests target has existed for some time without needing `public_deps`, so why does it need it now? Who are the dependents of android_browsertests that need the exported `public_deps`?
Lei ZhangI agree... I originally had it in deps, but I got this error message:
https://ci.chromium.org/ui/p/chromium/builders/try/android-desktop-x64-rel/248217/overviewWhen I moved it to public_deps, it resolved. I don't fully understand why this is, tho, so if you have insight, I'd appreciate it.
Josiah Kiehl```
//chrome/test:android_browsertests__library -->
//chrome/test:platform_browser_tests --[private]-->
//chrome/browser/preloading/prefetch:platform_browser_tests --[private]-->
//chrome/browser/ui/views/omnibox:base
```means you want a deps entry from `//chrome/test:android_browsertests__library` (maybe that's what android_browsertests expands out to?) to `//chrome/browser/ui/views/omnibox:base`.
Josiah KiehlThis bit is in `test("android_browsertests") {`. I suppose `test("android_browsertests")` might include a `android_browsertests__library`.
There isn't another location for `android_browsertests` other than this one (which apparently would need to use public_deps), but it looks like adding `...omnibox:base` to `platform_browser_tests` is the trick.
Lei ZhangWhat I don't know is if there's a reasonable way to capture all the generated targets from `test("...")`... each one needs to be added to `visibility` for this to work
Maybe just don't bother with `visibility`? i.e. Omitting it means anyone can depend on it.
That works. I suppose without a build cleaner of sorts, it's rough to manually maintain dependency visibility on widely used targets.
Related details
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang voted Code-Review+1![]( "Open in Gerrit")
- Josiah Kiehl
- Justin Donnelly
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
"//chrome/browser/ui/views/omnibox:base",Just one more thing - this line is in an `!is_android` section. The same happens in chrome/browser/ui/omnibox/BUILD.gn and chrome/browser/ui/webui/new_tab_page/composebox/BUILD.gn. This makes me wonder if this target is ever used on Android. I asked about it on the bug but nobody answered.
Josiah Kiehl (Gerrit)
Josiah Kiehl added 1 comment![]( "Open in Gerrit")
"//chrome/browser/ui/views/omnibox:base",Just one more thing - this line is in an `!is_android` section. The same happens in chrome/browser/ui/omnibox/BUILD.gn and chrome/browser/ui/webui/new_tab_page/composebox/BUILD.gn. This makes me wonder if this target is ever used on Android. I asked about it on the bug but nobody answered.
I was thinking this was a views-specific class and so wouldn't be used in Android, but that might not be the case. The android tests do seem to be passing, at least.
Related details
- Justin Donnelly
Josiah Kiehl (Gerrit)
Josiah Kiehl added 1 comment![]( "Open in Gerrit")
"//chrome/browser/ui/views/omnibox:base",Josiah KiehlJust one more thing - this line is in an `!is_android` section. The same happens in chrome/browser/ui/omnibox/BUILD.gn and chrome/browser/ui/webui/new_tab_page/composebox/BUILD.gn. This makes me wonder if this target is ever used on Android. I asked about it on the bug but nobody answered.
I was thinking this was a views-specific class and so wouldn't be used in Android, but that might not be the case. The android tests do seem to be passing, at least.
Turns out none of the above are actually views-specific code... Justin pointed out that we'd be better off moving the code to chrome/browser/ui/omnibox instead of chrome/browser/ui/views/omnibox as at least OmniboxEditModel is used in the webui implementation.
Related details
- Justin Donnelly
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
"//chrome/browser/ui/views/omnibox:base",Josiah KiehlJust one more thing - this line is in an `!is_android` section. The same happens in chrome/browser/ui/omnibox/BUILD.gn and chrome/browser/ui/webui/new_tab_page/composebox/BUILD.gn. This makes me wonder if this target is ever used on Android. I asked about it on the bug but nobody answered.
Josiah KiehlI was thinking this was a views-specific class and so wouldn't be used in Android, but that might not be the case. The android tests do seem to be passing, at least.
Turns out none of the above are actually views-specific code... Justin pointed out that we'd be better off moving the code to chrome/browser/ui/omnibox instead of chrome/browser/ui/views/omnibox as at least OmniboxEditModel is used in the webui implementation.
OK. This is why I was hoping Justin could have replied earlier with some guidance.
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
#include "chrome/browser/ui/omnibox/omnibox_controller.h"Need to auto-format and sort the list in all files.
Related details
- Josiah Kiehl
- Justin Donnelly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
"//chrome/browser/ui/omnibox:base",This is now adjacent to the TODO on line 4399, which is not what we want. Move this up to line ~4390.
source_set("base") {There already exists "omnibox" and "impl" targets here. The question for Omnibox OWNERS is whether they want a new "base" target, or if the "base" target should be folded into the existing targets.
Justin Donnelly (Gerrit)
Justin Donnelly added 5 comments![]( "Open in Gerrit")
Looks good, thanks, there's just some includes and BUILD file entries that are out of alphabetical order and need to be fixed. I pointed out some cases in the first few files but please audit the rest and make sure they're all corrected.
Note that Cider-G's format functions (format file, format modified lines), or "% git cl format" on the command line will fix these automatically, to save some effort and to ensure that they're all correct.
"//chrome/browser/ui/omnibox:base",Move to line 4388, both to alphabetize and to keep the TODO above associated with the correct entry ("//chrome/browser/ui/search:impl").
#include "chrome/browser/ui/omnibox/omnibox_controller.h"Move these 3 lines up 1 position to maintain alphabetical order.
#include "chrome/browser/ui/omnibox/omnibox_controller.h"Move up 1 line.
#include "chrome/browser/ui/omnibox/omnibox_view.h"Move to line 30.
Related details
- Josiah Kiehl
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl voted and added 7 comments![]( "Open in Gerrit")
Votes added by Josiah Kiehl
| Commit-Queue | +1 |
Looks good, thanks, there's just some includes and BUILD file entries that are out of alphabetical order and need to be fixed. I pointed out some cases in the first few files but please audit the rest and make sure they're all corrected.
Note that Cider-G's format functions (format file, format modified lines), or "% git cl format" on the command line will fix these automatically, to save some effort and to ensure that they're all correct.
I stopped using the autoformatter along the way because it adds unrelated formatting changes cause this CL touches so many files... I ran it again for this purpose, though, cause it's easier to fix the spurious changes for the 10th time than to do all this reformatting manually. ðŸ˜
This is now adjacent to the TODO on line 4399, which is not what we want. Move this up to line ~4390.
Done
Move to line 4388, both to alphabetize and to keep the TODO above associated with the correct entry ("//chrome/browser/ui/search:impl").
It seems the autoformatter gets lost trying to maintain comment separated sections.
Need to auto-format and sort the list in all files.
Done
#include "chrome/browser/ui/omnibox/omnibox_controller.h"Move these 3 lines up 1 position to maintain alphabetical order.
Done
#include "chrome/browser/ui/omnibox/omnibox_controller.h"Josiah KiehlMove up 1 line.
Done
#include "chrome/browser/ui/omnibox/omnibox_view.h"Josiah KiehlMove to line 30.
Done
Related details
- Josiah Kiehl
- Justin Donnelly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Donnelly (Gerrit)
Justin Donnelly voted and added 4 comments![]( "Open in Gerrit")
Votes added by Justin Donnelly
| Code-Review | +1 |
4 comments
Latest patchest (63) lgtm with just a couple minor suggestions.
There already exists "omnibox" and "impl" targets here. The question for Omnibox OWNERS is whether they want a new "base" target, or if the "base" target should be folded into the existing targets.
I think rolling this into the "omnibox" target probably makes the most sense here.
#include "chrome/browser/ui/views/omnibox/omnibox_popup_view_views.h"These 2 popup includes look unintentional. Remove?
"//components/open_from_clipboard:test_support",Did you mean to add this line?
Related details
- Josiah Kiehl
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 3 comments![]( "Open in Gerrit")
"//chrome/browser/ui/omnibox:{base,test_support}"Update
#import "chrome/browser/global_keyboard_shortcuts_mac.h"Put this back.
"omnibox_controller.cc",
"omnibox_edit_model.cc",
"omnibox_popup_view.cc",
"omnibox_view.cc",Can these go into the "impl" target, along with the `deps` entries below?
Related details
- Josiah Kiehl
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl added 5 comments![]( "Open in Gerrit")
"//chrome/browser/ui/omnibox:{base,test_support}"Josiah KiehlUpdate
Done
#import "chrome/browser/global_keyboard_shortcuts_mac.h"Put this back.
Do we have opinions on turning off clang formatter here? I added the flags here and one other spot cause the autoformatter keeps moving these lines.
"omnibox_controller.cc",
"omnibox_edit_model.cc",
"omnibox_popup_view.cc",
"omnibox_view.cc",Can these go into the "impl" target, along with the `deps` entries below?
Oh, is that how it's intended to be laid out?
Perhaps splitting out the `impl` target predated the ability to split `public` and `sources` list in the same target? It's not clear what advantage a separate `impl` target has over a single target. This cl is not the place to make changes to that layout, but I am curious what the reasoning is.
#include "chrome/browser/ui/views/omnibox/omnibox_popup_view_views.h"These 2 popup includes look unintentional. Remove?
This isn't a typical typo, so I'm puzzled how this change got in. I briefly had the gemini plugin turned on in vs code before disabling it out of annoyance... perhaps I accidentally inserted a suggestion without noticing.
"//components/open_from_clipboard:test_support",Did you mean to add this line?
I suppose not. It compiles fine without it. Perhaps a bad conflict resolution, ty for noticing.
Related details
- Justin Donnelly
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Donnelly (Gerrit)
Justin Donnelly added 1 comment![]( "Open in Gerrit")
#import "chrome/browser/global_keyboard_shortcuts_mac.h"Josiah KiehlPut this back.
Do we have opinions on turning off clang formatter here? I added the flags here and one other spot cause the autoformatter keeps moving these lines.
At first I thought this change was fine but I didn't notice that the line the formatter is moving here is the header for this implementation file. So I think Lei's right that it should be moved back. If you'd rather not fight the formatter, I think there's a directive you can add to disable it for specific lines.
Related details
- Josiah Kiehl
- Lei Zhang
Lei Zhang (Gerrit)
Lei Zhang added 5 comments![]( "Open in Gerrit")
#import "chrome/browser/global_keyboard_shortcuts_mac.h"Josiah KiehlPut this back.
Do we have opinions on turning off clang formatter here? I added the flags here and one other spot cause the autoformatter keeps moving these lines.
I'm not sure, especially when it comes to .mm files. In any case, I would suggest:
1) Putting it back where it was. It's in the wrong spot.
2) Don't bother with the `// clang-format` comments. I think this CL is mostly formatted correctly at this point, so the tug-o-war with clang-format is over.
"omnibox_controller.cc",
"omnibox_edit_model.cc",
"omnibox_popup_view.cc",
"omnibox_view.cc",Josiah KiehlCan these go into the "impl" target, along with the `deps` entries below?
Oh, is that how it's intended to be laid out?
Perhaps splitting out the `impl` target predated the ability to split `public` and `sources` list in the same target? It's not clear what advantage a separate `impl` target has over a single target. This cl is not the place to make changes to that layout, but I am curious what the reasoning is.
It's in chrome_browser_design_principles.md.
"//build:branding_buildflags",Already on line 62.
"//components/strings:components_strings_grit",Ditto.
Related details
- Josiah Kiehl
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl added 5 comments![]( "Open in Gerrit")
#import "chrome/browser/global_keyboard_shortcuts_mac.h"Josiah KiehlPut this back.
Lei ZhangDo we have opinions on turning off clang formatter here? I added the flags here and one other spot cause the autoformatter keeps moving these lines.
I'm not sure, especially when it comes to .mm files. In any case, I would suggest:
1) Putting it back where it was. It's in the wrong spot.
2) Don't bother with the `// clang-format` comments. I think this CL is mostly formatted correctly at this point, so the tug-o-war with clang-format is over.
If I remove the // clang-format comments, the formatter will put the import back in the middle of the includes on subsequent runs, perhaps because it's confused by this not being `global_keyboard_shortcuts_mac.cc`.
"omnibox_controller.cc",
"omnibox_edit_model.cc",
"omnibox_popup_view.cc",
"omnibox_view.cc",Josiah KiehlCan these go into the "impl" target, along with the `deps` entries below?
Lei ZhangOh, is that how it's intended to be laid out?
Perhaps splitting out the `impl` target predated the ability to split `public` and `sources` list in the same target? It's not clear what advantage a separate `impl` target has over a single target. This cl is not the place to make changes to that layout, but I am curious what the reasoning is.
It's in chrome_browser_design_principles.md.
Acknowledged
"//build:branding_buildflags",Already on line 62.
I wonder if there's a way to get gn to warn/error on duplicate dependencies to catch these things. I had been depending on the autoformatter to dedup lists, but that doesn't work across lists split like this.
"//components/search",Josiah KiehlDitto.
Done
"//components/strings:components_strings_grit",Josiah KiehlDitto.
Done
- Justin Donnelly
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
#import "chrome/browser/global_keyboard_shortcuts_mac.h"Josiah KiehlPut this back.
Lei ZhangDo we have opinions on turning off clang formatter here? I added the flags here and one other spot cause the autoformatter keeps moving these lines.
Josiah KiehlI'm not sure, especially when it comes to .mm files. In any case, I would suggest:
1) Putting it back where it was. It's in the wrong spot.
2) Don't bother with the `// clang-format` comments. I think this CL is mostly formatted correctly at this point, so the tug-o-war with clang-format is over.
If I remove the // clang-format comments, the formatter will put the import back in the middle of the includes on subsequent runs, perhaps because it's confused by this not being `global_keyboard_shortcuts_mac.cc`.
I understand the auto-formatter will do that. Just don't run the auto-formatter again, now that most of the CL is auto-formatted already?
"//build:branding_buildflags",Josiah KiehlAlready on line 62.
I wonder if there's a way to get gn to warn/error on duplicate dependencies to catch these things. I had been depending on the autoformatter to dedup lists, but that doesn't work across lists split like this.
GN is just another software project, so it's certainly possible. e.g. I taught the GN formatter to change `//path/to/target:target` to `//path/to/target` earlier this year.
Related details
- Josiah Kiehl
- Justin Donnelly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl added 2 comments![]( "Open in Gerrit")
#import "chrome/browser/global_keyboard_shortcuts_mac.h"Josiah KiehlPut this back.
Lei ZhangDo we have opinions on turning off clang formatter here? I added the flags here and one other spot cause the autoformatter keeps moving these lines.
Josiah KiehlI'm not sure, especially when it comes to .mm files. In any case, I would suggest:
1) Putting it back where it was. It's in the wrong spot.
2) Don't bother with the `// clang-format` comments. I think this CL is mostly formatted correctly at this point, so the tug-o-war with clang-format is over.
Lei ZhangIf I remove the // clang-format comments, the formatter will put the import back in the middle of the includes on subsequent runs, perhaps because it's confused by this not being `global_keyboard_shortcuts_mac.cc`.
I understand the auto-formatter will do that. Just don't run the auto-formatter again, now that most of the CL is auto-formatted already?
Oh, sure... I was more thinking for future CLs that touch that file... everyone is going to have to remember to put this line back.
"//build:branding_buildflags",Josiah KiehlAlready on line 62.
Lei ZhangI wonder if there's a way to get gn to warn/error on duplicate dependencies to catch these things. I had been depending on the autoformatter to dedup lists, but that doesn't work across lists split like this.
GN is just another software project, so it's certainly possible. e.g. I taught the GN formatter to change `//path/to/target:target` to `//path/to/target` earlier this year.
Sure, I was hoping maybe the feature already existed. I added a ticket in case anyone gets inspired and has a minute to find a solution: http://b/446914973
Related details
- Justin Donnelly
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
#import "chrome/browser/global_keyboard_shortcuts_mac.h"Josiah KiehlPut this back.
Lei ZhangDo we have opinions on turning off clang formatter here? I added the flags here and one other spot cause the autoformatter keeps moving these lines.
Josiah KiehlI'm not sure, especially when it comes to .mm files. In any case, I would suggest:
1) Putting it back where it was. It's in the wrong spot.
2) Don't bother with the `// clang-format` comments. I think this CL is mostly formatted correctly at this point, so the tug-o-war with clang-format is over.
Lei ZhangIf I remove the // clang-format comments, the formatter will put the import back in the middle of the includes on subsequent runs, perhaps because it's confused by this not being `global_keyboard_shortcuts_mac.cc`.
Josiah KiehlI understand the auto-formatter will do that. Just don't run the auto-formatter again, now that most of the CL is auto-formatted already?
Oh, sure... I was more thinking for future CLs that touch that file... everyone is going to have to remember to put this line back.
Let's file a bug report for that in the LLVM component and get on with this CL.
// clang-format offSimilarly, can we use `// clang-format off` sparingly and not use it in every place where there's a human / auto-formatter disagreement?
Related details
- Josiah Kiehl
- Justin Donnelly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl added 2 comments![]( "Open in Gerrit")
#import "chrome/browser/global_keyboard_shortcuts_mac.h"Josiah KiehlPut this back.
Lei ZhangDo we have opinions on turning off clang formatter here? I added the flags here and one other spot cause the autoformatter keeps moving these lines.
Josiah KiehlI'm not sure, especially when it comes to .mm files. In any case, I would suggest:
1) Putting it back where it was. It's in the wrong spot.
2) Don't bother with the `// clang-format` comments. I think this CL is mostly formatted correctly at this point, so the tug-o-war with clang-format is over.
Lei ZhangIf I remove the // clang-format comments, the formatter will put the import back in the middle of the includes on subsequent runs, perhaps because it's confused by this not being `global_keyboard_shortcuts_mac.cc`.
Josiah KiehlI understand the auto-formatter will do that. Just don't run the auto-formatter again, now that most of the CL is auto-formatted already?
Lei ZhangOh, sure... I was more thinking for future CLs that touch that file... everyone is going to have to remember to put this line back.
Let's file a bug report for that in the LLVM component and get on with this CL.
Done
// clang-format offSimilarly, can we use `// clang-format off` sparingly and not use it in every place where there's a human / auto-formatter disagreement?
- Justin Donnelly
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
#import "chrome/browser/global_keyboard_shortcuts_mac.h"This needs to be above line 5. Please diff against the original.
Related details
- Josiah Kiehl
- Justin Donnelly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Donnelly (Gerrit)
Justin Donnelly voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Donnelly
| Code-Review | +1 |
1 comment
Patchset 69 lgtm mod adressing Lei's last comment
Related details
- Josiah Kiehl
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl added 1 comment![]( "Open in Gerrit")
This needs to be above line 5. Please diff against the original.
- Justin Donnelly
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang voted Code-Review+1![]( "Open in Gerrit")
- Josiah Kiehl
- Justin Donnelly
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Donnelly (Gerrit)
Justin Donnelly voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Josiah Kiehl
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Lei Zhang (Gerrit)
Lei Zhang voted Code-Review+1![]( "Open in Gerrit")
- Josiah Kiehl
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Donnelly (Gerrit)
Justin Donnelly voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Josiah Kiehl
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Move omnibox base classes that are no longer used on iOS to //chrome.
//components/omnibox/browser -> //chrome/browser/ui/omnibox
This also creates new build targets,
"//chrome/browser/ui/omnibox:test_support"
- M chrome/browser/BUILD.gn
- M chrome/browser/autocomplete/autocomplete_browsertest.cc
- M chrome/browser/autocomplete/chrome_autocomplete_provider_client_browsertest.cc
- M chrome/browser/chrome_navigation_browsertest.cc
- M chrome/browser/chrome_render_widget_host_browsertests.cc
- M chrome/browser/extensions/api/omnibox/BUILD.gn
- M chrome/browser/extensions/api/omnibox/omnibox_api_interactive_test.cc
- M chrome/browser/extensions/extension_url_rewrite_browsertest.cc
- M chrome/browser/global_keyboard_shortcuts_interactive_uitest_mac.mm
- M chrome/browser/policy/safe_search_policy_test.cc
- M chrome/browser/policy/test/default_search_provider_policy_browsertest.cc
- M chrome/browser/preloading/prefetch/BUILD.gn
- M chrome/browser/preloading/prefetch/no_state_prefetch/prerender_nostate_prefetch_browsertest.cc
- M chrome/browser/preloading/prefetch/search_prefetch/search_prefetch_service_browsertest.cc
- M chrome/browser/preloading/prefetch/search_prefetch/search_preload_unified_browsertest.cc
- M chrome/browser/preloading/prefetch/zero_suggest_prefetch/zero_suggest_prefetch_tab_helper.cc
- M chrome/browser/preloading/prefetch/zero_suggest_prefetch/zero_suggest_prefetch_tab_helper_browsertest.cc
- M chrome/browser/preloading/prerender/prerender_omnibox_ui_browsertest.cc
- M chrome/browser/ssl/https_upgrades_browsertest.cc
- M chrome/browser/ssl/typed_navigation_upgrade_throttle_browsertest.cc
- M chrome/browser/ui/blocked_content/popup_blocker_browsertest.cc
- M chrome/browser/ui/browser_focus_uitest.cc
- M chrome/browser/ui/browser_navigator_browsertest.cc
- M chrome/browser/ui/lens/BUILD.gn
- M chrome/browser/ui/lens/lens_composebox_handler.cc
- M chrome/browser/ui/omnibox/BUILD.gn
- M chrome/browser/ui/omnibox/clipboard_utils.cc
- R chrome/browser/ui/omnibox/omnibox_controller.cc
- R chrome/browser/ui/omnibox/omnibox_controller.h
- R chrome/browser/ui/omnibox/omnibox_controller_unittest.cc
- R chrome/browser/ui/omnibox/omnibox_edit_model.cc
- R chrome/browser/ui/omnibox/omnibox_edit_model.h
- R chrome/browser/ui/omnibox/omnibox_edit_model_unittest.cc
- M chrome/browser/ui/omnibox/omnibox_metrics_browsertest.cc
- R chrome/browser/ui/omnibox/omnibox_popup_view.cc
- R chrome/browser/ui/omnibox/omnibox_popup_view.h
- M chrome/browser/ui/omnibox/omnibox_search_aggregator_browsertest.cc
- M chrome/browser/ui/omnibox/omnibox_tab_helper.cc
- R chrome/browser/ui/omnibox/omnibox_view.cc
- R chrome/browser/ui/omnibox/omnibox_view.h
- M chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
- R chrome/browser/ui/omnibox/omnibox_view_unittest.cc
- R chrome/browser/ui/omnibox/test_omnibox_edit_model.cc
- R chrome/browser/ui/omnibox/test_omnibox_edit_model.h
- R chrome/browser/ui/omnibox/test_omnibox_popup_view.cc
- R chrome/browser/ui/omnibox/test_omnibox_popup_view.h
- R chrome/browser/ui/omnibox/test_omnibox_view.cc
- R chrome/browser/ui/omnibox/test_omnibox_view.h
- M chrome/browser/ui/search/BUILD.gn
- M chrome/browser/ui/search/instant_extended_interactive_uitest.cc
- M chrome/browser/ui/search/omnibox_utils.cc
- M chrome/browser/ui/toasts/toast_controller_interactive_ui_test.cc
- M chrome/browser/ui/views/accessibility/navigation_accessibility_uitest_win.cc
- M chrome/browser/ui/views/frame/browser_view.cc
- M chrome/browser/ui/views/location_bar/ai_mode_page_action_icon_view.cc
- M chrome/browser/ui/views/location_bar/location_bar_view.cc
- M chrome/browser/ui/views/location_bar/location_icon_view.cc
- M chrome/browser/ui/views/location_bar/location_icon_view_browsertest.cc
- M chrome/browser/ui/views/omnibox/omnibox_header_view.cc
- M chrome/browser/ui/views/omnibox/omnibox_popup_closer.cc
- M chrome/browser/ui/views/omnibox/omnibox_popup_presenter.cc
- M chrome/browser/ui/views/omnibox/omnibox_popup_view_views.cc
- M chrome/browser/ui/views/omnibox/omnibox_popup_view_views.h
- M chrome/browser/ui/views/omnibox/omnibox_popup_view_views_test.h
- M chrome/browser/ui/views/omnibox/omnibox_popup_view_webui.cc
- M chrome/browser/ui/views/omnibox/omnibox_popup_view_webui.h
- M chrome/browser/ui/views/omnibox/omnibox_popup_view_webui_test.h
- M chrome/browser/ui/views/omnibox/omnibox_result_view.cc
- M chrome/browser/ui/views/omnibox/omnibox_result_view_unittest.cc
- M chrome/browser/ui/views/omnibox/omnibox_row_grouped_view_browsertest.cc
- M chrome/browser/ui/views/omnibox/omnibox_row_view.cc
- M chrome/browser/ui/views/omnibox/omnibox_suggestion_button_row_browsertest.cc
- M chrome/browser/ui/views/omnibox/omnibox_suggestion_button_row_view.cc
- M chrome/browser/ui/views/omnibox/omnibox_view_views.cc
- M chrome/browser/ui/views/omnibox/omnibox_view_views.h
- M chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
- M chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc
- M chrome/browser/ui/views/permissions/chip/LHS_indicators_interactive_uitest.cc
- M chrome/browser/ui/views/permissions/permission_request_chip_browsertest.cc
- M chrome/browser/ui/views/permissions/quiet_prompt_interactive_uitest.cc
- M chrome/browser/ui/views/sharing_hub/sharing_hub_icon_view.cc
- M chrome/browser/ui/views/tabs/tab_hover_card_controller.cc
- M chrome/browser/ui/views/toolbar/BUILD.gn
- M chrome/browser/ui/views/toolbar/toolbar_view.cc
- M chrome/browser/ui/views/user_education/impl/browser_feature_promo_controller_2x_interactive_uitest.cc
- M chrome/browser/ui/views/user_education/impl/browser_feature_promo_preconditions.cc
- M chrome/browser/ui/views/user_education/impl/browser_feature_promo_preconditions_interactive_uitest.cc
- M chrome/browser/ui/web_applications/pwa_install_page_action_browsertest.cc
- M chrome/browser/ui/web_applications/test/system_web_app_interactive_uitest.cc
- M chrome/browser/ui/webui/new_tab_page/composebox/BUILD.gn
- M chrome/browser/ui/webui/new_tab_page/composebox/composebox_handler.cc
- M chrome/browser/ui/webui/omnibox_popup/omnibox_popup_ui.cc
- M chrome/browser/ui/webui/omnibox_popup/omnibox_popup_web_contents_helper.cc
- M chrome/browser/ui/webui/searchbox/lens_searchbox_handler.cc
- M chrome/browser/ui/webui/searchbox/realbox_handler.cc
- M chrome/browser/ui/webui/searchbox/realbox_handler_browsertest.cc
- M chrome/browser/ui/webui/searchbox/searchbox_handler.cc
- M chrome/browser/ui/webui/searchbox/searchbox_handler_unittest.cc
- M chrome/browser/ui/webui/searchbox/searchbox_omnibox_client.cc
- M chrome/browser/ui/webui/searchbox/searchbox_test_utils.h
- M chrome/browser/ui/webui/searchbox/webui_omnibox_handler.cc
- M chrome/test/BUILD.gn
- M chrome/test/base/ui_test_utils.cc
- M components/omnibox/browser/BUILD.gn
Code-Review: +1 by Lei Zhang, +1 by Justin Donnelly
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |