WebUI: Leverage the new WebUIDataSourceImpl#SetDefaultResource(). [chromium/src : main]
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos voted and added 1 comment![]( "Open in Gerrit")
Votes added by Demetrios Papadopoulos
| Commit-Queue | +1 |
1 comment
PTAL. WDYT?
I am also wondering if we should rename `SeDefaultResource()` to `SetRootResource()` for more clarity. Although this would create quite some churn,I think the name would be less confusing and more disassociated with the current name.
Related details
- Rebekah Potter
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. |
Rebekah Potter (Gerrit)
Rebekah Potter added 3 comments![]( "Open in Gerrit")
PTAL. WDYT?
I am also wondering if we should rename `SeDefaultResource()` to `SetRootResource()` for more clarity. Although this would create quite some churn,I think the name would be less confusing and more disassociated with the current name.
I was going to say that this change could be a bit confusing, because we've been calling `SetDefaultResource()` deprecated/discouraging it in reviews for so long. Renaming SGTM although I agree it will create quite a lot of churn.
source->SetDefaultResource(default_resource);Remove duplicate call?
// We intentionally do not use `SetDefaultResource` here as we do not want to
// serve index.html for non-HTML paths.Comment needs to be updated.
Related details
- Demetrios Papadopoulos
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. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 4 comments![]( "Open in Gerrit")
Rebekah PotterPTAL. WDYT?
I am also wondering if we should rename `SeDefaultResource()` to `SetRootResource()` for more clarity. Although this would create quite some churn,I think the name would be less confusing and more disassociated with the current name.
I was going to say that this change could be a bit confusing, because we've been calling `SetDefaultResource()` deprecated/discouraging it in reviews for so long. Renaming SGTM although I agree it will create quite a lot of churn.
I was going to say that this change could be a bit confusing, because we've been calling SetDefaultResource() deprecated/discouraging it in reviews for so long.
What is the alternative though? We could try to completely eliminate it and replace all usages in the other direciton (Replace `SetDefaultResource()` -> `AddResourcePath("", ...)`). My initial thinking was that this might be a harder API to get right than still providing a dedicated method to do this.
I removed all the files that are not owned by ui/webui/PLATFORM_OWNERS from this CL, and will address in follow-ups, to make it easier to land as it would require too many reviewers. Planning to actually send CLs to update the ownership to make it easier for the WebUI team to perform these horizontal changes.
PTAL as the +1 was invalidated.
source->SetDefaultResource(default_resource);Demetrios PapadopoulosRemove duplicate call?
Done
// We intentionally do not use `SetDefaultResource` here as we do not want to
// serve index.html for non-HTML paths.Comment needs to be updated.
- Rebekah Potter
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. |
Rebekah Potter (Gerrit)
Rebekah Potter voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rebekah Potter
| Code-Review | +1 |
Rebekah PotterPTAL. WDYT?
I am also wondering if we should rename `SeDefaultResource()` to `SetRootResource()` for more clarity. Although this would create quite some churn,I think the name would be less confusing and more disassociated with the current name.
Demetrios PapadopoulosI was going to say that this change could be a bit confusing, because we've been calling `SetDefaultResource()` deprecated/discouraging it in reviews for so long. Renaming SGTM although I agree it will create quite a lot of churn.
I was going to say that this change could be a bit confusing, because we've been calling SetDefaultResource() deprecated/discouraging it in reviews for so long.
What is the alternative though? We could try to completely eliminate it and replace all usages in the other direciton (Replace `SetDefaultResource()` -> `AddResourcePath("", ...)`). My initial thinking was that this might be a harder API to get right than still providing a dedicated method to do this.
I think the renaming would be the alternative (or something we could do in addition). My comment was supporting the renaming, not saying we shouldn't land this CL.
Replacing the other direction would be less confusing (previously deprecated method -> non deprecated method), but I agree the API is more awkward that way.
Related details
- Demetrios Papadopoulos
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos voted and added 1 comment![]( "Open in Gerrit")
Votes added by Demetrios Papadopoulos
| Commit-Queue | +2 |
Rebekah PotterPTAL. WDYT?
I am also wondering if we should rename `SeDefaultResource()` to `SetRootResource()` for more clarity. Although this would create quite some churn,I think the name would be less confusing and more disassociated with the current name.
Demetrios PapadopoulosI was going to say that this change could be a bit confusing, because we've been calling `SetDefaultResource()` deprecated/discouraging it in reviews for so long. Renaming SGTM although I agree it will create quite a lot of churn.
Rebekah PotterI was going to say that this change could be a bit confusing, because we've been calling SetDefaultResource() deprecated/discouraging it in reviews for so long.
What is the alternative though? We could try to completely eliminate it and replace all usages in the other direciton (Replace `SetDefaultResource()` -> `AddResourcePath("", ...)`). My initial thinking was that this might be a harder API to get right than still providing a dedicated method to do this.
I think the renaming would be the alternative (or something we could do in addition). My comment was supporting the renaming, not saying we shouldn't land this CL.
Replacing the other direction would be less confusing (previously deprecated method -> non deprecated method), but I agree the API is more awkward that way.
SG. I'll follow-up with the ranaming and using it as an opportunity to update the OWNERShip of any files that are not already owned by ui/webui/PLATFORM_OWNERRS.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
WebUI: Leverage the new WebUIDataSourceImpl#SetDefaultResource().
SetDefaultResource() semantics were recently changed and it can now be
deployed in all places that were previously intentionally bypassing it,
meaning places that were calling `source->AddResourcePath("", ...)`
- M ash/webui/annotator/untrusted_annotator_ui.cc
- M ash/webui/boca_receiver_app_ui/boca_receiver_ui.cc
- M ash/webui/boca_receiver_app_ui/boca_receiver_untrusted_ui.cc
- M ash/webui/boca_ui/boca_ui.cc
- M ash/webui/color_internals/color_internals_ui.cc
- M ash/webui/diagnostics_ui/diagnostics_ui.cc
- M ash/webui/eche_app_ui/eche_app_ui.cc
- M ash/webui/file_manager/file_manager_ui.cc
- M ash/webui/files_internals/files_internals_ui.cc
- M ash/webui/focus_mode/focus_mode_ui.cc
- M ash/webui/growth_internals/growth_internals_ui.cc
- M ash/webui/personalization_app/personalization_app_ui.cc
- M ash/webui/print_preview_cros/print_preview_cros_ui.cc
- M ash/webui/projector_app/untrusted_projector_ui.cc
- M ash/webui/recorder_app_ui/recorder_app_ui.cc
- M ash/webui/sample_system_web_app_ui/sample_system_web_app_ui.cc
- M ash/webui/sanitize_ui/sanitize_ui.cc
- M ash/webui/scanner_feedback_ui/scanner_feedback_untrusted_ui.cc
- M ash/webui/shimless_rma/shimless_rma.cc
- M ash/webui/status_area_internals/status_area_internals_ui.cc
- M ash/webui/vc_background_ui/vc_background_ui.cc
- M chrome/browser/ui/webui/ash/sys_internals/sys_internals_ui.cc
- M chrome/browser/ui/webui/download_internals/download_internals_ui.cc
- M chrome/browser/ui/webui/side_panel/read_anything/read_anything_untrusted_ui.cc
- M chrome/browser/ui/webui/user_actions/user_actions_ui.cc
- M chrome/browser/ui/webui/version/version_ui.cc
- M content/browser/webui/web_ui_data_source_unittest.cc
- M content/browser/webui/web_ui_mojo_browsertest.cc
- M content/browser/webui/web_ui_mojo_conformance_browsertest.cc
- M docs/webui/webui_explainer.md
- M ui/webui/examples/browser/ui/web/webui.cc
- M ui/webui/webui_util.cc
Code-Review: +1 by Rebekah Potter
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |