legion: Add mojo interface for webui [chromium/src : main]
Christian Dullweber (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Oleh Lamzin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Dullweber (Gerrit)
Christian Dullweber added 1 comment![]( "Open in Gerrit")
interface LegionInternalsPageHandler {Does this interface work for you or should we define this differently?
Related details
- Oleh Lamzin
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. |
Oleh Lamzin (Gerrit)
Oleh Lamzin added 9 comments![]( "Open in Gerrit")
string defaultUrl;should we also send API Key separately or it will be included in this URL?
Sending and displaying URL + API Key might be less error-prone for a user and they won't need to figure out in which format they should concatenate both
interface LegionInternalsPageHandler {Does this interface work for you or should we define this differently?
I think for PoC it works.
SendRequest(string request) => (string response);How do we propagate errors?
mojo::PendingReceiver<legion_internals::mojom::LegionInternalsPageHandler>nit: same, check headers
mojo::PendingReceiver<legion_internals::mojom::LegionInternalsPageHandler>nit: missing header
mojo::PendingReceiver<legion_internals::mojom::LegionInternalsPageHandler>nit: missing header
please check other files.
std::make_unique<LegionInternalsPageHandler>(std::move(receiver));nit: missing header
std::make_unique<LegionInternalsPageHandler>(std::move(receiver));nit: missing header
Related details
- Christian Dullweber
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. |
Oleh Lamzin (Gerrit)
Oleh Lamzin added 1 comment![]( "Open in Gerrit")
struct LegionInternals {Actually, we might just pass URL + API key as variables and therefore simplify Mojo interface:
https://source.chromium.org/chromium/chromium/src/+/main:docs/webui/webui_in_chrome.md;l=275-277?q=webui%20variable
Related details
- Christian Dullweber
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. |
Christian Dullweber (Gerrit)
Christian Dullweber voted and added 11 comments![]( "Open in Gerrit")
Votes added by Christian Dullweber
| Commit-Queue | +1 |
11 comments
Actually, we might just pass URL + API key as variables and therefore simplify Mojo interface:
https://source.chromium.org/chromium/chromium/src/+/main:docs/webui/webui_in_chrome.md;l=275-277?q=webui%20variable
Done
should we also send API Key separately or it will be included in this URL?
Sending and displaying URL + API Key might be less error-prone for a user and they won't need to figure out in which format they should concatenate both
Done
Oleh LamzinDoes this interface work for you or should we define this differently?
I think for PoC it works.
Acknowledged
SendRequest(string request) => (string response);How do we propagate errors?
I'm not quite sure what's the best approach here. We could just return them as a string "Error: <somemessage>".
mojo::PendingReceiver<legion_internals::mojom::LegionInternalsPageHandler>Christian Dullwebernit: same, check headers
Done
mojo::PendingReceiver<legion_internals::mojom::LegionInternalsPageHandler>nit: missing header
please check other files.
Done
mojo::PendingReceiver<legion_internals::mojom::LegionInternalsPageHandler>Christian Dullwebernit: missing header
Done
std::make_unique<LegionInternalsPageHandler>(std::move(receiver));Christian Dullwebernit: missing header
Done
std::make_unique<LegionInternalsPageHandler>(std::move(receiver));Christian Dullwebernit: missing header
Done
RunTest("legion_internals/legion_internals_test.js", "mocha.run()");I spent a lot of time trying to figure out how to add a simple test but somehow mocha is not found when executing this. I'm not quite sure what's going on but I'd suggest to just submit this and figure out this issue separately
Related details
- Oleh Lamzin
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. |
Oleh Lamzin (Gerrit)
Oleh Lamzin added 1 comment![]( "Open in Gerrit")
RunTest("legion_internals/legion_internals_test.js", "mocha.run()");I spent a lot of time trying to figure out how to add a simple test but somehow mocha is not found when executing this. I'm not quite sure what's going on but I'd suggest to just submit this and figure out this issue separately
- Christian Dullweber
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. |
Oleh Lamzin (Gerrit)
Oleh Lamzin added 1 comment![]( "Open in Gerrit")
SendRequest(string request) => (string response);Christian DullweberHow do we propagate errors?
I'm not quite sure what's the best approach here. We could just return them as a string "Error: <somemessage>".
wdyt about returning `{response?: string, error?: string}`?
Christian Dullweber (Gerrit)
Christian Dullweber added 2 comments![]( "Open in Gerrit")
SendRequest(string request) => (string response);Christian DullweberHow do we propagate errors?
Oleh LamzinI'm not quite sure what's the best approach here. We could just return them as a string "Error: <somemessage>".
wdyt about returning `{response?: string, error?: string}`?
Done
RunTest("legion_internals/legion_internals_test.js", "mocha.run()");Oleh LamzinI spent a lot of time trying to figure out how to add a simple test but somehow mocha is not found when executing this. I'm not quite sure what's going on but I'd suggest to just submit this and figure out this issue separately
SG. but then probably we should not land broken tests?
- Oleh Lamzin
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: cl...@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): cl...@chromium.org
Reviewer source(s):
cl...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Camille Lamy
- Nikhil Jakhar
- Oleh Lamzin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Oleh Lamzin (Gerrit)
Oleh Lamzin voted Code-Review+1![]( "Open in Gerrit")
- Camille Lamy
- Christian Dullweber
- Nikhil Jakhar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nikhil Jakhar (Gerrit)
Nikhil Jakhar added 1 comment![]( "Open in Gerrit")
});Can you add a .catch() block here so we can determine the error during debugging?
Related details
- Camille Lamy
- Christian Dullweber
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. |
Nikhil Jakhar (Gerrit)
Nikhil Jakhar voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |