WebUI page for rendering the Extensions Zero State Promo IPH. [chromium/src : main]
Yiming Zhou (Gerrit)
Yiming Zhou added 1 comment![]( "Open in Gerrit")
"META": {"sizes": {"includes": [124],}},Adding 4 new SVG files.
Related details
- Christopher Grant
- Dana Fried
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried added 3 comments![]( "Open in Gerrit")
Overall this looks pretty good but you're going to need a WebUI expert to sign off on all of the WebUI bits.
You _are_ going to need to write some regression/CUJ tests for this bubble, however. If you want to discuss how to do that we can chat, and it can be done in a follow-up considering how large this CL already is.
I'm wondering if there's a good way to break this CL up into multiple CLs. It is a fairly significant chunk of code.
enum WebStoreLinkClicked {This doesn't seem to line up with the UMA histogram values. Is there a separate enum in the code that's going to be emitted instead?
Also, If this *is* intended to be used for UMA reporting, you should clearly mark that the values are used for logging and should not be changed - there's boilerplate for this:
```
// These values are persisted to logs. Entries should not be renumbered and
// numeric values should never be reused.
```
Related details
- Christopher Grant
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiming Zhou (Gerrit)
Yiming Zhou added 3 comments![]( "Open in Gerrit")
Overall this looks pretty good but you're going to need a WebUI expert to sign off on all of the WebUI bits.
You _are_ going to need to write some regression/CUJ tests for this bubble, however. If you want to discuss how to do that we can chat, and it can be done in a follow-up considering how large this CL already is.
Oh any pointer is welcome!
I sampled a few webui change lists and concluded that just a unit test is enough.
But for the actual webui IPH, I am not sure. The test hooks in HelpBubbleView that other IPH interactive tests use do not apply here.
I'm wondering if there's a good way to break this CL up into multiple CLs. It is a fairly significant chunk of code.
Yeah... although it is a pretty hello-world case if not for all the UI flourishes.
enum WebStoreLinkClicked {This doesn't seem to line up with the UMA histogram values. Is there a separate enum in the code that's going to be emitted instead?
Also, If this *is* intended to be used for UMA reporting, you should clearly mark that the values are used for logging and should not be changed - there's boilerplate for this:
```
// These values are persisted to logs. Entries should not be renumbered and
// numeric values should never be reused.
```
I do intend these to be used for UMA reporting. And I am logging inside javascript instead of doing them in the page handler simply because it is less boilerplate code to write. But what, in your view, is the better practice?
Related details
- Christopher Grant
- Dana Fried
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried added 3 comments![]( "Open in Gerrit")
Yiming ZhouOverall this looks pretty good but you're going to need a WebUI expert to sign off on all of the WebUI bits.
You _are_ going to need to write some regression/CUJ tests for this bubble, however. If you want to discuss how to do that we can chat, and it can be done in a follow-up considering how large this CL already is.
Oh any pointer is welcome!
I sampled a few webui change lists and concluded that just a unit test is enough.
But for the actual webui IPH, I am not sure. The test hooks in HelpBubbleView that other IPH interactive tests use do not apply here.
You can look at the custom help bubble UI test, which is end-to-end:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/user_education/custom_webui_help_bubble_interactive_uitest.cc
Yiming ZhouI'm wondering if there's a good way to break this CL up into multiple CLs. It is a fairly significant chunk of code.
Yeah... although it is a pretty hello-world case if not for all the UI flourishes.
Yeah I think this isn't a hard requirement; I'm happy with the code.
enum WebStoreLinkClicked {Yiming ZhouThis doesn't seem to line up with the UMA histogram values. Is there a separate enum in the code that's going to be emitted instead?
Also, If this *is* intended to be used for UMA reporting, you should clearly mark that the values are used for logging and should not be changed - there's boilerplate for this:
```
// These values are persisted to logs. Entries should not be renumbered and
// numeric values should never be reused.
```
I do intend these to be used for UMA reporting. And I am logging inside javascript instead of doing them in the page handler simply because it is less boilerplate code to write. But what, in your view, is the better practice?
It's fine; you *will* need to put the comment boilerplate in. Just make sure the numeric values line up (they didn't appear to before).
Also if it is possible for .mojom files you should use IfChange...ThenChange in both files (mojom and enums) to ensure they stay in sync.
https://www.chromium.org/chromium-os/developer-library/guides/development/keep-files-in-sync/
Related details
- Christopher Grant
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christopher Grant (Gerrit)
Christopher Grant added 1 comment![]( "Open in Gerrit")
Removing self; Dana's got this covered.
Related details
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiming Zhou (Gerrit)
Yiming Zhou added 2 comments![]( "Open in Gerrit")
Yiming ZhouOverall this looks pretty good but you're going to need a WebUI expert to sign off on all of the WebUI bits.
You _are_ going to need to write some regression/CUJ tests for this bubble, however. If you want to discuss how to do that we can chat, and it can be done in a follow-up considering how large this CL already is.
Dana FriedOh any pointer is welcome!
I sampled a few webui change lists and concluded that just a unit test is enough.
But for the actual webui IPH, I am not sure. The test hooks in HelpBubbleView that other IPH interactive tests use do not apply here.
You can look at the custom help bubble UI test, which is end-to-end:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/user_education/custom_webui_help_bubble_interactive_uitest.cc
Thanks! I added a big test in the subsequent cl, which actually hooks up the page to a custom UI featurepromo: https://chromium-review.googlesource.com/c/chromium/src/+/6404446
Hi Rebekah, may I trouble you to review the webui files and give feedback? Thank you so much!
Related details
- Dana Fried
- Rebekah Potter
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter added 3 comments![]( "Open in Gerrit")
Sending some initial comments (did not have time for a full review today due to an all day meeting). Also adding johntlee@ since I will be ooo tomorrow.
#couponsButton .cr-icon {
--cr-icon-image: url(icons/coupons.svg);
}
#writingButton .cr-icon {
--cr-icon-image: url(icons/writing.svg);
}
#productivityButton .cr-icon {
--cr-icon-image: url(icons/productivity.svg);
}
#aiButton .cr-icon {
--cr-icon-image: url(icons/ai.svg);
}Why don't we just use cr-icon, put all these svgs in an iconset, and pass the appropriate icon id in the template? This will also prevent the need for an extra grdp to put all the icons in.
webui::SetupWebUIDataSource(source, kExtensionsResources,Why do we need a second WebUI that serves all the same resources as chrome://extensions?
Should this just be a subpage/different route on the extensions page, or is it intended to be a fully separate UI? If the latter, it should have its own resource bundle/build_webui target.
Related details
- Dana Fried
- John Lee
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John Lee (Gerrit)
John Lee added 6 comments![]( "Open in Gerrit")
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="32" height="32" fill="none" viewBox="0 0 32 32"><path fill="url(#a)" d="M0 0h32v32H0z"/><defs><pattern id="a" width="1" height="1" patternContentUnits="objectBoundingBox"><use xlink:href="#b" transform="scale(.0052)"/></pattern><image xlink:href="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAMAAAADACAYAAABS3GwHAAANvUlEQVR4AezBgQAAAACAoP2pF6kCAAAAAAAAAAAAAAAAAABm7z5yGoaiKAw/+oxlMvNmmLGFbCKLIL1IuJte55dc2oSOpaf48Ef6w5B2vlRb6falKArrWnme/zHKskyt8FV+9WV1XVusqqr6Y1SWJX1c+Cq/+jIAACB+ACAAAIAAAAD9AAAAxg8AABAAACAZAADA+Bk/AAAAAAAwfgAAAAAAAADjBQAAZAMAAATGn2eZLZdLm4zHNhwObTAY2OD4eH0bDPxntcV87oeGAyA+AJ3xLxYLm0wmjsAuLi7s5vraC+vc0cHBzvn5uWVZZqPRyCEAAAC/bzqZWFWW9vDw4IUudnd3Z2ma2mw6BcCaAOjMLf/Z2ZkXFEqSZD/GPQEABMaf57nf8ntBqSxN/XcDQHsA2o/705OTt4c9Sq0uG/67AaA9AOmXOpum8YJi/v8AQHsAsuP3rq+uvKDYYZLsAaA9AOU3uCQf/rxmZpsAaA9A/Z3doJqZbQCgPQDpQxpub2+9oFi/398GQHsAPhTZLi8vvaCYv0MMgF8CkBv51zlw2ecBAmMGQIwjK/1eQPDWP8LBcQCQQeCPl1cFkbZjvgsMAIH81tLvCRRu+SXGD4D4+XD8qx8KbWabq0IX6vV6W69PeNuOHwCcbdXVzxyQfMwPAEASAKIGAAAQAABAjB8ABAAAEAAAQIwfAAQAABDjBwABAADE+AFAAAAAAQAAxPgBQAAAADF+ABAAAECMHwDE+AFAAAAAMX4AEAAAwPgJAIyfAAAAAgDjJwAAgADA+AkAjJ8AwPgJAAAgAQD/ffyDR/buGzdiGArC8N7Lm3OWc7i1Y+XsfsMJnD3Olh8XT8KImOID2PMfgZ0mDQDFWvwByNm0+ReEblBPQxmaBqDYHeKHut249kyhawA5BJ+0Xk2bDPFjAJDhGDQARR8eP+Qav/8YNABF7xG//9PHbvRBA9AA7NEzxg/h8fMPQQMgDD+i+PmHoAEQhB9B/DAyGFafRRi+BqD47QMADQAg/vD544cM4wfQAAC/3S+y46RlN22uZtIIcjSphxnX7EZGw+q/7u7uiq6UBoc/LZfLh6K6PpiE2R+vaBRmL9Du0Oxqd2Cz07fZ7rvfy2KxyFMpDQ5pHormcr0dLmmtZtoM1AgzqduNbS5GNaPqZy53M5/P81ZKg8OfCIJW/AQDAN74NQCH8EniB7L4QQOIP37Fb0ASvwbAGz5P/M4DAA0g1vAVv8Gw8izf+DUAxQ8E8YMGkCff8BW/wwAg6/g1AO7waeKHPOMHDSAr9vD5v/qU8TsMAHzj1wAIwo8xfnCMHwZlDcCLPXzFzxA/BuAwAg2AIHzFD/b4NQAP/tHzx+82AIL4QQMIh+jjCR/xx//0cRmBBsARvuJ3iB80ACP+8OOPH7zih5gGoPgVv30A0P9zBBoAT/SO4St+DcAgs/AVP3DEDxrAhzjCB//wPeJnGgBoAOAfvuInjx80gEf2zgFIkqQLwP3btm3bHNu2zmvbtm3bGKztMXp7tqfHbFT1Gu/2xV1fXMdNve7qyOvJvKmJ+Co8/L6szCzMlhdDGUgvoPia/FoAyF+XpYM+0tvS0+Kzl1+Tn6LXBvCtTdmATB4aRUvPp/i9U34tAPYB/GxNBlxN8GMrOiG+Jr/3R3+af/S+AL65McuJ3EnxvVN8TX4tAAcHM4P4FB/R5FeFCvkd9IIACPmRgPkpfElPi69a/jvRfmDMjAfTkNegYcpoaF69DKSb20G6shKkC3NBOjkepGMDQDraTxW2o33Vc0Qtfdhz+DUnekUAKDrFkj7hnojPnfz4VuWGiSOg/egBMFdXgq2lGSRJcgvZ0ghyRznca9gLj2uGAZSEAdwKYIi/Om76qcCXhkYL4Dcr06Ek1k+F9PyIb0yLhqa5U6Hz3GmwdXaizEyQbe1wr6UAHt2eDM9KosWXn4b7ABjITzNsVAwtPWfim4a+Bp0Xz7ESnsZmgbvNRfCk4kXO5NcCYBbAdzZkwdlkf1p6HsR/MRU6TuSjmF7FZrM9xwL3Gg/A09IkMeWnETQABvJ/402iZyainNxOddoO7wObxdIj8jth7YT7pk3wrCRKRPm1AL5BsC0vhLvFbfOKhcT83qvyOyFZW+CRYapQ8tP4vPcC+IZK/rIsDfQc7ep0HD/isbwWqwS1TTJcrZEg/6YMO87LsLxAhlVFMuy6KENhsQzX9TIYmmQX8tPcr98KUBwsuvzvrQBo0WkmDY7oUfERY0YMmItvqJYehd9xQYZBm+0QNtsOQTPdI2KuHYZvk2H/FRlMrSi2Ou62nsEpkfoA+JFfC8DBT1enw9V4X0bSq8c04EWwmkxuS29qlXBUh1fW2CF4JkLLTjLjDfpvtMOmMzK0dLgfgdxZCU/LswQb/X20ALojZ1Kc18VHGiaOdHu+394lwdqTdohbgNKzkt+ZpEV22H4ep1Ok/E7rgidV/cSa+jgjfAAoMBMOZgZS0jPHNPgVYpfHeW6/77Idkhej8AzlJ8habof8m5K7EeCZQAj5tQAI/OcnE9KzBe/XsZrq3Jrjv7SaEp+B/AQDN8nQ2ObedAjXBPzLrwVAsvi1MEJcBoTibk8AmMuKXcp/uVr26qhPnQ3KjO4tjKE4iFv5CQQNgIH8X9+Y6cSvVqRBcYzPuyG+A7eu7B64IkPYbC+O+gSBz4mca4dTpZJ7W6RcyK8FQAtPMHRkNHPxHbSsXeFS/mX5tPjelt9ByCxcILuO4JFhCv/y94IAaNEJvrUhE84k+jJ/OMWYk+Ry0bvvMiejvgIYgaszgWRpgWfFkTzKT/B/YQNAaZkTPTOB+VNZHYXHSPkvVuG0h5NRnyByrss1Ad47JJL8QgZACMyGrXnB6qQnqOubS8qvb5AgcZHXxaflJ8hcZqd3hyyd8LQkkXv5tQAI/rw0FfThhPgq6Lp8UVH+TrMEuSv5lJ8At0jJs8C9xv2E/HyN/g60AJyBiYMj3JOeoGHMYHL033xW5ld8goDpdjh+g1gPWM3wpDyPe/m1AAh+vCoVrsT9n5achtzzb+mQIHaBmPIjOBUyW4hrA82FosiPaAF8bUPGO8ieGOux/HUvZ5Cj/6LjsvfFZyS/g23n6bMA7ghxJr/4AbAXnuZAur9HATSvWqIoP96XHzGXf/Ep+ZHEhXbyLtJHtyfyIr8WACk6ge+8JI8CoO7xn3tYFmrUJ8BbqZUXwy35/MsvfgCMZCdY/Fqoqjeu3UmPJqc/qYtl9eJzKD/Sb6OduFu0HZ8g401+8QOghWXPL1ekQnH0/wnpnWmaN01Rfnw0kV/xCfkJqCfLHlcP5kl+8QNAIXuCISOiCPGd6bxwVikAfKKLEF88+ZF9VyTlaVDDHv7k1wJQzzfXZ8CZBB9SfAe29nbFAHJXCio+wbCtyusAub2MI/kJbvxPC6A7vvo2ImfEu5S/NiaAfKaXK/EZyO//nNDZxDrA3Mh7ACi/FoCT8ARbcwLJAIzZidSDLoT4YsrvwNCkPA2CWwE8y9+7A/iqSv64JAX0ocoBmIa8qhQAPmsrrviE/Mg1vXIAT0uTeZFfC+CrDJgwKEwxgMZpYxVfOrXlrMSF+KzlRwpuKQfwpKoPr/ILFYB3ZSf4Ed4nFPv/bgNoXjpf8W1rS/NlcUd9Qn5k5wXlAB7VjOZVfvED+GoPkTUhptt/JdS6ZZ2iCFP2yWKP+gQrCpUDeFg7m734jOVXG0BHRweio8ADSVdXl5oAUDyuOJzmj9I70bp9o6IIk/bI/ItPyU+wLJ8I4M48DuXnIIBTp059UtQAvrI+HXzmJrwjgOblCxRFwDtA1YvPv/zItnPEFEg/jkP5OQhg3rx5nxUkABS+W5a+HOQUQOOMiUoi4I1jYotPQD0g87iqH/fyqw2gtbUV0VHggWTo0KGfUrsO8L70NL9YngLl4f9xBICvPVQU4eh1iXvxSfkJrlTbiG3QJA7l5yCAPn36fKStrY2bAEjZCQYPj3grgLq8ZEURLldJhPS8j/oE01xcCCsO5l5+NQE0NzWBTqd7v47BB0bwJfYBsBee4ttr0+B0/P8xAKiNC1YUASXhUXzEM/ERjFn58UjJ0sy//CoC6OzshMaGBkTnCjy4ZHxc3IcbGhrYB8BedpLoqbEYAIKvPVeMIG2JLJ74hPxI/43EzXAdlfzLryIAU10dMfp7ehZISfk0nlbUB+B90Sm25ARiANB19ZLyTtAxmQvx2ciP0DtA95oOsZGfgwDq6+vpuT+9BqBJT0//An4BFwGgaNzy50VJUBP6b2heOo9cB4gvvjOGJneuAvtwLT8VAF6vqjMace6P6NwFD6rIDg//VK3BAMTCGEXjmDSYNCAUjFnxikLgf2CJW6hWfH7lf2GVTLwlrgOe3QoVWv7m5mYwGAzEyK/6DEDzwh/+8CGcZ92prYWmxka82IAFchxAmhM/WJkMV2L+C+aqCvKWCGHEJ+RHVhQoj/53W05zKD8dAPqGAzCuS1F8k8mEc/4P6Lz9gVukySEhn8MQ8KxwW68H5GuLE3ucry5KIEkfEgrli+bh990t564bIHC8EfzGdo8vS8Z4jo8LwiYZ4WaZQfHnbLoyGtqK/qOSf3uHwn85oa+pQfD7fmvw/a9O98Hn6DwDD6+3TwckAAAACID+v45mRAJO8BQIUAgAAoAAIAAIAAKAACAACAACwAABQAAQAAQAAUAAEAAEgAB5lREvwKhUDgAAAABJRU5ErkJggg==" id="b" width="192" height="192"/></defs></svg>Should this just be a png file?
--zero-state-promo-foreground-color: var(--cr-fallback-color-on-primary);Why is this using a hybrid of the color pipeline and fallback/md colors? Could we stick to one?
aira-label="close"aria
You can also probably remove this attribute if you already have a `title` attribute below.
aira-label="close"
title="close"These strings should be translated.
<div id="sectionHeaderContainer">Please fix indentation in this file.
dismissPromoButtonToast: CrToastElement,Was this copied from somewhere? I don't see a toast.
Related details
- Dana Fried
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried added 1 comment![]( "Open in Gerrit")
Please resolve merge conflicts and ensure tests pass.
Related details
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiming Zhou (Gerrit)
Yiming Zhou added 10 comments![]( "Open in Gerrit")
Please resolve merge conflicts and ensure tests pass.
Done
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="32" height="32" fill="none" viewBox="0 0 32 32"><path fill="url(#a)" d="M0 0h32v32H0z"/><defs><pattern id="a" width="1" height="1" patternContentUnits="objectBoundingBox"><use xlink:href="#b" transform="scale(.0052)"/></pattern><image xlink:href="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAMAAAADACAYAAABS3GwHAAANvUlEQVR4AezBgQAAAACAoP2pF6kCAAAAAAAAAAAAAAAAAABm7z5yGoaiKAw/+oxlMvNmmLGFbCKLIL1IuJte55dc2oSOpaf48Ef6w5B2vlRb6falKArrWnme/zHKskyt8FV+9WV1XVusqqr6Y1SWJX1c+Cq/+jIAACB+ACAAAIAAAAD9AAAAxg8AABAAACAZAADA+Bk/AAAAAAAwfgAAAAAAAADjBQAAZAMAAATGn2eZLZdLm4zHNhwObTAY2OD4eH0bDPxntcV87oeGAyA+AJ3xLxYLm0wmjsAuLi7s5vraC+vc0cHBzvn5uWVZZqPRyCEAAAC/bzqZWFWW9vDw4IUudnd3Z2ma2mw6BcCaAOjMLf/Z2ZkXFEqSZD/GPQEABMaf57nf8ntBqSxN/XcDQHsA2o/705OTt4c9Sq0uG/67AaA9AOmXOpum8YJi/v8AQHsAsuP3rq+uvKDYYZLsAaA9AOU3uCQf/rxmZpsAaA9A/Z3doJqZbQCgPQDpQxpub2+9oFi/398GQHsAPhTZLi8vvaCYv0MMgF8CkBv51zlw2ecBAmMGQIwjK/1eQPDWP8LBcQCQQeCPl1cFkbZjvgsMAIH81tLvCRRu+SXGD4D4+XD8qx8KbWabq0IX6vV6W69PeNuOHwCcbdXVzxyQfMwPAEASAKIGAAAQAABAjB8ABAAAEAAAQIwfAAQAABDjBwABAADE+AFAAAAAAQAAxPgBQAAAADF+ABAAAECMHwDE+AFAAAAAMX4AEAAAwPgJAIyfAAAAAgDjJwAAgADA+AkAjJ8AwPgJAAAgAQD/ffyDR/buGzdiGArC8N7Lm3OWc7i1Y+XsfsMJnD3Olh8XT8KImOID2PMfgZ0mDQDFWvwByNm0+ReEblBPQxmaBqDYHeKHut249kyhawA5BJ+0Xk2bDPFjAJDhGDQARR8eP+Qav/8YNABF7xG//9PHbvRBA9AA7NEzxg/h8fMPQQMgDD+i+PmHoAEQhB9B/DAyGFafRRi+BqD47QMADQAg/vD544cM4wfQAAC/3S+y46RlN22uZtIIcjSphxnX7EZGw+q/7u7uiq6UBoc/LZfLh6K6PpiE2R+vaBRmL9Du0Oxqd2Cz07fZ7rvfy2KxyFMpDQ5pHormcr0dLmmtZtoM1AgzqduNbS5GNaPqZy53M5/P81ZKg8OfCIJW/AQDAN74NQCH8EniB7L4QQOIP37Fb0ASvwbAGz5P/M4DAA0g1vAVv8Gw8izf+DUAxQ8E8YMGkCff8BW/wwAg6/g1AO7waeKHPOMHDSAr9vD5v/qU8TsMAHzj1wAIwo8xfnCMHwZlDcCLPXzFzxA/BuAwAg2AIHzFD/b4NQAP/tHzx+82AIL4QQMIh+jjCR/xx//0cRmBBsARvuJ3iB80ACP+8OOPH7zih5gGoPgVv30A0P9zBBoAT/SO4St+DcAgs/AVP3DEDxrAhzjCB//wPeJnGgBoAOAfvuInjx80gEf2zgFIkqQLwP3btm3bHNu2zmvbtm3bGKztMXp7tqfHbFT1Gu/2xV1fXMdNve7qyOvJvKmJ+Co8/L6szCzMlhdDGUgvoPia/FoAyF+XpYM+0tvS0+Kzl1+Tn6LXBvCtTdmATB4aRUvPp/i9U34tAPYB/GxNBlxN8GMrOiG+Jr/3R3+af/S+AL65McuJ3EnxvVN8TX4tAAcHM4P4FB/R5FeFCvkd9IIACPmRgPkpfElPi69a/jvRfmDMjAfTkNegYcpoaF69DKSb20G6shKkC3NBOjkepGMDQDraTxW2o33Vc0Qtfdhz+DUnekUAKDrFkj7hnojPnfz4VuWGiSOg/egBMFdXgq2lGSRJcgvZ0ghyRznca9gLj2uGAZSEAdwKYIi/Om76qcCXhkYL4Dcr06Ek1k+F9PyIb0yLhqa5U6Hz3GmwdXaizEyQbe1wr6UAHt2eDM9KosWXn4b7ABjITzNsVAwtPWfim4a+Bp0Xz7ESnsZmgbvNRfCk4kXO5NcCYBbAdzZkwdlkf1p6HsR/MRU6TuSjmF7FZrM9xwL3Gg/A09IkMeWnETQABvJ/402iZyainNxOddoO7wObxdIj8jth7YT7pk3wrCRKRPm1AL5BsC0vhLvFbfOKhcT83qvyOyFZW+CRYapQ8tP4vPcC+IZK/rIsDfQc7ep0HD/isbwWqwS1TTJcrZEg/6YMO87LsLxAhlVFMuy6KENhsQzX9TIYmmQX8tPcr98KUBwsuvzvrQBo0WkmDY7oUfERY0YMmItvqJYehd9xQYZBm+0QNtsOQTPdI2KuHYZvk2H/FRlMrSi2Ou62nsEpkfoA+JFfC8DBT1enw9V4X0bSq8c04EWwmkxuS29qlXBUh1fW2CF4JkLLTjLjDfpvtMOmMzK0dLgfgdxZCU/LswQb/X20ALojZ1Kc18VHGiaOdHu+394lwdqTdohbgNKzkt+ZpEV22H4ep1Ok/E7rgidV/cSa+jgjfAAoMBMOZgZS0jPHNPgVYpfHeW6/77Idkhej8AzlJ8habof8m5K7EeCZQAj5tQAI/OcnE9KzBe/XsZrq3Jrjv7SaEp+B/AQDN8nQ2ObedAjXBPzLrwVAsvi1MEJcBoTibk8AmMuKXcp/uVr26qhPnQ3KjO4tjKE4iFv5CQQNgIH8X9+Y6cSvVqRBcYzPuyG+A7eu7B64IkPYbC+O+gSBz4mca4dTpZJ7W6RcyK8FQAtPMHRkNHPxHbSsXeFS/mX5tPjelt9ByCxcILuO4JFhCv/y94IAaNEJvrUhE84k+jJ/OMWYk+Ry0bvvMiejvgIYgaszgWRpgWfFkTzKT/B/YQNAaZkTPTOB+VNZHYXHSPkvVuG0h5NRnyByrss1Ad47JJL8QgZACMyGrXnB6qQnqOubS8qvb5AgcZHXxaflJ8hcZqd3hyyd8LQkkXv5tQAI/rw0FfThhPgq6Lp8UVH+TrMEuSv5lJ8At0jJs8C9xv2E/HyN/g60AJyBiYMj3JOeoGHMYHL033xW5ld8goDpdjh+g1gPWM3wpDyPe/m1AAh+vCoVrsT9n5achtzzb+mQIHaBmPIjOBUyW4hrA82FosiPaAF8bUPGO8ieGOux/HUvZ5Cj/6LjsvfFZyS/g23n6bMA7ghxJr/4AbAXnuZAur9HATSvWqIoP96XHzGXf/Ep+ZHEhXbyLtJHtyfyIr8WACk6ge+8JI8CoO7xn3tYFmrUJ8BbqZUXwy35/MsvfgCMZCdY/Fqoqjeu3UmPJqc/qYtl9eJzKD/Sb6OduFu0HZ8g401+8QOghWXPL1ekQnH0/wnpnWmaN01Rfnw0kV/xCfkJqCfLHlcP5kl+8QNAIXuCISOiCPGd6bxwVikAfKKLEF88+ZF9VyTlaVDDHv7k1wJQzzfXZ8CZBB9SfAe29nbFAHJXCio+wbCtyusAub2MI/kJbvxPC6A7vvo2ImfEu5S/NiaAfKaXK/EZyO//nNDZxDrA3Mh7ACi/FoCT8ARbcwLJAIzZidSDLoT4YsrvwNCkPA2CWwE8y9+7A/iqSv64JAX0ocoBmIa8qhQAPmsrrviE/Mg1vXIAT0uTeZFfC+CrDJgwKEwxgMZpYxVfOrXlrMSF+KzlRwpuKQfwpKoPr/ILFYB3ZSf4Ed4nFPv/bgNoXjpf8W1rS/NlcUd9Qn5k5wXlAB7VjOZVfvED+GoPkTUhptt/JdS6ZZ2iCFP2yWKP+gQrCpUDeFg7m734jOVXG0BHRweio8ADSVdXl5oAUDyuOJzmj9I70bp9o6IIk/bI/ItPyU+wLJ8I4M48DuXnIIBTp059UtQAvrI+HXzmJrwjgOblCxRFwDtA1YvPv/zItnPEFEg/jkP5OQhg3rx5nxUkABS+W5a+HOQUQOOMiUoi4I1jYotPQD0g87iqH/fyqw2gtbUV0VHggWTo0KGfUrsO8L70NL9YngLl4f9xBICvPVQU4eh1iXvxSfkJrlTbiG3QJA7l5yCAPn36fKStrY2bAEjZCQYPj3grgLq8ZEURLldJhPS8j/oE01xcCCsO5l5+NQE0NzWBTqd7v47BB0bwJfYBsBee4ttr0+B0/P8xAKiNC1YUASXhUXzEM/ERjFn58UjJ0sy//CoC6OzshMaGBkTnCjy4ZHxc3IcbGhrYB8BedpLoqbEYAIKvPVeMIG2JLJ74hPxI/43EzXAdlfzLryIAU10dMfp7ehZISfk0nlbUB+B90Sm25ARiANB19ZLyTtAxmQvx2ciP0DtA95oOsZGfgwDq6+vpuT+9BqBJT0//An4BFwGgaNzy50VJUBP6b2heOo9cB4gvvjOGJneuAvtwLT8VAF6vqjMace6P6NwFD6rIDg//VK3BAMTCGEXjmDSYNCAUjFnxikLgf2CJW6hWfH7lf2GVTLwlrgOe3QoVWv7m5mYwGAzEyK/6DEDzwh/+8CGcZ92prYWmxka82IAFchxAmhM/WJkMV2L+C+aqCvKWCGHEJ+RHVhQoj/53W05zKD8dAPqGAzCuS1F8k8mEc/4P6Lz9gVukySEhn8MQ8KxwW68H5GuLE3ucry5KIEkfEgrli+bh990t564bIHC8EfzGdo8vS8Z4jo8LwiYZ4WaZQfHnbLoyGtqK/qOSf3uHwn85oa+pQfD7fmvw/a9O98Hn6DwDD6+3TwckAAAACID+v45mRAJO8BQIUAgAAoAAIAAIAAKAACAACAACwAABQAAQAAQAAUAAEAAEgAB5lREvwKhUDgAAAABJRU5ErkJggg==" id="b" width="192" height="192"/></defs></svg>Should this just be a png file?
Got rid of the image, no longer needed.
--zero-state-promo-foreground-color: var(--cr-fallback-color-on-primary);Why is this using a hybrid of the color pipeline and fallback/md colors? Could we stick to one?
Done
#couponsButton .cr-icon {
--cr-icon-image: url(icons/coupons.svg);
}
#writingButton .cr-icon {
--cr-icon-image: url(icons/writing.svg);
}
#productivityButton .cr-icon {
--cr-icon-image: url(icons/productivity.svg);
}
#aiButton .cr-icon {
--cr-icon-image: url(icons/ai.svg);
}Why don't we just use cr-icon, put all these svgs in an iconset, and pass the appropriate icon id in the template? This will also prevent the need for an extra grdp to put all the icons in.
Done
aria
You can also probably remove this attribute if you already have a `title` attribute below.
Done
These strings should be translated.
Done
Please fix indentation in this file.
Done
Was this copied from somewhere? I don't see a toast.
Done
Yiming ZhouThis doesn't seem to line up with the UMA histogram values. Is there a separate enum in the code that's going to be emitted instead?
Also, If this *is* intended to be used for UMA reporting, you should clearly mark that the values are used for logging and should not be changed - there's boilerplate for this:
```
// These values are persisted to logs. Entries should not be renumbered and
// numeric values should never be reused.
```
Dana FriedI do intend these to be used for UMA reporting. And I am logging inside javascript instead of doing them in the page handler simply because it is less boilerplate code to write. But what, in your view, is the better practice?
It's fine; you *will* need to put the comment boilerplate in. Just make sure the numeric values line up (they didn't appear to before).
Also if it is possible for .mojom files you should use IfChange...ThenChange in both files (mojom and enums) to ensure they stay in sync.
https://www.chromium.org/chromium-os/developer-library/guides/development/keep-files-in-sync/
Done
webui::SetupWebUIDataSource(source, kExtensionsResources,Why do we need a second WebUI that serves all the same resources as chrome://extensions?
Should this just be a subpage/different route on the extensions page, or is it intended to be a fully separate UI? If the latter, it should have its own resource bundle/build_webui target.
- Dana Fried
- John Lee
- Rebekah Potter
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter added 15 comments![]( "Open in Gerrit")
Also adding extensions WebUI OWNER, since this new UI is going into the same folders, so will be owned by the same team.
"//tools/typescript/definitions/chrome_send.d.ts",
"//tools/typescript/definitions/developer_private.d.ts",are we using all these APIs? In particular chrome.send isn't usually needed for a new UI that's relying on Mojo. Same question for all the .d.ts files below, e.g. tabs, runtime, pending etc.
"//ui/webui/resources/cr_components/cr_shortcut_input:build_ts",where is this used?
"//ui/webui/resources/cr_components/managed_footnote:build_ts",where is this used?
if (optimize) {
optimize_webui_host = "extensions-zero-state"
optimize_webui_in_files = [ "zero_state_promo.js" ]
}This UI only has a couple of files. Usually such UIs don't benefit from bundling. Are you seeing a significant performance improvement from this?
* #scheme=relativeOmit, or actually use scheme-relative imports in this file?
#contentContainer {should these styles be explicitly shared with #promoContainer, e.g. with a CSS class, or just by partially combining the rules? It seems like they are using an identical gap for example.
#couponsButton .cr-icon {
--cr-icon-image: url(icons/coupons.svg);
}
#writingButton .cr-icon {
--cr-icon-image: url(icons/writing.svg);
}
#productivityButton .cr-icon {
--cr-icon-image: url(icons/productivity.svg);
}
#aiButton .cr-icon {
--cr-icon-image: url(icons/ai.svg);
}out of date?
<extensions-zero-state-promo>
</extensions-zero-state-promo>fits on one line?
@click="${this.onProductivityButtonClick_}">here and elsewhere in this file: wrapped HTML lines should be indented +4 from the preceding line.
import 'chrome://resources/cr_elements/cr_menu_selector/cr_menu_selector.js';where is this used?
this.apiProxy_ = ZeroStatePromoBrowserProxyImpl.getInstance();
this.customHelpBubblehandler_ =
CustomHelpBubbleProxyImpl.getInstance().getHandler();Just do this in the member declarations above, and don't override the constructor?
handler: PageHandlerRemote;Normally browser proxies follow one of the following patterns:
(1) Public `handler`, such that clients can just call `BrowserProxy.getInstance().handler.someMethod()` or
(2) Any `handler` is private, and wrapper methods (like on line 38) are used. So clients use `BrowserProxy.getInstance().someMethod()`
This class uses both, so it's not clear how clients are supposed to use it. Can we either make handler private, if it's not intended for clients to invoke methods on it directly, or remove the wrapper method?
: public TopChromeWebUIController,Not the expert on this, but I thought Top Chrome WebUIs where generally UIs with a ".top-chrome" host. Is there a reason this is using TopChromeWebUIController and not just MojoWebUIController?
Related details
- Dana Fried
- John Lee
- Kelvin Jiang
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried added 1 comment![]( "Open in Gerrit")
I am awaiting resolution of Rebekah's feedback, as she's more of an expert in this area. Feel free to tag me in any questions/threads if you want my input or don't know how to resolve an issue (or DM me)
Related details
Kelvin Jiang (Gerrit)
Kelvin Jiang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kelvin Jiang
| Code-Review | +1 |
1 comment
- John Lee
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiming Zhou (Gerrit)
Yiming Zhou added 14 comments![]( "Open in Gerrit")
"//tools/typescript/definitions/chrome_send.d.ts",
"//tools/typescript/definitions/developer_private.d.ts",are we using all these APIs? In particular chrome.send isn't usually needed for a new UI that's relying on Mojo. Same question for all the .d.ts files below, e.g. tabs, runtime, pending etc.
Done
"//ui/webui/resources/cr_components/cr_shortcut_input:build_ts",Yiming Zhouwhere is this used?
Removed.
"//ui/webui/resources/cr_components/managed_footnote:build_ts",Yiming Zhouwhere is this used?
Removed.
if (optimize) {
optimize_webui_host = "extensions-zero-state"
optimize_webui_in_files = [ "zero_state_promo.js" ]
}This UI only has a couple of files. Usually such UIs don't benefit from bundling. Are you seeing a significant performance improvement from this?
Removed.
Omit, or actually use scheme-relative imports in this file?
Omitted.
should these styles be explicitly shared with #promoContainer, e.g. with a CSS class, or just by partially combining the rules? It seems like they are using an identical gap for example.
Removed.
#couponsButton .cr-icon {
--cr-icon-image: url(icons/coupons.svg);
}
#writingButton .cr-icon {
--cr-icon-image: url(icons/writing.svg);
}
#productivityButton .cr-icon {
--cr-icon-image: url(icons/productivity.svg);
}
#aiButton .cr-icon {
--cr-icon-image: url(icons/ai.svg);
}Yiming Zhouout of date?
Replaced with the proper variable.
body {
margin: 0px;
}Yiming Zhouindentation is off
Done
<extensions-zero-state-promo>
</extensions-zero-state-promo>Yiming Zhoufits on one line?
Done
here and elsewhere in this file: wrapped HTML lines should be indented +4 from the preceding line.
Done
import 'chrome://resources/cr_elements/cr_menu_selector/cr_menu_selector.js';Yiming Zhouwhere is this used?
Removed
this.apiProxy_ = ZeroStatePromoBrowserProxyImpl.getInstance();
this.customHelpBubblehandler_ =
CustomHelpBubbleProxyImpl.getInstance().getHandler();Just do this in the member declarations above, and don't override the constructor?
Done
Normally browser proxies follow one of the following patterns:
(1) Public `handler`, such that clients can just call `BrowserProxy.getInstance().handler.someMethod()` or
(2) Any `handler` is private, and wrapper methods (like on line 38) are used. So clients use `BrowserProxy.getInstance().someMethod()`This class uses both, so it's not clear how clients are supposed to use it. Can we either make handler private, if it's not intended for clients to invoke methods on it directly, or remove the wrapper method?
Done
Not the expert on this, but I thought Top Chrome WebUIs where generally UIs with a ".top-chrome" host. Is there a reason this is using TopChromeWebUIController and not just MojoWebUIController?
- John Lee
- Kelvin Jiang
- Rebekah Potter
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiming Zhou (Gerrit)
Yiming Zhou added 1 comment![]( "Open in Gerrit")
: public TopChromeWebUIController,Yiming ZhouNot the expert on this, but I thought Top Chrome WebUIs where generally UIs with a ".top-chrome" host. Is there a reason this is using TopChromeWebUIController and not just MojoWebUIController?
Switched to MojoWebUiController.
Actually this class does need to extend TopChromeWebUI, as that is what the CustomWebUIHelpBubble class, which creates the webui-based IPH in https://chromium-review.googlesource.com/c/chromium/src/+/6404446, requires. Perhaps @dfr...@chromium.org can chime in, and on whether the association with a ".top-chrome" host is a hard requirement.
Related details
- John Lee
- Kelvin Jiang
- Rebekah Potter
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter added 9 comments![]( "Open in Gerrit")
<cr-iconset name="icons" size="14">nit: use a more descriptive name?
#couponsButton .cr-icon {
--cr-icon-image: url(icons/coupons.svg);
}
#writingButton .cr-icon {
--cr-icon-image: url(icons/writing.svg);
}
#productivityButton .cr-icon {
--cr-icon-image: url(icons/productivity.svg);
}
#aiButton .cr-icon {
--cr-icon-image: url(icons/ai.svg);
}These selectors do not apply to any element in the app. Nothing has a CSS class of cr-icon.
Moreover, cr-icon instances do not require additional CSS variables to set their icons when using the icon property.
<h3 id="title" class="col">$i18n{extensionsZeroStateIphHeader}</h3>where is this CSS class used? I don't see it in the CSS file.
import 'chrome://resources/cr_elements/icons.html.js';Where is this iconset used?
import {getCss} from './zero_state_promo.css.js';This file should be named the same as the custom element.
zero_state_promo.css should refer only to a CSS file used by the top level zero_state_promo.html (which doesn't seem to exist).
private customHelpBubblehandler_: CustomHelpBubbleHandlerInterface =use consistent camelCase? i.e. customHelpBubbleHandler?
constructor() {
super();
}
remove trivial constructor.
// Open in active tab the user is on the New Tab Page.Can you make this comment a bit more clear? Is this saying that this UI is always shown with the NTP?
: public TopChromeWebUIController,Yiming ZhouNot the expert on this, but I thought Top Chrome WebUIs where generally UIs with a ".top-chrome" host. Is there a reason this is using TopChromeWebUIController and not just MojoWebUIController?
Yiming ZhouSwitched to MojoWebUiController.
Actually this class does need to extend TopChromeWebUI, as that is what the CustomWebUIHelpBubble class, which creates the webui-based IPH in https://chromium-review.googlesource.com/c/chromium/src/+/6404446, requires. Perhaps @dfr...@chromium.org can chime in, and on whether the association with a ".top-chrome" host is a hard requirement.
I had thought there was a way to add help bubbles to other WebUIs also - for example I thought settings used them (see e.g. this code: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/webui/settings/settings_ui.cc;l=772) and Settings is not a top chrome WebUI; it runs in its own renderer.
Agree this is a better question for dfried@.
Related details
- John Lee
- Kelvin Jiang
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiming Zhou (Gerrit)
Yiming Zhou added 8 comments![]( "Open in Gerrit")
nit: use a more descriptive name?
Done
#couponsButton .cr-icon {
--cr-icon-image: url(icons/coupons.svg);
}
#writingButton .cr-icon {
--cr-icon-image: url(icons/writing.svg);
}
#productivityButton .cr-icon {
--cr-icon-image: url(icons/productivity.svg);
}
#aiButton .cr-icon {
--cr-icon-image: url(icons/ai.svg);
}These selectors do not apply to any element in the app. Nothing has a CSS class of cr-icon.
Moreover, cr-icon instances do not require additional CSS variables to set their icons when using the icon property.
Beg pardon! forgot to remove them after switching over to icon-set.
<h3 id="title" class="col">$i18n{extensionsZeroStateIphHeader}</h3>where is this CSS class used? I don't see it in the CSS file.
Removed.
Where is this iconset used?
Removed.
This file should be named the same as the custom element.
zero_state_promo.css should refer only to a CSS file used by the top level zero_state_promo.html (which doesn't seem to exist).
Done
private customHelpBubblehandler_: CustomHelpBubbleHandlerInterface =use consistent camelCase? i.e. customHelpBubbleHandler?
Done
constructor() {
super();
}
Yiming Zhouremove trivial constructor.
Done
// Open in active tab the user is on the New Tab Page.Can you make this comment a bit more clear? Is this saying that this UI is always shown with the NTP?
Beg pardon, copied and pasted from another cl I was working on.
Related details
- John Lee
- Kelvin Jiang
- Rebekah Potter
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter added 1 comment![]( "Open in Gerrit")
: public TopChromeWebUIController,Yiming ZhouNot the expert on this, but I thought Top Chrome WebUIs where generally UIs with a ".top-chrome" host. Is there a reason this is using TopChromeWebUIController and not just MojoWebUIController?
Yiming ZhouSwitched to MojoWebUiController.
Rebekah PotterActually this class does need to extend TopChromeWebUI, as that is what the CustomWebUIHelpBubble class, which creates the webui-based IPH in https://chromium-review.googlesource.com/c/chromium/src/+/6404446, requires. Perhaps @dfr...@chromium.org can chime in, and on whether the association with a ".top-chrome" host is a hard requirement.
I had thought there was a way to add help bubbles to other WebUIs also - for example I thought settings used them (see e.g. this code: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/webui/settings/settings_ui.cc;l=772) and Settings is not a top chrome WebUI; it runs in its own renderer.
Agree this is a better question for dfried@.
Actually adding dfried@ to attention set for this question.
Related details
- Dana Fried
- John Lee
- Kelvin Jiang
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried added 1 comment![]( "Open in Gerrit")
: public TopChromeWebUIController,Yiming ZhouNot the expert on this, but I thought Top Chrome WebUIs where generally UIs with a ".top-chrome" host. Is there a reason this is using TopChromeWebUIController and not just MojoWebUIController?
Yiming ZhouSwitched to MojoWebUiController.
Rebekah PotterActually this class does need to extend TopChromeWebUI, as that is what the CustomWebUIHelpBubble class, which creates the webui-based IPH in https://chromium-review.googlesource.com/c/chromium/src/+/6404446, requires. Perhaps @dfr...@chromium.org can chime in, and on whether the association with a ".top-chrome" host is a hard requirement.
Rebekah PotterI had thought there was a way to add help bubbles to other WebUIs also - for example I thought settings used them (see e.g. this code: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/webui/settings/settings_ui.cc;l=772) and Settings is not a top chrome WebUI; it runs in its own renderer.
Agree this is a better question for dfried@.
Actually adding dfried@ to attention set for this question.
Yes, these are two entirely different things:
- A generic help bubble that can appear in a WebUI in a tab.
- A custom bubble that appears anchored to Top Chrome whose contents can be WebUI
The TopChromeWebUI approach is correct.
Related details
Rebekah Potter (Gerrit)
Rebekah Potter added 8 comments![]( "Open in Gerrit")
Mostly minor comments regarding the CSS. Other WebUI changes largely LGTM.
<path fill="#A8C7FA" d="M7 13.4c-.022 0-.033-.011-.033-.033 0-.878-.167-1.7-.5-2.467A6.244 6.244 0 0 0 3.1 7.533a6.119 6.119 0 0 0-2.467-.5C.611 7.033.6 7.023.6 7c0-.022.011-.033.033-.033A6.3 6.3 0 0 0 3.1 6.483a6.474 6.474 0 0 0 2.017-1.366A6.244 6.244 0 0 0 6.467 3.1c.333-.767.5-1.589.5-2.467C6.967.611 6.977.6 7 .6c.022 0 .033.011.033.033 0 .878.161 1.7.484 2.467a6.474 6.474 0 0 0 1.366 2.017A6.473 6.473 0 0 0 10.9 6.483a6.3 6.3 0 0 0 2.467.484c.022 0 .033.01.033.033 0 .022-.011.033-.033.033-.878 0-1.7.167-2.467.5a6.244 6.244 0 0 0-2.017 1.35A6.473 6.473 0 0 0 7.517 10.9a6.3 6.3 0 0 0-.484 2.467c0 .022-.01.033-.033.033Z"/>Most icons in iconsets don't use fill, and instead do any icon color adjustments using CSS variables. See example from cr-icon-button here, where it sets the colors for cr-icon with --iron-icon-fill/stroke-color: https://source.chromium.org/chromium/chromium/src/+/main:ui/webui/resources/cr_elements/cr_icon_button/cr_icon_button.css;l=115-116
#promoContainer {do we need this outer div or could all these styles be set on :host instead?
#headerContainer>* {
padding: 0px;
}does it work to set the 0 padding on headerContainer itself instead, or does #title or #dismissButton have some default padding you're trying to remove with this?
width: 240px;nit: Add a comment or explicitly calculate this from the 300px parent width - 2*20px padding - 20px icon?
width: 272px;this is a little weird since the parent of this element has width: 300px and then 20px padding, which implies 20px start/end, leaving 260px for content in the middle. Should we just let this be full width in the parent instead of specifying a width that looks like it will overflow?
static override get properties() {
return {};
}do we need to override this to empty? What breaks if we remove this?
private handler: PageHandlerRemote;nit (optional): other extensions code, including the other .ts file in this CL, uses a trailing "_" suffix for private properties. Let's be consistent?
Rebekah Potter (Gerrit)
Rebekah Potter added 1 comment![]( "Open in Gerrit")
: public TopChromeWebUIController,Yiming ZhouNot the expert on this, but I thought Top Chrome WebUIs where generally UIs with a ".top-chrome" host. Is there a reason this is using TopChromeWebUIController and not just MojoWebUIController?
Yiming ZhouSwitched to MojoWebUiController.
Rebekah PotterActually this class does need to extend TopChromeWebUI, as that is what the CustomWebUIHelpBubble class, which creates the webui-based IPH in https://chromium-review.googlesource.com/c/chromium/src/+/6404446, requires. Perhaps @dfr...@chromium.org can chime in, and on whether the association with a ".top-chrome" host is a hard requirement.
Rebekah PotterI had thought there was a way to add help bubbles to other WebUIs also - for example I thought settings used them (see e.g. this code: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/webui/settings/settings_ui.cc;l=772) and Settings is not a top chrome WebUI; it runs in its own renderer.
Agree this is a better question for dfried@.
Dana FriedActually adding dfried@ to attention set for this question.
Yes, these are two entirely different things:
- A generic help bubble that can appear in a WebUI in a tab.
- A custom bubble that appears anchored to Top Chrome whose contents can be WebUI
The TopChromeWebUI approach is correct.
I think the question is whether the latter approach is correct given that this particular UI is not necessarily Top Chrome. My understanding was that Top Chrome WebUIs are WebUI pages with ".top-chrome" in the URL, which have additional special properties/treatment and are not just any WebUI that doesn't appear in a tab. For example, Print Preview isn't a Top Chrome WebUI, despite appearing in a constrained dialog. This WebUI is appearing in a dialog but isn't using the "top-chrome" URL suffix, which makes it seem more like the Print Preview case on initial review.
However, this UI extends TopChromeWebUIController/TopChromeWebUIConfig, so it is going to be handled as a Top Chrome UI (for example, top Chrome configs are tracked in a special set: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/webui/top_chrome/top_chrome_webui_config.cc)
Do you know if it's okay for a WebUI page that isn't using ".top-chrome" to be treated as a Top Chrome UI? Is the current recommendation to promote any WebUI page that needs a help bubble and isn't in a tab to a Top Chrome UI? If so, should such UIs use the ".top-chrome" suffix?
Related details
- Dana Fried
Dana Fried (Gerrit)
Dana Fried added 1 comment![]( "Open in Gerrit")
: public TopChromeWebUIController,Yiming ZhouNot the expert on this, but I thought Top Chrome WebUIs where generally UIs with a ".top-chrome" host. Is there a reason this is using TopChromeWebUIController and not just MojoWebUIController?
Yiming ZhouSwitched to MojoWebUiController.
Rebekah PotterActually this class does need to extend TopChromeWebUI, as that is what the CustomWebUIHelpBubble class, which creates the webui-based IPH in https://chromium-review.googlesource.com/c/chromium/src/+/6404446, requires. Perhaps @dfr...@chromium.org can chime in, and on whether the association with a ".top-chrome" host is a hard requirement.
Rebekah PotterI had thought there was a way to add help bubbles to other WebUIs also - for example I thought settings used them (see e.g. this code: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/webui/settings/settings_ui.cc;l=772) and Settings is not a top chrome WebUI; it runs in its own renderer.
Agree this is a better question for dfried@.
Dana FriedActually adding dfried@ to attention set for this question.
Rebekah PotterYes, these are two entirely different things:
- A generic help bubble that can appear in a WebUI in a tab.
- A custom bubble that appears anchored to Top Chrome whose contents can be WebUI
The TopChromeWebUI approach is correct.
I think the question is whether the latter approach is correct given that this particular UI is not necessarily Top Chrome. My understanding was that Top Chrome WebUIs are WebUI pages with ".top-chrome" in the URL, which have additional special properties/treatment and are not just any WebUI that doesn't appear in a tab. For example, Print Preview isn't a Top Chrome WebUI, despite appearing in a constrained dialog. This WebUI is appearing in a dialog but isn't using the "top-chrome" URL suffix, which makes it seem more like the Print Preview case on initial review.
However, this UI extends TopChromeWebUIController/TopChromeWebUIConfig, so it is going to be handled as a Top Chrome UI (for example, top Chrome configs are tracked in a special set: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/webui/top_chrome/top_chrome_webui_config.cc)
Do you know if it's okay for a WebUI page that isn't using ".top-chrome" to be treated as a Top Chrome UI? Is the current recommendation to promote any WebUI page that needs a help bubble and isn't in a tab to a Top Chrome UI? If so, should such UIs use the ".top-chrome" suffix?
To be fair, it was basically the only way to easily make it work. The Top Chrome WebUI is designed for relatively lightweight dialogs, which these are. And they do appear in Top Chrome. However, you do have a point. I think the question would be more one of authorial intent. I'll reach out to Keren Zhu about it.
I've pinged him on DMs and will see what he says.
Related details
Keren Zhu (Gerrit)
Keren Zhu added 2 comments![]( "Open in Gerrit")
Adding myself to the reviewer list per Dana's request. Reviwer
It should be ok to use TopChromeWebUIController in this case.
A bit of background: TopChromeWebUIController used to be called "MojoBubbleWebUIController". It was wrong name because this controller is also used by side panel WebUIs, so I renamed it in [crbug.com/329677669](https://crbug.com/329677669).
".top-chrome" and TopChromeController can be used independently. For example, the lens overlay [uses](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/lens/lens_overlay_untrusted_ui.cc;l=41;drc=4341cc4e529444bd201ad3aeeed0ec561e04103f) UntrustedTopChromeWebUIController, but its [URL](https://source.chromium.org/chromium/chromium/src/+/main:chrome/common/webui_url_constants.h;l=350;drc=1e17874cde2d4733a808080d203f6271ce8783fd) does not have the ".top-chrome" suffix.
Using ".top-chrome" has a few side-effects. For example, ".top-chrome" WebUIs share a single process. On mac, a click event on a ".top-chrome" WebUI in an inactive window will reach the content, whereas a non-topchrome one will require an additional click.
Using TopChromeWebUIController has another set of side-effects, e.g., it can opt in for preloading, it will appear on the `WebUI.TopChrome.RequestToFCP.{WebUIName}` histograms.
Ideally, I think we should consolidate these 2 concepts. For now, documentation is probably more important. I filed [crbug.com/415076949](https://crbug.com/415076949) for documentation.
Kelvin Jiang (Gerrit)
Kelvin Jiang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kelvin Jiang
| Code-Review | +1 |
1 comment
- John Lee
- Yiming Zhou
Yiming Zhou (Gerrit)
Yiming Zhou added 8 comments![]( "Open in Gerrit")
<path fill="#A8C7FA" d="M7 13.4c-.022 0-.033-.011-.033-.033 0-.878-.167-1.7-.5-2.467A6.244 6.244 0 0 0 3.1 7.533a6.119 6.119 0 0 0-2.467-.5C.611 7.033.6 7.023.6 7c0-.022.011-.033.033-.033A6.3 6.3 0 0 0 3.1 6.483a6.474 6.474 0 0 0 2.017-1.366A6.244 6.244 0 0 0 6.467 3.1c.333-.767.5-1.589.5-2.467C6.967.611 6.977.6 7 .6c.022 0 .033.011.033.033 0 .878.161 1.7.484 2.467a6.474 6.474 0 0 0 1.366 2.017A6.473 6.473 0 0 0 10.9 6.483a6.3 6.3 0 0 0 2.467.484c.022 0 .033.01.033.033 0 .022-.011.033-.033.033-.878 0-1.7.167-2.467.5a6.244 6.244 0 0 0-2.017 1.35A6.473 6.473 0 0 0 7.517 10.9a6.3 6.3 0 0 0-.484 2.467c0 .022-.01.033-.033.033Z"/>Most icons in iconsets don't use fill, and instead do any icon color adjustments using CSS variables. See example from cr-icon-button here, where it sets the colors for cr-icon with --iron-icon-fill/stroke-color: https://source.chromium.org/chromium/chromium/src/+/main:ui/webui/resources/cr_elements/cr_icon_button/cr_icon_button.css;l=115-116
Done
do we need this outer div or could all these styles be set on :host instead?
Setting it on :host works.
does it work to set the 0 padding on headerContainer itself instead, or does #title or #dismissButton have some default padding you're trying to remove with this?
Done
nit: Add a comment or explicitly calculate this from the 300px parent width - 2*20px padding - 20px icon?
Done
this is a little weird since the parent of this element has width: 300px and then 20px padding, which implies 20px start/end, leaving 260px for content in the middle. Should we just let this be full width in the parent instead of specifying a width that looks like it will overflow?
Removed hard-coded width.
static override get properties() {
return {};
}do we need to override this to empty? What breaks if we remove this?
Done
nit (optional): other extensions code, including the other .ts file in this CL, uses a trailing "_" suffix for private properties. Let's be consistent?
Done
: public TopChromeWebUIController,- Dana Fried
- John Lee
- Kelvin Jiang
- Keren Zhu
- Rebekah Potter
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter voted and added 5 comments![]( "Open in Gerrit")
Votes added by Rebekah Potter
| Code-Review | +1 |
5 comments
LGTM with comments.
Also, high level question, can you confirm there is a plan to add some basic WebUI automated tests for this in chrome/test/data/webui in a followup CL?
# Copyright 2025 The Chromium AuthorsDoes this folder need a dedicated OWNERS file or is it considered to be owned by the folks in chrome/browser/resources/extensions/OWNERS? Same question for the corresponding chrome/browser/ui/webui/ folder.
New WebUIs normally are required to have an owning team, so if that isn't the chrome://extensions owners, we should add OWNERS files.
--cr-icon-button-size: 20px;use dismiss-button-icon-size here too?
<h3 id="title">$i18n{extensionsZeroStateIphHeader}</h3>
<cr-icon-button id="dismissButton"
class="icon-clear"
title="$i18n{extensionsZeroStateIphDismissButtonTitle}"
@click="${this.onDismissButtonClick_}">
</cr-icon-button>needs -2 indent
return 'extensions-zero-state-promo';nit: tag name should normally be either class name, converted to dash case with the "Element" bit removed, or the same thing + a prefix (so in this case either extensions-zero-state-promo-app or just zero-state-promo-app)
Related details
- Dana Fried
- John Lee
- Kelvin Jiang
- Keren Zhu
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter added 1 comment![]( "Open in Gerrit")
kerenzhu@ thanks for the detailed explanation. It has indeed not been clear to me at least what the difference was between ".top-chrome" and TopChromeWebUIController, and had assumed that these two concepts were the same, rather than independent things where any given UI can be one, both, or neither. Thanks for the documentation bug.
John Lee (Gerrit)
John Lee voted and added 3 comments![]( "Open in Gerrit")
Votes added by John Lee
| Code-Review | +1 |
3 comments
<cr-iconset name="web-store-links" size="14">`zero-state-promo`? These icons don't all seem specific to the web store.
width: calc(100% - var(--top-container-padding) * 2 - var(dismiss-button-icon-size));`var(--dismiss`
I don't think you actually need this width. All you might need is a `flex-shrink: 0` on the #dismissButton so that the button never shrinks.
--color-chip-icon: #A8C7FA;Does this hardcoded color work for all themes?
Related details
- Dana Fried
- Kelvin Jiang
- Keren Zhu
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kelvin Jiang (Gerrit)
Kelvin Jiang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kelvin Jiang
| Code-Review | +1 |
1 comment
- Dana Fried
- Keren Zhu
- Yiming Zhou
Dana Fried (Gerrit)
Dana Fried voted and added 3 comments![]( "Open in Gerrit")
Votes added by Dana Fried
| Code-Review | +1 |
3 comments
LGTM modulo comments
# Copyright 2025 The Chromium AuthorsDoes this folder need a dedicated OWNERS file or is it considered to be owned by the folks in chrome/browser/resources/extensions/OWNERS? Same question for the corresponding chrome/browser/ui/webui/ folder.
New WebUIs normally are required to have an owning team, so if that isn't the chrome://extensions owners, we should add OWNERS files.
+1
--color-chip-icon: #A8C7FA;Does this hardcoded color work for all themes?
+1, I would not hard-code a color in this situation; ideally you would use only theme-generated colors
Even if this does visually "work" for all themes it may not end up being accessible for all themes.
Related details
- Keren Zhu
- Yiming Zhou
Yiming Zhou (Gerrit)
Yiming Zhou added 9 comments![]( "Open in Gerrit")
I am awaiting resolution of Rebekah's feedback, as she's more of an expert in this area. Feel free to tag me in any questions/threads if you want my input or don't know how to resolve an issue (or DM me)
Acknowledged
LGTM with comments.
Also, high level question, can you confirm there is a plan to add some basic WebUI automated tests for this in chrome/test/data/webui in a followup CL?
Added tests within this cl.
Dana FriedDoes this folder need a dedicated OWNERS file or is it considered to be owned by the folks in chrome/browser/resources/extensions/OWNERS? Same question for the corresponding chrome/browser/ui/webui/ folder.
New WebUIs normally are required to have an owning team, so if that isn't the chrome://extensions owners, we should add OWNERS files.
+1
Done
`zero-state-promo`? These icons don't all seem specific to the web store.
Done
width: calc(100% - var(--top-container-padding) * 2 - var(dismiss-button-icon-size));`var(--dismiss`
I don't think you actually need this width. All you might need is a `flex-shrink: 0` on the #dismissButton so that the button never shrinks.
Done
--cr-icon-button-size: 20px;Yiming Zhouuse dismiss-button-icon-size here too?
Done
Dana FriedDoes this hardcoded color work for all themes?
+1, I would not hard-code a color in this situation; ideally you would use only theme-generated colors
Even if this does visually "work" for all themes it may not end up being accessible for all themes.
Done
<h3 id="title">$i18n{extensionsZeroStateIphHeader}</h3>
<cr-icon-button id="dismissButton"
class="icon-clear"
title="$i18n{extensionsZeroStateIphDismissButtonTitle}"
@click="${this.onDismissButtonClick_}">
</cr-icon-button>Yiming Zhouneeds -2 indent
Done
nit: tag name should normally be either class name, converted to dash case with the "Element" bit removed, or the same thing + a prefix (so in this case either extensions-zero-state-promo-app or just zero-state-promo-app)
- Dana Fried
- John Lee
- Kelvin Jiang
- Keren Zhu
- Rebekah Potter
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dana Fried
| Code-Review | +1 |
- John Lee
- Kelvin Jiang
- Keren Zhu
- Rebekah Potter
- Yiming Zhou
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter added 7 comments![]( "Open in Gerrit")
uwyi...@chromium.orgCan you add at least 1 more Chromium committer here?
Also, if this UI is completely separate from chrome://extensions with separate ownership, should this be in its own chrome/browser/resources/ folder instead of nested under extensions/? In general, a WebUI chrome://foo should have its resources under chrome/browser/resources/foo. The only exception is collections of related WebUIs that are sharing some code, e.g. side_panel.
--cr-icon-button-size: var(--dismiss-button-icon-size);can just remove this var now, since it's only used here.
"//third_party/lit/v3_0:build_ts",
"//ui/webui/resources/cr_elements:build_ts",
"//ui/webui/resources/js:build_ts",These tests are not importing anything from chrome://resources, so they shouldn't need any of these ts_deps.
import {TestMock} from 'chrome://webui-test/test_mock.js';TestBrowserProxy should be used instead of TestMock for stubbing out browser proxies. It has much stronger typing.
return microtasksFinished();Why is this necessary? CrLitElement forces a synchronous render when the element is attached. Is this element doing something that requires multiple render cycles?
await microtasksFinished();This should be using the "whenCalled()" mechanism from TestBrowserProxy, since it is trying to validate that a proxy call has happened, not that something has changed in the UI. i.e.:
```
const args = await promoProxy.whenCalled('openUrl');
```
`await microtasksFinished()` should only be used in cases where something happens that updates a Lit reactive property, and we need to validate that something changed in the DOM as a result. Since Lit rendering is async, we need to wait 1 render cycle for the change to the property to propagate to the DOM. The element you are testing here has no Lit reactive properties, so this should probably never be needed.
Using it for other waiting patterns risks creating flaky tests, because we are not waiting for the right thing, and instead just arbitrarily waiting for a setTimeout of 0.
IN_PROC_BROWSER_TEST_F(ZeroStatePromoChipsUiTest, ClickCouponsChip) {
RunTestCase("ClickCouponsChip");
}
IN_PROC_BROWSER_TEST_F(ZeroStatePromoChipsUiTest, ClickWritingChip) {
RunTestCase("ClickWritingChip");
}
IN_PROC_BROWSER_TEST_F(ZeroStatePromoChipsUiTest, ClickProductivityChip) {
RunTestCase("ClickProductivityChip");
}
IN_PROC_BROWSER_TEST_F(ZeroStatePromoChipsUiTest, ClickAiChip) {
RunTestCase("ClickAiChip");
}FYI, unless you expect this suite to become sufficiently large/long that it would be at risk of timing out on slower trybots, it's probably easier to just run the whole suite with 'mocha.run()' instead of introducing the RunTestCase helper and running the cases individually.
I know a few existing WebUIs do this, but they are generally older UIs. The infrastructure has improved quite since those were added so that you can now see in test results which test case specifically has failed within a mocha suite without needing to actually run all the tests separately.
Related details
- John Lee
- Kelvin Jiang
- Keren Zhu
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiming Zhou (Gerrit)
Yiming Zhou added 7 comments![]( "Open in Gerrit")
Can you add at least 1 more Chromium committer here?
Also, if this UI is completely separate from chrome://extensions with separate ownership, should this be in its own chrome/browser/resources/ folder instead of nested under extensions/? In general, a WebUI chrome://foo should have its resources under chrome/browser/resources/foo. The only exception is collections of related WebUIs that are sharing some code, e.g. side_panel.
Refactored code to new location.
Also fixed my own email to the Google.com one, which is what I am using in this cl chain.
--cr-icon-button-size: var(--dismiss-button-icon-size);can just remove this var now, since it's only used here.
Done
"//third_party/lit/v3_0:build_ts",
"//ui/webui/resources/cr_elements:build_ts",
"//ui/webui/resources/js:build_ts",These tests are not importing anything from chrome://resources, so they shouldn't need any of these ts_deps.
Done
import {TestMock} from 'chrome://webui-test/test_mock.js';TestBrowserProxy should be used instead of TestMock for stubbing out browser proxies. It has much stronger typing.
Thanks, this removes the barrier for me to add a few more test conditions.
Why is this necessary? CrLitElement forces a synchronous render when the element is attached. Is this element doing something that requires multiple render cycles?
Removed.
This should be using the "whenCalled()" mechanism from TestBrowserProxy, since it is trying to validate that a proxy call has happened, not that something has changed in the UI. i.e.:
```
const args = await promoProxy.whenCalled('openUrl');
````await microtasksFinished()` should only be used in cases where something happens that updates a Lit reactive property, and we need to validate that something changed in the DOM as a result. Since Lit rendering is async, we need to wait 1 render cycle for the change to the property to propagate to the DOM. The element you are testing here has no Lit reactive properties, so this should probably never be needed.
Using it for other waiting patterns risks creating flaky tests, because we are not waiting for the right thing, and instead just arbitrarily waiting for a setTimeout of 0.
Done
IN_PROC_BROWSER_TEST_F(ZeroStatePromoChipsUiTest, ClickCouponsChip) {
RunTestCase("ClickCouponsChip");
}
IN_PROC_BROWSER_TEST_F(ZeroStatePromoChipsUiTest, ClickWritingChip) {
RunTestCase("ClickWritingChip");
}
IN_PROC_BROWSER_TEST_F(ZeroStatePromoChipsUiTest, ClickProductivityChip) {
RunTestCase("ClickProductivityChip");
}
IN_PROC_BROWSER_TEST_F(ZeroStatePromoChipsUiTest, ClickAiChip) {
RunTestCase("ClickAiChip");
}FYI, unless you expect this suite to become sufficiently large/long that it would be at risk of timing out on slower trybots, it's probably easier to just run the whole suite with 'mocha.run()' instead of introducing the RunTestCase helper and running the cases individually.
I know a few existing WebUIs do this, but they are generally older UIs. The infrastructure has improved quite since those were added so that you can now see in test results which test case specifically has failed within a mocha suite without needing to actually run all the tests separately.
- Dana Fried
- John Lee
- Kelvin Jiang
- Keren Zhu
- Rebekah Potter
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried added 1 comment![]( "Open in Gerrit")
Can you add at least 1 more Chromium committer here?
Also, if this UI is completely separate from chrome://extensions with separate ownership, should this be in its own chrome/browser/resources/ folder instead of nested under extensions/? In general, a WebUI chrome://foo should have its resources under chrome/browser/resources/foo. The only exception is collections of related WebUIs that are sharing some code, e.g. side_panel.
Feels like. I am also happy to be an owner if we're talking about only promotional surfaces.
Related details
- John Lee
- Kelvin Jiang
- Keren Zhu
- Rebekah Potter
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiming Zhou (Gerrit)
Yiming Zhou voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yiming Zhou
| Commit-Queue | +1 |
1 comment
Dana FriedCan you add at least 1 more Chromium committer here?
Also, if this UI is completely separate from chrome://extensions with separate ownership, should this be in its own chrome/browser/resources/ folder instead of nested under extensions/? In general, a WebUI chrome://foo should have its resources under chrome/browser/resources/foo. The only exception is collections of related WebUIs that are sharing some code, e.g. side_panel.
Feels like. I am also happy to be an owner if we're talking about only promotional surfaces.
- Chromium IPC Reviews
- Dana Fried
- John Lee
- Keren Zhu
- Rebekah Potter
- Yiming Zhou
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: tse...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): tse...@chromium.org
Reviewer source(s):
tse...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Dana Fried
- John Lee
- Keren Zhu
- Rebekah Potter
- Tom Sepez
- Yiming Zhou
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter added 5 comments![]( "Open in Gerrit")
#include "chrome/browser/ui/webui/extensions/web_store/zero_state_promo_ui.h"remove? shouldn't need 2 includes of this file.
Should have additional OWNERS, the same way the chrome/browser/resources folder does.
Also, should we move this C++ code to its own directory also, if it's not closely related to the chrome://extensions UI and isn't intended to be owned by the same team?
sources += [ "extensions_zero_state/zero_state_promo_browsertest.cc" ]This source should still be in the browser_tests target, not interactive_ui_tests. It is extending WebUIMochaBrowserTest and not WebUIMochaFocusTest.
Also, since the prod code is included whenever enable_extensions_core is true, the tests should also be run on Android, and shouldn't be exclusive to Win/Mac/Linux/CrOS.
Code behind enable_extensions_core, IIUC, is supposed to work on some Android builds. If that's not what you want, maybe you want enable_extensions instead?
export class TestBubbleProxy extends TestBrowserProxy implementsThis one probably doesn't need to extend TestBrowserProxy, since it's not making use of methodCalled or passing anything to super(). This can just be a class that implements CustomHelpBubbleProxy.
#include "base/strings/stringprintf.h"unused?
Related details
- Dana Fried
- John Lee
- Keren Zhu
- Tom Sepez
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
Dana Fried (Gerrit)
Dana Fried added 3 comments![]( "Open in Gerrit")
Overall I think we are approaching a submittable CL. I will try to stay on top of this over the next few days.
Should have additional OWNERS, the same way the chrome/browser/resources folder does.
Also, should we move this C++ code to its own directory also, if it's not closely related to the chrome://extensions UI and isn't intended to be owned by the same team?
+1 to putting this into its own subfolder; also I do volunteer to be an owner if there is a separate promo folder with separate OWNERS (separate OWNERS not required unless Rebekah says).
sources += [ "extensions_zero_state/zero_state_promo_browsertest.cc" ]This source should still be in the browser_tests target, not interactive_ui_tests. It is extending WebUIMochaBrowserTest and not WebUIMochaFocusTest.
Also, since the prod code is included whenever enable_extensions_core is true, the tests should also be run on Android, and shouldn't be exclusive to Win/Mac/Linux/CrOS.
Code behind enable_extensions_core, IIUC, is supposed to work on some Android builds. If that's not what you want, maybe you want enable_extensions instead?
The naming conventions here are off, unfortunatley.
All browser_tests should be in files of the form: `XXXX_browsertest.cc` and have tests named `XXXXBrowserTest`.
All interactive_ui_tests should be in files of the form: `XXX_interactive_uitest.cc` and have tests named `XXXUiTest`.
That said, aside from naming convention, browser_tests and interactive_ui_tests admit the same types of tests and the primary decider for whether something should be in browser_tests or interactive_ui_tests is that a test should be in the latter if (and only if) it does one of the following:
- cares about focus or activation
- tries to control the mouse
- tries to send accelerators
- invokes surfaces like menus and dialogs that close on loss of focus
These require the test process isolation guarantees provided by interactive_ui_tests but not by browser_tests.
Related details
Rebekah Potter (Gerrit)
Rebekah Potter added 1 comment![]( "Open in Gerrit")
sources += [ "extensions_zero_state/zero_state_promo_browsertest.cc" ]Dana FriedThis source should still be in the browser_tests target, not interactive_ui_tests. It is extending WebUIMochaBrowserTest and not WebUIMochaFocusTest.
Also, since the prod code is included whenever enable_extensions_core is true, the tests should also be run on Android, and shouldn't be exclusive to Win/Mac/Linux/CrOS.
Code behind enable_extensions_core, IIUC, is supposed to work on some Android builds. If that's not what you want, maybe you want enable_extensions instead?
The naming conventions here are off, unfortunatley.
All browser_tests should be in files of the form: `XXXX_browsertest.cc` and have tests named `XXXXBrowserTest`.
All interactive_ui_tests should be in files of the form: `XXX_interactive_uitest.cc` and have tests named `XXXUiTest`.
That said, aside from naming convention, browser_tests and interactive_ui_tests admit the same types of tests and the primary decider for whether something should be in browser_tests or interactive_ui_tests is that a test should be in the latter if (and only if) it does one of the following:
- cares about focus or activation
- tries to control the mouse
- tries to send accelerators
- invokes surfaces like menus and dialogs that close on loss of focus
These require the test process isolation guarantees provided by interactive_ui_tests but not by browser_tests.
For WebUI tests, WebUIMochaFocusTest (and as a result, interactive_ui_tests) is used when the WebContents need to have consistent focus, and usually this is the first case you listed (the test cares about focus). For this to work correctly the test needs to *both* be in the interactive_ui_tests target and use WebUIMochaFocusTest instead of WebUIMochaBrowserTest as the base class.
This test doesn't seem to care about focus, and is also still extending WebUIMochaBrowserTest, which is an indicator that it probably should still be in the browser_tests target.
If it cares about focus, it needs to also extend WebUIMochaFocusTest, not just move to the interactive_ui_tests target.
Yiming Zhou (Gerrit)
Yiming Zhou added 3 comments![]( "Open in Gerrit")
#include "chrome/browser/ui/webui/extensions/web_store/zero_state_promo_ui.h"remove? shouldn't need 2 includes of this file.
Done
export class TestBubbleProxy extends TestBrowserProxy implementsThis one probably doesn't need to extend TestBrowserProxy, since it's not making use of methodCalled or passing anything to super(). This can just be a class that implements CustomHelpBubbleProxy.
Done
#include "base/strings/stringprintf.h"Yiming Zhouunused?
Done
Related details
- Chromium IPC Reviews
- John Lee
- Keren Zhu
- Rebekah Potter
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: w...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): w...@chromium.org
Reviewer source(s):
w...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- John Lee
- Keren Zhu
- Rebekah Potter
- Will Harris
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris added 2 comments![]( "Open in Gerrit")
can you link a design or some kind of description of what this is, to the bug? thanks.
#include "ui/webui/resources/cr_components/help_bubble/custom_help_bubble.mojom.h"is this change intended?
Related details
- John Lee
- Keren Zhu
- Rebekah Potter
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiming Zhou (Gerrit)
Yiming Zhou voted and added 3 comments![]( "Open in Gerrit")
Votes added by Yiming Zhou
| Commit-Queue | +1 |
3 comments
#include "ui/webui/resources/cr_components/help_bubble/custom_help_bubble.mojom.h"Yiming Zhouis this change intended?
Done
Dana FriedShould have additional OWNERS, the same way the chrome/browser/resources folder does.
Also, should we move this C++ code to its own directory also, if it's not closely related to the chrome://extensions UI and isn't intended to be owned by the same team?
+1 to putting this into its own subfolder; also I do volunteer to be an owner if there is a separate promo folder with separate OWNERS (separate OWNERS not required unless Rebekah says).
Done
sources += [ "extensions_zero_state/zero_state_promo_browsertest.cc" ]This source should still be in the browser_tests target, not interactive_ui_tests. It is extending WebUIMochaBrowserTest and not WebUIMochaFocusTest.
Also, since the prod code is included whenever enable_extensions_core is true, the tests should also be run on Android, and shouldn't be exclusive to Win/Mac/Linux/CrOS.
Code behind enable_extensions_core, IIUC, is supposed to work on some Android builds. If that's not what you want, maybe you want enable_extensions instead?
Rebekah PotterThe naming conventions here are off, unfortunatley.
All browser_tests should be in files of the form: `XXXX_browsertest.cc` and have tests named `XXXXBrowserTest`.
All interactive_ui_tests should be in files of the form: `XXX_interactive_uitest.cc` and have tests named `XXXUiTest`.
That said, aside from naming convention, browser_tests and interactive_ui_tests admit the same types of tests and the primary decider for whether something should be in browser_tests or interactive_ui_tests is that a test should be in the latter if (and only if) it does one of the following:
- cares about focus or activation
- tries to control the mouse
- tries to send accelerators
- invokes surfaces like menus and dialogs that close on loss of focus
These require the test process isolation guarantees provided by interactive_ui_tests but not by browser_tests.
For WebUI tests, WebUIMochaFocusTest (and as a result, interactive_ui_tests) is used when the WebContents need to have consistent focus, and usually this is the first case you listed (the test cares about focus). For this to work correctly the test needs to *both* be in the interactive_ui_tests target and use WebUIMochaFocusTest instead of WebUIMochaBrowserTest as the base class.
This test doesn't seem to care about focus, and is also still extending WebUIMochaBrowserTest, which is an indicator that it probably should still be in the browser_tests target.
If it cares about focus, it needs to also extend WebUIMochaFocusTest, not just move to the interactive_ui_tests target.
- Dana Fried
- John Lee
- Keren Zhu
- Rebekah Potter
- Will Harris
- Yiming Zhou
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris added 1 comment![]( "Open in Gerrit")
can you link a design or some kind of description of what this is, to the bug? thanks.
marking unresolved to highlight this question. I cannot review without knowing what this CL does or what it's used for.
Related details
- Dana Fried
- John Lee
- Keren Zhu
- Rebekah Potter
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiming Zhou (Gerrit)
Yiming Zhou added 1 comment![]( "Open in Gerrit")
Will Harriscan you link a design or some kind of description of what this is, to the bug? thanks.
marking unresolved to highlight this question. I cannot review without knowing what this CL does or what it's used for.
Thanks for taking a look, Will!
Essentially, this chain of cls is trying to bring up a custom In-Product-Help message, encouraging users with no previous experience with extensions to try them out by exploring the Chrome Web Store. Here is a screencast of what the final product should look like: https://screencast.googleplex.com/cast/NjY1NjM2MjkzODM2ODAwMHwyOGQ5MTNjZC03OA
PRD: https://docs.google.com/document/d/1YJfV1tbe9tbOqgyZUyAdV79dJ2nbTdF0buMx2xpX8Ms/edit?tab=t.0
Partial Design Doc: go/extension-discovery-experiments-on-chrome-desktop-design
Related details
- Dana Fried
- John Lee
- Keren Zhu
- Rebekah Potter
- Will Harris
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris added 2 comments![]( "Open in Gerrit")
Will Harriscan you link a design or some kind of description of what this is, to the bug? thanks.
Yiming Zhoumarking unresolved to highlight this question. I cannot review without knowing what this CL does or what it's used for.
Thanks for taking a look, Will!
Essentially, this chain of cls is trying to bring up a custom In-Product-Help message, encouraging users with no previous experience with extensions to try them out by exploring the Chrome Web Store. Here is a screencast of what the final product should look like: https://screencast.googleplex.com/cast/NjY1NjM2MjkzODM2ODAwMHwyOGQ5MTNjZC03OA
PRD: https://docs.google.com/document/d/1YJfV1tbe9tbOqgyZUyAdV79dJ2nbTdF0buMx2xpX8Ms/edit?tab=t.0
Partial Design Doc: go/extension-discovery-experiments-on-chrome-desktop-design
Thanks. Please add these links into the bug so they are permanently tied to the chain of CLs and anyone can reference them in future.
OpenURL(url.mojom.Url url);do you need to have this IPC in the browser?
Is it not possible for the webui to just call via JS to e.g. `window.open`?
Related details
- Dana Fried
- John Lee
- Keren Zhu
- Rebekah Potter
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiming Zhou (Gerrit)
Yiming Zhou added 2 comments![]( "Open in Gerrit")
Will Harriscan you link a design or some kind of description of what this is, to the bug? thanks.
Yiming Zhoumarking unresolved to highlight this question. I cannot review without knowing what this CL does or what it's used for.
Will HarrisThanks for taking a look, Will!
Essentially, this chain of cls is trying to bring up a custom In-Product-Help message, encouraging users with no previous experience with extensions to try them out by exploring the Chrome Web Store. Here is a screencast of what the final product should look like: https://screencast.googleplex.com/cast/NjY1NjM2MjkzODM2ODAwMHwyOGQ5MTNjZC03OA
PRD: https://docs.google.com/document/d/1YJfV1tbe9tbOqgyZUyAdV79dJ2nbTdF0buMx2xpX8Ms/edit?tab=t.0
Partial Design Doc: go/extension-discovery-experiments-on-chrome-desktop-design
Thanks. Please add these links into the bug so they are permanently tied to the chain of CLs and anyone can reference them in future.
Done
do you need to have this IPC in the browser?
Is it not possible for the webui to just call via JS to e.g. `window.open`?
I do, because the webui will be hosted inside an CustomUiHelpBubble, which renders outside of a Chrome tab.
Related details
- Dana Fried
- John Lee
- Keren Zhu
- Rebekah Potter
- Will Harris
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 2 comments![]( "Open in Gerrit")
OpenURL(url.mojom.Url url);Yiming Zhoudo you need to have this IPC in the browser?
Is it not possible for the webui to just call via JS to e.g. `window.open`?
I do, because the webui will be hosted inside an CustomUiHelpBubble, which renders outside of a Chrome tab.
OK, but is there a reason `window.open()` doesn't work? Is it explicitly disabled, or is the functionality in `WebContentsDelegate` [1][2] overridden somehow? If we really need to duplicate web platform functionality in MojoJS, we should try to understand why it's broken first.
[1] window.open() is ultimately handled here in the browser side: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/web_contents/web_contents_impl.cc;l=5230;drc=4a1f2646209c5ecc0aae120e3654feb6d3f393f3
[2] which is delegated to the embedder with https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/web_contents_delegate.cc;l=47;drc=4a1f2646209c5ecc0aae120e3654feb6d3f393f3
Related details
- Dana Fried
- John Lee
- Keren Zhu
- Rebekah Potter
- Will Harris
- Yiming Zhou
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried added 1 comment![]( "Open in Gerrit")
The test that's coming up flaky here isn't even added in this CL. However, attempting to activate the browser on Linux is a known issue; I think your `BrowserActivationWater` is spurious and should probably be removed, especially considering your promo gets hammered immediately and will take focus. This is almost certainly the cause of the flake.
Related details
- John Lee
- Keren Zhu
- Rebekah Potter
- Will Harris
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
Rebekah Potter (Gerrit)
Rebekah Potter added 14 comments![]( "Open in Gerrit")
<message name="IDS_EXTENSIONS_ZERO_STATE_IPH_HEADER" desc="Header of the extensions zero state in-product help promo.">
Supercharge your browser with extensions and themes
</message>
<message name="IDS_EXTENSIONS_ZERO_STATE_CHIPS_IPH_DESCRIPTION" desc="An one line description for the extensions zero state in-product-help promo.">
Extensions are add-ons for Chrome that can help you do more
</message>
<message name="IDS_EXTENSIONS_ZERO_STATE_IPH_SHOPPING_CATEGORY_LABEL" desc="Label for the button that opens the Chrome Web Store shopping category page.">
Find coupons
</message>
<message name="IDS_EXTENSIONS_ZERO_STATE_IPH_WRITING_HELP_COLLECTION_LABEL" desc="Label for the button that opens the Chrome Web Store writing help collection page.">
Help me write
</message>
<message name="IDS_EXTENSIONS_ZERO_STATE_IPH_PRODUCTIVITY_CATEGORY_LABEL" desc="Label for the button that opens the Chrome Web Store producitivty category page.">
Boost productivity
</message>
<message name="IDS_EXTENSIONS_ZERO_STATE_IPH_AI_PRODUCTIVITY_COLLECTION_LABEL" desc="Label for the button that opens the Chrome Web Store ai productivity collection page.">
Enhance with AI
</message>
<message name="IDS_EXTENSIONS_ZERO_STATE_IPH_DISMISS_BUTTON_TITLE" desc="Title attribute for the button that dismisses the extensions zero state in-product-help promo.">
Close
</message>should we put these behind a "enable_extensions" flag, such that they are not unnecessarily included on builds where they are unused?
"extensions zero state promo should not be enabeld for android")nit: spelling (here and below)
deps += [
"//chrome/browser/ui/webui/extensions_zero_state_promo:mojo_bindings",
]Remove - this dep is now added twice, and seems to belong only on line 5360, under the same condition as the other source code that uses it.
#if BUILDFLAG(ENABLE_EXTENSIONS_CORE) && \
(BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX) || \
BUILDFLAG(IS_CHROMEOS))
#include "chrome/browser/ui/webui/extensions_zero_state_promo/zero_state_promo_ui.h"
#endif // BUILDFLAG(ENABLE_EXTENSIONS_CORE) && (BUILDFLAG(IS_WIN) ||
// BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS))Condition for this #if should match the one on line 351, i.e. let's use ENABLE_EXTENSIONS or
ENABLE_EXTENSIONS_CORE && (<long list of OSes>)
in both spots.
Dana FriedShould have additional OWNERS, the same way the chrome/browser/resources folder does.
Also, should we move this C++ code to its own directory also, if it's not closely related to the chrome://extensions UI and isn't intended to be owned by the same team?
Yiming Zhou+1 to putting this into its own subfolder; also I do volunteer to be an owner if there is a separate promo folder with separate OWNERS (separate OWNERS not required unless Rebekah says).
Done
Re-opening as this has been moved to a subfolder, but I don't see any OWNERS specified other than for the mojom file, which means this code is proposed to be de-facto owned by the WebUI team. We request that teams adding new WebUIs commit to maintaining their UIs, which is why we also ask for OWNERS from those teams to be added.
#include "base/memory/raw_ptr.h"is raw_ptr needed? I don't see this used in this file but may be missing something.
#include "chrome/browser/ui/chrome_pages.h"
#include "chrome/grit/generated_resources.h"
#include "components/prefs/pref_service.h"
#include "components/profile_metrics/browser_profile_type.h"
#include "components/strings/grit/components_strings.h"unused?
#include "content/public/browser/web_contents.h"
#include "content/public/browser/web_ui.h"also unused?
#include "ui/base/l10n/l10n_util.h"unused?
#include "chrome/browser/ui/ui_features.h"unused?
#include "chrome/grit/branded_strings.h"I don't think we have any branded strings here?
#include "components/strings/grit/components_strings.h"The strings for this UI are in the chrome/grit/generated_resources.h header AFAICT, not in components/
#include "ui/accessibility/accessibility_features.h"
#include "ui/base/ui_base_features.h"Do we need all these feature includes?
#include "ui/webui/resources/grit/webui_resources.h"
#include "ui/webui/resources/grit/webui_resources_map.h"What resources from the shared resources are being used in this file? Normally WebUIs should get shared resources from chrome://resources, and not need to manually register them with their individual data source.
Related details
- John Lee
- Keren Zhu
- Will Harris
- Yiming Zhou
Will Harris (Gerrit)
Will Harris added 1 comment![]( "Open in Gerrit")
OpenURL(url.mojom.Url url);Yiming Zhoudo you need to have this IPC in the browser?
Is it not possible for the webui to just call via JS to e.g. `window.open`?
Daniel ChengI do, because the webui will be hosted inside an CustomUiHelpBubble, which renders outside of a Chrome tab.
OK, but is there a reason `window.open()` doesn't work? Is it explicitly disabled, or is the functionality in `WebContentsDelegate` [1][2] overridden somehow? If we really need to duplicate web platform functionality in MojoJS, we should try to understand why it's broken first.
[1] window.open() is ultimately handled here in the browser side: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/web_contents/web_contents_impl.cc;l=5230;drc=4a1f2646209c5ecc0aae120e3654feb6d3f393f3
[2] which is delegated to the embedder with https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/web_contents_delegate.cc;l=47;drc=4a1f2646209c5ecc0aae120e3654feb6d3f393f3
Hi this is not resolved, can you further explain why you need to add this IPC? Thanks.
Related details
- John Lee
- Keren Zhu
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiming Zhou (Gerrit)
Yiming Zhou added 15 comments![]( "Open in Gerrit")
<message name="IDS_EXTENSIONS_ZERO_STATE_IPH_HEADER" desc="Header of the extensions zero state in-product help promo.">
Supercharge your browser with extensions and themes
</message>
<message name="IDS_EXTENSIONS_ZERO_STATE_CHIPS_IPH_DESCRIPTION" desc="An one line description for the extensions zero state in-product-help promo.">
Extensions are add-ons for Chrome that can help you do more
</message>
<message name="IDS_EXTENSIONS_ZERO_STATE_IPH_SHOPPING_CATEGORY_LABEL" desc="Label for the button that opens the Chrome Web Store shopping category page.">
Find coupons
</message>
<message name="IDS_EXTENSIONS_ZERO_STATE_IPH_WRITING_HELP_COLLECTION_LABEL" desc="Label for the button that opens the Chrome Web Store writing help collection page.">
Help me write
</message>
<message name="IDS_EXTENSIONS_ZERO_STATE_IPH_PRODUCTIVITY_CATEGORY_LABEL" desc="Label for the button that opens the Chrome Web Store producitivty category page.">
Boost productivity
</message>
<message name="IDS_EXTENSIONS_ZERO_STATE_IPH_AI_PRODUCTIVITY_COLLECTION_LABEL" desc="Label for the button that opens the Chrome Web Store ai productivity collection page.">
Enhance with AI
</message>
<message name="IDS_EXTENSIONS_ZERO_STATE_IPH_DISMISS_BUTTON_TITLE" desc="Title attribute for the button that dismisses the extensions zero state in-product-help promo.">
Close
</message>should we put these behind a "enable_extensions" flag, such that they are not unnecessarily included on builds where they are unused?
Done
"extensions zero state promo should not be enabeld for android")nit: spelling (here and below)
Done
deps += [
"//chrome/browser/ui/webui/extensions_zero_state_promo:mojo_bindings",
]Remove - this dep is now added twice, and seems to belong only on line 5360, under the same condition as the other source code that uses it.
#if BUILDFLAG(ENABLE_EXTENSIONS_CORE) && \
(BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX) || \
BUILDFLAG(IS_CHROMEOS))
#include "chrome/browser/ui/webui/extensions_zero_state_promo/zero_state_promo_ui.h"
#endif // BUILDFLAG(ENABLE_EXTENSIONS_CORE) && (BUILDFLAG(IS_WIN) ||
// BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS))Condition for this #if should match the one on line 351, i.e. let's use ENABLE_EXTENSIONS or
ENABLE_EXTENSIONS_CORE && (<long list of OSes>)in both spots.
Done
Dana FriedShould have additional OWNERS, the same way the chrome/browser/resources folder does.
Also, should we move this C++ code to its own directory also, if it's not closely related to the chrome://extensions UI and isn't intended to be owned by the same team?
Yiming Zhou+1 to putting this into its own subfolder; also I do volunteer to be an owner if there is a separate promo folder with separate OWNERS (separate OWNERS not required unless Rebekah says).
Rebekah PotterDone
Re-opening as this has been moved to a subfolder, but I don't see any OWNERS specified other than for the mojom file, which means this code is proposed to be de-facto owned by the WebUI team. We request that teams adding new WebUIs commit to maintaining their UIs, which is why we also ask for OWNERS from those teams to be added.
Done
OpenURL(url.mojom.Url url);Yiming Zhoudo you need to have this IPC in the browser?
Is it not possible for the webui to just call via JS to e.g. `window.open`?
Daniel ChengI do, because the webui will be hosted inside an CustomUiHelpBubble, which renders outside of a Chrome tab.
Will HarrisOK, but is there a reason `window.open()` doesn't work? Is it explicitly disabled, or is the functionality in `WebContentsDelegate` [1][2] overridden somehow? If we really need to duplicate web platform functionality in MojoJS, we should try to understand why it's broken first.
[1] window.open() is ultimately handled here in the browser side: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/web_contents/web_contents_impl.cc;l=5230;drc=4a1f2646209c5ecc0aae120e3654feb6d3f393f3
[2] which is delegated to the embedder with https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/web_contents_delegate.cc;l=47;drc=4a1f2646209c5ecc0aae120e3654feb6d3f393f3
Hi this is not resolved, can you further explain why you need to add this IPC? Thanks.
window.open does not work - and I have tried it. It is not all too surprising considering that this page is in a Chrome dialog. ui/webui/resources/js/browser_command/browser_command.mojom also has commands for opening URLs in a new tab, used by NTP.
Perhaps @dfr...@chromium.org can comment on why window.open does not work even though it is routed to and handled from the browser side.
is raw_ptr needed? I don't see this used in this file but may be missing something.
Done
#include "chrome/browser/ui/chrome_pages.h"
#include "chrome/grit/generated_resources.h"
#include "components/prefs/pref_service.h"
#include "components/profile_metrics/browser_profile_type.h"
#include "components/strings/grit/components_strings.h"Yiming Zhouunused?
Done
#include "content/public/browser/web_contents.h"
#include "content/public/browser/web_ui.h"Yiming Zhoualso unused?
Done
#include "ui/base/l10n/l10n_util.h"Yiming Zhouunused?
Done
#include "chrome/browser/ui/ui_features.h"Yiming Zhouunused?
Done
I don't think we have any branded strings here?
Done
#include "components/strings/grit/components_strings.h"The strings for this UI are in the chrome/grit/generated_resources.h header AFAICT, not in components/
Done
#include "ui/accessibility/accessibility_features.h"
#include "ui/base/ui_base_features.h"Do we need all these feature includes?
Done
#include "ui/webui/resources/grit/webui_resources.h"
#include "ui/webui/resources/grit/webui_resources_map.h"What resources from the shared resources are being used in this file? Normally WebUIs should get shared resources from chrome://resources, and not need to manually register them with their individual data source.
- Dana Fried
- Daniel Cheng
- John Lee
- Keren Zhu
- Rebekah Potter
- Will Harris
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keren Zhu (Gerrit)
Keren Zhu added 1 comment![]( "Open in Gerrit")
OpenURL(url.mojom.Url url);Yiming Zhoudo you need to have this IPC in the browser?
Is it not possible for the webui to just call via JS to e.g. `window.open`?
Daniel ChengI do, because the webui will be hosted inside an CustomUiHelpBubble, which renders outside of a Chrome tab.
Will HarrisOK, but is there a reason `window.open()` doesn't work? Is it explicitly disabled, or is the functionality in `WebContentsDelegate` [1][2] overridden somehow? If we really need to duplicate web platform functionality in MojoJS, we should try to understand why it's broken first.
[1] window.open() is ultimately handled here in the browser side: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/web_contents/web_contents_impl.cc;l=5230;drc=4a1f2646209c5ecc0aae120e3654feb6d3f393f3
[2] which is delegated to the embedder with https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/web_contents_delegate.cc;l=47;drc=4a1f2646209c5ecc0aae120e3654feb6d3f393f3
Yiming ZhouHi this is not resolved, can you further explain why you need to add this IPC? Thanks.
window.open does not work - and I have tried it. It is not all too surprising considering that this page is in a Chrome dialog. ui/webui/resources/js/browser_command/browser_command.mojom also has commands for opening URLs in a new tab, used by NTP.
Perhaps @dfr...@chromium.org can comment on why window.open does not work even though it is routed to and handled from the browser side.
Top-chrome WebUI's WebContentsDelegate is WebUIContentsWrapper. It forwards the AddNewWebContents() call to WebUIContentsWrapper::Host. There are a few different types of hosts, the one used here I believe is WebUIBubbleDialogView, which does not implement AddNewWebContents(), so `window.open()` becomes a no-op.
Related details
- Dana Fried
- Daniel Cheng
- John Lee
- Rebekah Potter
- Will Harris
- Yiming Zhou
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
OpenURL(url.mojom.Url url);Yiming Zhoudo you need to have this IPC in the browser?
Is it not possible for the webui to just call via JS to e.g. `window.open`?
Daniel ChengI do, because the webui will be hosted inside an CustomUiHelpBubble, which renders outside of a Chrome tab.
Will HarrisOK, but is there a reason `window.open()` doesn't work? Is it explicitly disabled, or is the functionality in `WebContentsDelegate` [1][2] overridden somehow? If we really need to duplicate web platform functionality in MojoJS, we should try to understand why it's broken first.
[1] window.open() is ultimately handled here in the browser side: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/web_contents/web_contents_impl.cc;l=5230;drc=4a1f2646209c5ecc0aae120e3654feb6d3f393f3
[2] which is delegated to the embedder with https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/web_contents_delegate.cc;l=47;drc=4a1f2646209c5ecc0aae120e3654feb6d3f393f3
Yiming ZhouHi this is not resolved, can you further explain why you need to add this IPC? Thanks.
Keren Zhuwindow.open does not work - and I have tried it. It is not all too surprising considering that this page is in a Chrome dialog. ui/webui/resources/js/browser_command/browser_command.mojom also has commands for opening URLs in a new tab, used by NTP.
Perhaps @dfr...@chromium.org can comment on why window.open does not work even though it is routed to and handled from the browser side.
Top-chrome WebUI's WebContentsDelegate is WebUIContentsWrapper. It forwards the AddNewWebContents() call to WebUIContentsWrapper::Host. There are a few different types of hosts, the one used here I believe is WebUIBubbleDialogView, which does not implement AddNewWebContents(), so `window.open()` becomes a no-op.
Right... I think the question is "should WebUIBubbleDialogView implement it", so we don't have N places implementing window.open() via a custom Mojo call?
Related details
- Dana Fried
Dana Fried (Gerrit)
Dana Fried added 1 comment![]( "Open in Gerrit")
OpenURL(url.mojom.Url url);Yiming Zhoudo you need to have this IPC in the browser?
Is it not possible for the webui to just call via JS to e.g. `window.open`?
Daniel ChengI do, because the webui will be hosted inside an CustomUiHelpBubble, which renders outside of a Chrome tab.
Will HarrisOK, but is there a reason `window.open()` doesn't work? Is it explicitly disabled, or is the functionality in `WebContentsDelegate` [1][2] overridden somehow? If we really need to duplicate web platform functionality in MojoJS, we should try to understand why it's broken first.
[1] window.open() is ultimately handled here in the browser side: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/web_contents/web_contents_impl.cc;l=5230;drc=4a1f2646209c5ecc0aae120e3654feb6d3f393f3
[2] which is delegated to the embedder with https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/web_contents_delegate.cc;l=47;drc=4a1f2646209c5ecc0aae120e3654feb6d3f393f3
Yiming ZhouHi this is not resolved, can you further explain why you need to add this IPC? Thanks.
Keren Zhuwindow.open does not work - and I have tried it. It is not all too surprising considering that this page is in a Chrome dialog. ui/webui/resources/js/browser_command/browser_command.mojom also has commands for opening URLs in a new tab, used by NTP.
Perhaps @dfr...@chromium.org can comment on why window.open does not work even though it is routed to and handled from the browser side.
Daniel ChengTop-chrome WebUI's WebContentsDelegate is WebUIContentsWrapper. It forwards the AddNewWebContents() call to WebUIContentsWrapper::Host. There are a few different types of hosts, the one used here I believe is WebUIBubbleDialogView, which does not implement AddNewWebContents(), so `window.open()` becomes a no-op.
Right... I think the question is "should WebUIBubbleDialogView implement it", so we don't have N places implementing window.open() via a custom Mojo call?
I don't necessarily disagree that TopChrome WebUI bubbles should have access to `window.open()` but that is way out of scope for this CL or even stack of CLs.
Yiming is definitely not the person who would be implementing this. Maybe we create a follow-up and see if there's a good person to take care of this.
Related details
Will Harris (Gerrit)
Will Harris voted and added 2 comments![]( "Open in Gerrit")
Votes added by Will Harris
| Code-Review | +1 |
2 comments
lgtm the `chrome/browser/chrome_browser_interface_binders_webui.cc` and `chrome/browser/ui/webui/extensions_zero_state_promo/zero_state_promo.mojom` files
OpenURL(url.mojom.Url url);Yiming Zhoudo you need to have this IPC in the browser?
Is it not possible for the webui to just call via JS to e.g. `window.open`?
Daniel ChengI do, because the webui will be hosted inside an CustomUiHelpBubble, which renders outside of a Chrome tab.
Will HarrisOK, but is there a reason `window.open()` doesn't work? Is it explicitly disabled, or is the functionality in `WebContentsDelegate` [1][2] overridden somehow? If we really need to duplicate web platform functionality in MojoJS, we should try to understand why it's broken first.
[1] window.open() is ultimately handled here in the browser side: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/web_contents/web_contents_impl.cc;l=5230;drc=4a1f2646209c5ecc0aae120e3654feb6d3f393f3
[2] which is delegated to the embedder with https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/web_contents_delegate.cc;l=47;drc=4a1f2646209c5ecc0aae120e3654feb6d3f393f3
Yiming ZhouHi this is not resolved, can you further explain why you need to add this IPC? Thanks.
Keren Zhuwindow.open does not work - and I have tried it. It is not all too surprising considering that this page is in a Chrome dialog. ui/webui/resources/js/browser_command/browser_command.mojom also has commands for opening URLs in a new tab, used by NTP.
Perhaps @dfr...@chromium.org can comment on why window.open does not work even though it is routed to and handled from the browser side.
Daniel ChengTop-chrome WebUI's WebContentsDelegate is WebUIContentsWrapper. It forwards the AddNewWebContents() call to WebUIContentsWrapper::Host. There are a few different types of hosts, the one used here I believe is WebUIBubbleDialogView, which does not implement AddNewWebContents(), so `window.open()` becomes a no-op.
Dana FriedRight... I think the question is "should WebUIBubbleDialogView implement it", so we don't have N places implementing window.open() via a custom Mojo call?
I don't necessarily disagree that TopChrome WebUI bubbles should have access to `window.open()` but that is way out of scope for this CL or even stack of CLs.
Yiming is definitely not the person who would be implementing this. Maybe we create a follow-up and see if there's a good person to take care of this.
understood. thank you for restricting the links that can be opened by this API.
perhaps we should add a TODO here to try and implement the required plumbing to allow window.open to work?
Related details
- John Lee
- Rebekah Potter
- Yiming Zhou
Yiming Zhou (Gerrit)
Yiming Zhou added 2 comments![]( "Open in Gerrit")
The test that's coming up flaky here isn't even added in this CL. However, attempting to activate the browser on Linux is a known issue; I think your `BrowserActivationWater` is spurious and should probably be removed, especially considering your promo gets hammered immediately and will take focus. This is almost certainly the cause of the flake.
Acknowledged
OpenURL(url.mojom.Url url);Yiming Zhoudo you need to have this IPC in the browser?
Is it not possible for the webui to just call via JS to e.g. `window.open`?
Daniel ChengI do, because the webui will be hosted inside an CustomUiHelpBubble, which renders outside of a Chrome tab.
Will HarrisOK, but is there a reason `window.open()` doesn't work? Is it explicitly disabled, or is the functionality in `WebContentsDelegate` [1][2] overridden somehow? If we really need to duplicate web platform functionality in MojoJS, we should try to understand why it's broken first.
[1] window.open() is ultimately handled here in the browser side: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/web_contents/web_contents_impl.cc;l=5230;drc=4a1f2646209c5ecc0aae120e3654feb6d3f393f3
[2] which is delegated to the embedder with https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/web_contents_delegate.cc;l=47;drc=4a1f2646209c5ecc0aae120e3654feb6d3f393f3
Yiming ZhouHi this is not resolved, can you further explain why you need to add this IPC? Thanks.
Keren Zhuwindow.open does not work - and I have tried it. It is not all too surprising considering that this page is in a Chrome dialog. ui/webui/resources/js/browser_command/browser_command.mojom also has commands for opening URLs in a new tab, used by NTP.
Perhaps @dfr...@chromium.org can comment on why window.open does not work even though it is routed to and handled from the browser side.
Daniel ChengTop-chrome WebUI's WebContentsDelegate is WebUIContentsWrapper. It forwards the AddNewWebContents() call to WebUIContentsWrapper::Host. There are a few different types of hosts, the one used here I believe is WebUIBubbleDialogView, which does not implement AddNewWebContents(), so `window.open()` becomes a no-op.
Dana FriedRight... I think the question is "should WebUIBubbleDialogView implement it", so we don't have N places implementing window.open() via a custom Mojo call?
Will HarrisI don't necessarily disagree that TopChrome WebUI bubbles should have access to `window.open()` but that is way out of scope for this CL or even stack of CLs.
Yiming is definitely not the person who would be implementing this. Maybe we create a follow-up and see if there's a good person to take care of this.
understood. thank you for restricting the links that can be opened by this API.
perhaps we should add a TODO here to try and implement the required plumbing to allow window.open to work?
- Dana Fried
- Daniel Cheng
- John Lee
- Keren Zhu
- Rebekah Potter
- Will Harris
Code-Coverage
Code-Review
Rebekah Potter (Gerrit)
Rebekah Potter voted and added 5 comments![]( "Open in Gerrit")
Votes added by Rebekah Potter
| Code-Review | +1 |
5 comments
ts_definitions = [ "//tools/typescript/definitions/metrics_private.d.ts" ]no longer needed?
"//ui/webui/resources/js:build_ts",I don't think you're using this anywhere either, but maybe missing something.
"//ui/webui/resources/mojo:build_ts",no longer used
"//chrome/test/data/webui/extensions_zero_state:build_ts",not actually needed, because the test target does not depend on //third_party/lit
Related details
- Dana Fried
- Daniel Cheng
- John Lee
- Keren Zhu
- Will Harris
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiming Zhou (Gerrit)
Yiming Zhou added 4 comments![]( "Open in Gerrit")
ts_definitions = [ "//tools/typescript/definitions/metrics_private.d.ts" ]Yiming Zhouno longer needed?
Done
I don't think you're using this anywhere either, but maybe missing something.
Actually the icons.html file needs it, otherwise it hit this build error: https://paste.googleplex.com/6675324332670976
"//ui/webui/resources/mojo:build_ts",Yiming Zhouno longer used
Done
"//chrome/test/data/webui/extensions_zero_state:build_ts",not actually needed, because the test target does not depend on //third_party/lit
- Dana Fried
- Daniel Cheng
- John Lee
- Keren Zhu
- Rebekah Potter
- Will Harris
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rebekah Potter
| Code-Review | +1 |
1 comment
"//ui/webui/resources/js:build_ts",Yiming ZhouI don't think you're using this anywhere either, but maybe missing something.
Actually the icons.html file needs it, otherwise it hit this build error: https://paste.googleplex.com/6675324332670976
- Dana Fried
- Daniel Cheng
- John Lee
- Keren Zhu
- Will Harris
- Yiming Zhou
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiming Zhou (Gerrit)
Yiming Zhou added 1 comment![]( "Open in Gerrit")
Hi @rdevlin...@chromium.org, may I trouble you to review the change to extensions/common/api/_api_features.json? This addition was required by presubmit.
Related details
- Dana Fried
- Demetrios Papadopoulos
- Devlin Cronin
- Hidehiko Abe
- John Lee
- Keren Zhu
- Robert Kaplow
- Tim
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 5 comments![]( "Open in Gerrit")
Hi @rdevlin...@chromium.org, may I trouble you to review the change to extensions/common/api/_api_features.json? This addition was required by presubmit.
Did you mean to add a bunch of new reviewers in this CL? If so please specify what you are asking each reviewer to review.
Also, what about breaking down this CL to smallper pieces (like backend vs frontend). I can see that this has already gone through quite a few rounds of reviews, but landing an entire feature in a single massive CL is not great to begin with.
Please add DIR_METADATA files for all newly added folders.
margin: 0px;```suggestion
margin: 0;
```
<cr-chip id="writingButton" chip-role="link"Remove extra space.
static getInstance(): ZeroStatePromoBrowserProxy {
return instance || (instance = new ZeroStatePromoBrowserProxyImpl());
}
static setInstance(obj: ZeroStatePromoBrowserProxy) {
instance = obj;
}Nit: Move these after all non-static methods.
Related details
- Dana Fried
- Devlin Cronin
- Keren Zhu
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiming Zhou (Gerrit)
Yiming Zhou added 5 comments![]( "Open in Gerrit")
Demetrios PapadopoulosHi @rdevlin...@chromium.org, may I trouble you to review the change to extensions/common/api/_api_features.json? This addition was required by presubmit.
Did you mean to add a bunch of new reviewers in this CL? If so please specify what you are asking each reviewer to review.
Also, what about breaking down this CL to smallper pieces (like backend vs frontend). I can see that this has already gone through quite a few rounds of reviews, but landing an entire feature in a single massive CL is not great to begin with.
Pardon, I was trying to find the owner for a file and must have clicked the wrong button in 'suggest owners'.
Please add DIR_METADATA files for all newly added folders.
Done
margin: 0px;Yiming Zhou```suggestion
margin: 0;
```
Done
<cr-chip id="writingButton" chip-role="link"Yiming ZhouRemove extra space.
Done
static getInstance(): ZeroStatePromoBrowserProxy {
return instance || (instance = new ZeroStatePromoBrowserProxyImpl());
}
static setInstance(obj: ZeroStatePromoBrowserProxy) {
instance = obj;
}Nit: Move these after all non-static methods.
- Dana Fried
- Demetrios Papadopoulos
- Devlin Cronin
- Keren Zhu
- Rebekah Potter
Code-Coverage
Code-Review
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
1 comment
Just confirming: You only need me to take a look at extensions/common/api/_api_features.json ? If so, that file LGTM.
Related details
- Dana Fried
- Demetrios Papadopoulos
- Keren Zhu
- Rebekah Potter
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiming Zhou (Gerrit)
Yiming Zhou added 1 comment![]( "Open in Gerrit")
Just confirming: You only need me to take a look at extensions/common/api/_api_features.json ? If so, that file LGTM.
Yes, that is the one file missing approval. Thanks!
Related details
- Dana Fried
- Demetrios Papadopoulos
- Keren Zhu
- Rebekah Potter
Code-Coverage
Code-Review
Rebekah Potter (Gerrit)
Rebekah Potter voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rebekah Potter
| Code-Review | +1 |
1 comment
- Dana Fried
- Demetrios Papadopoulos
- Keren Zhu
- Yiming Zhou
Code-Coverage
Code-Review
Yiming Zhou (Gerrit)
Yiming Zhou voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Dana Fried
- Demetrios Papadopoulos
- Keren Zhu
Dana Fried (Gerrit)
Dana Fried added 2 comments![]( "Open in Gerrit")
bool ShouldAutoResizeHost() override; \Please do not change this code, it will break things.
If you need to move this out of the header file due to e.g. compile warnings or code duplication, we should add a matching `DEFINE_TOP_CHROME_WEBUI_CONFIG` for the .cc files.
bool TestWebUIHelpBubbleControllerConfig::ShouldAutoResizeHost() {See comment in `custom_webui_help_bubble_controller.h`
Related details
- Demetrios Papadopoulos
- Devlin Cronin
- Keren Zhu
- Rebekah Potter
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried added 1 comment![]( "Open in Gerrit")
Browser* browser = chrome::FindLastActive();Presubmit in later CLs is failing because of the use of this function.
I think you just want to use `Navigate()`:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/browser_navigator.h;l=21
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
1 comment
extensions/common/api/_api_features.json s lgtm
Related details
- Demetrios Papadopoulos
- Keren Zhu
- Rebekah Potter
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter voted Code-Review+1![]( "Open in Gerrit")
- Demetrios Papadopoulos
- Keren Zhu
- Yiming Zhou
Code-Coverage
Code-Review
Yiming Zhou (Gerrit)
Yiming Zhou added 3 comments![]( "Open in Gerrit")
Please do not change this code, it will break things.
If you need to move this out of the header file due to e.g. compile warnings or code duplication, we should add a matching `DEFINE_TOP_CHROME_WEBUI_CONFIG` for the .cc files.
Done
bool TestWebUIHelpBubbleControllerConfig::ShouldAutoResizeHost() {Yiming ZhouSee comment in `custom_webui_help_bubble_controller.h`
Done
Presubmit in later CLs is failing because of the use of this function.
I think you just want to use `Navigate()`:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/browser_navigator.h;l=21
- Dana Fried
- Demetrios Papadopoulos
- Keren Zhu
Code-Coverage
Code-Review
Dana Fried (Gerrit)
Dana Fried voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dana Fried
| Code-Review | +1 |
1 comment
Browser* browser = chrome::FindLastActive();Yiming ZhouPresubmit in later CLs is failing because of the use of this function.
I think you just want to use `Navigate()`:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/browser_navigator.h;l=21
Done
You are still calling `FindLastActive`; the `Navigate()` call below should be sufficient. You can create the navigate params using the current profile (which you can get from the browser context of the web contents) which will provide the correct behavior (new tab in the most foreground window, bringing window to the front).
Alternatively, you can get the browser from the anchor view/widget of the bubble.
In other case, you do not need to generate the disposition from a function; you can just use the default disposition.
Related details
- Demetrios Papadopoulos
- Devlin Cronin
- Keren Zhu
- Rebekah Potter
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried added 1 comment![]( "Open in Gerrit")
#define DEFINE_TOP_CHROME_WEBUI_CONFIG(ControllerClass) \thank you!
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
extensions/common/api/_api_features.json s lgtm
- Demetrios Papadopoulos
- Keren Zhu
- Rebekah Potter
- Yiming Zhou
Code-Coverage
Code-Review
Rebekah Potter (Gerrit)
Rebekah Potter voted Code-Review+1![]( "Open in Gerrit")
- Demetrios Papadopoulos
- Keren Zhu
- Yiming Zhou
Yiming Zhou (Gerrit)
Yiming Zhou added 1 comment![]( "Open in Gerrit")
Browser* browser = chrome::FindLastActive();Yiming ZhouPresubmit in later CLs is failing because of the use of this function.
I think you just want to use `Navigate()`:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/browser_navigator.h;l=21
Dana FriedDone
You are still calling `FindLastActive`; the `Navigate()` call below should be sufficient. You can create the navigate params using the current profile (which you can get from the browser context of the web contents) which will provide the correct behavior (new tab in the most foreground window, bringing window to the front).
Alternatively, you can get the browser from the anchor view/widget of the bubble.
In other case, you do not need to generate the disposition from a function; you can just use the default disposition.
I take it that means routing the profile or webcontent from the page UI to the page handler. Please let me know if this is done correctly. The IPH works just fine.
Related details
- Dana Fried
- Demetrios Papadopoulos
- Devlin Cronin
- Keren Zhu
- Rebekah Potter
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dana Fried
| Code-Review | +1 |
2 comments
Profile* profile,Note that you can get this from the WebContents (WebContents::GetBrowserContext() is a profile) but this is fine.
Related details
- Demetrios Papadopoulos
- Devlin Cronin
- Keren Zhu
- Rebekah Potter
- Yiming Zhou
Code-Coverage
Code-Review
No-Unresolved-Comments
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
Devlin Croninextensions/common/api/_api_features.json s lgtm
and still
- Demetrios Papadopoulos
- Keren Zhu
- Rebekah Potter
- Yiming Zhou
Code-Coverage
Code-Review
Dana Fried (Gerrit)
Dana Fried voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dana Fried
| Code-Review | +1 |
1 comment
Related details
- Demetrios Papadopoulos
- Devlin Cronin
- Keren Zhu
- Rebekah Potter
- Yiming Zhou
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter voted Code-Review+1![]( "Open in Gerrit")
- Demetrios Papadopoulos
- Devlin Cronin
- Keren Zhu
- Yiming Zhou
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
Devlin Croninextensions/common/api/_api_features.json s lgtm
Devlin Croninand still
and still
- Demetrios Papadopoulos
- Keren Zhu
- Yiming Zhou
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiming Zhou (Gerrit)
Yiming Zhou voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Demetrios Papadopoulos
- Keren Zhu
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Extensions] WebUI page for rendering the Extensions Zero State Promo IPH.
Screenshot: https://screenshot.googleplex.com/BifQ5bV5erYnizs
- M chrome/app/extensions_strings.grdp
- A chrome/app/extensions_strings_grdp/IDS_EXTENSIONS_ZERO_STATE_CHIPS_IPH_DESCRIPTION.png.sha1
- A chrome/app/extensions_strings_grdp/IDS_EXTENSIONS_ZERO_STATE_IPH_AI_PRODUCTIVITY_COLLECTION_LABEL.png.sha1
- A chrome/app/extensions_strings_grdp/IDS_EXTENSIONS_ZERO_STATE_IPH_DISMISS_BUTTON_TITLE.png.sha1
- A chrome/app/extensions_strings_grdp/IDS_EXTENSIONS_ZERO_STATE_IPH_HEADER.png.sha1
- A chrome/app/extensions_strings_grdp/IDS_EXTENSIONS_ZERO_STATE_IPH_PRODUCTIVITY_CATEGORY_LABEL.png.sha1
- A chrome/app/extensions_strings_grdp/IDS_EXTENSIONS_ZERO_STATE_IPH_SHOPPING_CATEGORY_LABEL.png.sha1
- A chrome/app/extensions_strings_grdp/IDS_EXTENSIONS_ZERO_STATE_IPH_WRITING_HELP_COLLECTION_LABEL.png.sha1
- M chrome/browser/BUILD.gn
- M chrome/browser/chrome_browser_interface_binders_webui.cc
- M chrome/browser/resources/BUILD.gn
- A chrome/browser/resources/extensions_zero_state_promo/BUILD.gn
- A chrome/browser/resources/extensions_zero_state_promo/DIR_METADATA
- A chrome/browser/resources/extensions_zero_state_promo/OWNERS
- A chrome/browser/resources/extensions_zero_state_promo/icons.html
- A chrome/browser/resources/extensions_zero_state_promo/zero_state_promo.html
- A chrome/browser/resources/extensions_zero_state_promo/zero_state_promo_app.css
- A chrome/browser/resources/extensions_zero_state_promo/zero_state_promo_app.html.ts
- A chrome/browser/resources/extensions_zero_state_promo/zero_state_promo_app.ts
- A chrome/browser/resources/extensions_zero_state_promo/zero_state_promo_browser_proxy.ts
- M chrome/browser/ui/BUILD.gn
- M chrome/browser/ui/views/user_education/custom_webui_help_bubble_controller.h
- M chrome/browser/ui/views/user_education/custom_webui_help_bubble_interactive_uitest.cc
- M chrome/browser/ui/webui/chrome_web_ui_configs.cc
- A chrome/browser/ui/webui/extensions_zero_state_promo/BUILD.gn
- A chrome/browser/ui/webui/extensions_zero_state_promo/DIR_METADATA
- A chrome/browser/ui/webui/extensions_zero_state_promo/zero_state_promo.mojom
- A chrome/browser/ui/webui/extensions_zero_state_promo/zero_state_promo_page_handler.cc
- A chrome/browser/ui/webui/extensions_zero_state_promo/zero_state_promo_page_handler.h
- A chrome/browser/ui/webui/extensions_zero_state_promo/zero_state_promo_page_handler_unittest.cc
- A chrome/browser/ui/webui/extensions_zero_state_promo/zero_state_promo_ui.cc
- A chrome/browser/ui/webui/extensions_zero_state_promo/zero_state_promo_ui.h
- M chrome/browser_exposed_mojom_targets.gni
- M chrome/chrome_paks.gni
- M chrome/common/webui_url_constants.h
- M chrome/test/data/webui/BUILD.gn
- A chrome/test/data/webui/extensions_zero_state/BUILD.gn
- A chrome/test/data/webui/extensions_zero_state/DIR_METADATA
- A chrome/test/data/webui/extensions_zero_state/zero_state_promo_app_test.ts
- A chrome/test/data/webui/extensions_zero_state/zero_state_promo_browsertest.cc
- M extensions/common/api/_api_features.json
- M third_party/lit/v3_0/BUILD.gn
- M tools/gritsettings/resource_ids.spec
- M tools/metrics/histograms/enums.xml
- M tools/metrics/histograms/metadata/extensions/histograms.xml
- M tools/metrics/histograms/metadata/ui/enums.xml
Code-Review: +1 by Dana Fried, +1 by Devlin Cronin, +1 by Rebekah Potter
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |