Rename ReloadButtonWebView and related classes [chromium/src : main]
Russ Hamilton (Gerrit)
Russ Hamilton added 2 comments![]( "Open in Gerrit")
[ "chrome://reload-button.top-chrome|" +Russ Hamiltonshould this be `webui-toolbar`, as defined in enums.xml?
Good catch! Done.
class ReloadControlImpl : public ReloadControl {Paul JensenIt makes sense to have this layer of abstraction, but I'd suggest to do it in a follow-up CL, so this CL becomes a pure renaming. I think that would make the diff easier to track as well.
It's also applicable to many other changes that are not just renaming reload button -> toolbar.
Russ HamiltonRenaming the HTML and the WebView separately will leave the code in an inconsistent state for a time, where either the toolbar WebView will be hosting reload-button-specific HTML or the reload button WebView will be hosting the toolbar HTML. Is this okay?
I split the CL to do the abstraction first and then rename.
Related details
- Mingyu Lei
- Paul Jensen
- Qingxin Wu
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Russ Hamilton (Gerrit)
Russ Hamilton added 1 comment![]( "Open in Gerrit")
class ReloadControlImpl : public ReloadControl {Paul JensenIt makes sense to have this layer of abstraction, but I'd suggest to do it in a follow-up CL, so this CL becomes a pure renaming. I think that would make the diff easier to track as well.
It's also applicable to many other changes that are not just renaming reload button -> toolbar.
Russ HamiltonRenaming the HTML and the WebView separately will leave the code in an inconsistent state for a time, where either the toolbar WebView will be hosting reload-button-specific HTML or the reload button WebView will be hosting the toolbar HTML. Is this okay?
I split the CL to do the abstraction first and then rename.
The abstraction has landed. @le...@chromium.org Do have time to review this?
Related details
- Mingyu Lei
- Paul Jensen
- Qingxin Wu
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni voted and added 2 comments![]( "Open in Gerrit")
Votes added by Rakina Zata Amni
| Code-Review | +1 |
2 comments
content/public/browser/web_ui_controller.h and other parts that I own LGTM
{features::kInitialWebUI, features::kWebUIReloadButton,Russ HamiltonSince we are actively experimenting the reload button, I'm curious what the plan is for implementing the next phases while keeping the previous phase experiment live but without exposing the still-not-ready additions. Maybe we will have separate flags for each phase, and what the `WebUIToolbarWebView` contains will be incremental depending on the flag?
Rakina Zata AmniYes, we will have separate flags for each button that we add and the WebUIToolbar TypeScript would only show the controls that are actually enabled. That logic is not in this CL (as this CL is already too big).
Acknowledged
Related details
- Mingyu Lei
- Paul Jensen
- Qingxin Wu
- Russ Hamilton
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mingyu Lei (Gerrit)
Mingyu Lei voted and added 4 comments![]( "Open in Gerrit")
Votes added by Mingyu Lei
| Code-Review | +1 |
4 comments
Sorry for the delay, there are also some new changes checked in with the old url (e.g. https://chromium-review.googlesource.com/c/catapult/+/7274162) please make sure we cover all of them while resolving the conflicts.
"reload_button.ts",Russ HamiltonI'm not 100% sure about the rules but I remember from the very beginning I was told to keep the file name `app.ts`: https://chromium-review.googlesource.com/c/chromium/src/+/6942651/comment/d986381b_40a7c2c4/
Mingyu LeiThat makes sense if the typescript file is the top-level WebUI element, but it's not in our case. At some point we may need a Lit control at the top level, which we would need to name app.ts/app.html.ts/ap.html.
Paul JensenI thought webui_toolbar.ts is the top level element for this component though.
Mingyu LeiIn this CL, there is no top level Lit control, there is only a top level static HTML file (webui_toolbar.html) and a webui_toolbar.ts file that includes the TypeScript files for the subcomponents (just reload_button.js for now) similar to https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/resources/webui_browser/webui_browser.ts
Acknowledged
component_id: 1456991I think probably 1935629 is a better ID for this, by the time we added the DIR_METADATA, this component was not created yet.
class ReloadControlImpl : public ReloadControl {Paul JensenIt makes sense to have this layer of abstraction, but I'd suggest to do it in a follow-up CL, so this CL becomes a pure renaming. I think that would make the diff easier to track as well.
It's also applicable to many other changes that are not just renaming reload button -> toolbar.
Russ HamiltonRenaming the HTML and the WebView separately will leave the code in an inconsistent state for a time, where either the toolbar WebView will be hosting reload-button-specific HTML or the reload button WebView will be hosting the toolbar HTML. Is this okay?
Russ HamiltonI split the CL to do the abstraction first and then rename.
The abstraction has landed. @le...@chromium.org Do have time to review this?
Russ Hamilton (Gerrit)
Russ Hamilton voted and added 3 comments![]( "Open in Gerrit")
Votes added by Russ Hamilton
| Commit-Queue | +1 |
Sorry for the delay, there are also some new changes checked in with the old url (e.g. https://chromium-review.googlesource.com/c/catapult/+/7274162) please make sure we cover all of them while resolving the conflicts.
Done. Do you know why some of the URLs use `chrome://reload-button` and others use `chrome://reload-button.top-chrome`? I've kept things consistent, but I'm curious about the discrepancy.
Adding Chromium IPC reviewers for mojoms changes
Adding chromium-chrome-brow...@google.com for chrome/browser/ui/views/
Adding john...@chromium.org for chrome/browser/ui/webui and chrome/browser/resources/
I think probably 1935629 is a better ID for this, by the time we added the DIR_METADATA, this component was not created yet.
- //chrome/browser/ui/views Reviews
- Chromium IPC Reviews
- John Lee
- Mingyu Lei
- Paul Jensen
- Qingxin Wu
- Rakina Zata Amni
- Russ Hamilton
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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: d...@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): d...@chromium.org
Reviewer source(s):
dfr...@chromium.org is from context(googleclient/chrome/chromium_gwsq/chrome/browser/ui/views/config.gwsq)
d...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Dana Fried
- Dominic Farolino
- John Lee
- Mingyu Lei
- Paul Jensen
- Qingxin Wu
- Rakina Zata Amni
- Russ Hamilton
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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")
- Dominic Farolino
- John Lee
- Mingyu Lei
- Paul Jensen
- Qingxin Wu
- Rakina Zata Amni
- Russ Hamilton
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Mingyu Lei (Gerrit)
Mingyu Lei added 1 comment![]( "Open in Gerrit")
Russ HamiltonSorry for the delay, there are also some new changes checked in with the old url (e.g. https://chromium-review.googlesource.com/c/catapult/+/7274162) please make sure we cover all of them while resolving the conflicts.
Done. Do you know why some of the URLs use `chrome://reload-button` and others use `chrome://reload-button.top-chrome`? I've kept things consistent, but I'm curious about the discrepancy.
Hmm I think we should always use `chrome://reload-button.top-chrome`.
Looking at https://chromium-review.googlesource.com/c/catapult/+/7274162 it seems the CL checks if `chrome://reload-button` is part of the tab url (`if any(url in tab.url for url in UNCLOSEABLE_URLS)`) so it passes. I think it's better to keep the full `chrome://reload-button.top-chrome` unless it's failing for some cases.
@elk...@chromium.org do you remember why the `UNCLOSEABLE_URLS` is not using the full url with the `.top-chrome` suffix?
Related details
- Dominic Farolino
- John Lee
Russ Hamilton (Gerrit)
Russ Hamilton added 1 comment![]( "Open in Gerrit")
Adding dpa...@chromium.org for third_party/lit/v3_0/BUILD.gn
Adding cshar...@chromium.org for tools/metrics/histograms/page/histograms.xml
Related details
- Charlie Harrison
- Dana Fried
- Demetrios Papadopoulos
- Dominic Farolino
- John Lee
- Paul Jensen
- Qingxin Wu
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Harrison (Gerrit)
Charlie Harrison added 1 comment![]( "Open in Gerrit")
- Dana Fried
- Demetrios Papadopoulos
- Dominic Farolino
- John Lee
- Paul Jensen
- Qingxin Wu
- Rakina Zata Amni
- Russ Hamilton
Charlie Harrison (Gerrit)
Charlie Harrison voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Dana Fried
- Demetrios Papadopoulos
- Dominic Farolino
- John Lee
- Paul Jensen
- Qingxin Wu
- Rakina Zata Amni
- Russ Hamilton
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Russ Hamilton (Gerrit)
Russ Hamilton added 1 comment![]( "Open in Gerrit")
Russ HamiltonSorry for the delay, there are also some new changes checked in with the old url (e.g. https://chromium-review.googlesource.com/c/catapult/+/7274162) please make sure we cover all of them while resolving the conflicts.
Mingyu LeiDone. Do you know why some of the URLs use `chrome://reload-button` and others use `chrome://reload-button.top-chrome`? I've kept things consistent, but I'm curious about the discrepancy.
Hmm I think we should always use `chrome://reload-button.top-chrome`.
Looking at https://chromium-review.googlesource.com/c/catapult/+/7274162 it seems the CL checks if `chrome://reload-button` is part of the tab url (`if any(url in tab.url for url in UNCLOSEABLE_URLS)`) so it passes. I think it's better to keep the full `chrome://reload-button.top-chrome` unless it's failing for some cases.
@elk...@chromium.org do you remember why the `UNCLOSEABLE_URLS` is not using the full url with the `.top-chrome` suffix?
Created crrev.com/c/7419235 and crrev.com/c/7414573 to fix the discrepancy.
Related details
- Dana Fried
- Demetrios Papadopoulos
- Dominic Farolino
- John Lee
- Mingyu Lei
Dana Fried (Gerrit)
Dana Fried voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dana Fried
| Code-Review | +1 |
1 comment
Renewing my +1, but please fix presubmit errors.
Related details
Dana Fried (Gerrit)
Dana Fried voted Code-Review+1![]( "Open in Gerrit")
- Charlie Harrison
- Demetrios Papadopoulos
- Dominic Farolino
- John Lee
- Mingyu Lei
- Paul Jensen
- Rakina Zata Amni
- Russ Hamilton
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |