Create renderer side code for ai taxonomy API [chromium/src : main]
Nathan Memmott (Gerrit)
Nathan Memmott added 5 comments![]( "Open in Gerrit")
assert_equals(availability, 'unavailable');Assert that `availability` is one of the valid return values instead of asserting that it is unavailable.
}, 'Classifier.availability() is callable and returns "unavailable" in the stub implementation');Don't call out the implementation in the WPT names. These test the API.
try {
await Classifier.create({
signal: AbortSignal.timeout(1000)
});
assert_unreached('create() should not resolve because the backend is a stub.');
} catch (e) {
}So since you always return a `unavailable`, you should update your `create` function to return [a rejected promise](https://webmachinelearning.github.io/writing-assistance-apis/#create-an-ai-model-object). Then for this test, you should get the availability and assert that create should reject if it is unavailable. Also use [`promise_rejects_dom`](https://web-platform-tests.org/writing-tests/testharness-api.html#promise-rejection).
}, 'Classifier.create() executes the C++ layer without crashing');The implementation might not necessarily be written in C++.
<!DOCTYPE HTML>
<meta charset="utf-8">
<script src="/resources/testdriver.js"></script>
<script src="/resources/testdriver-vendor.js"></script>
<body></body>
<script>
test_driver.set_test_context(parent);
window.onmessage = async message => {
const { id, type } = message.data;
try {
switch (type) {
case 'ClassifierCreate':
await test_driver.bless('Classifier.create', Classifier.create, window);
parent.postMessage({ id, success: 'Success' }, '*');
break;
case 'ClassifierAvailability':
const availability = await Classifier.availability();
parent.postMessage({ id, success: availability }, '*');
break;
}
} catch (err) {
parent.postMessage({ id, err: err }, '*');
}
};
</script>
This is unused?
Related details
- Devin Cabillo
- Jingyun Liu
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. |
Devin Cabillo (Gerrit)
Devin Cabillo added 5 comments![]( "Open in Gerrit")
Assert that `availability` is one of the valid return values instead of asserting that it is unavailable.
Done
}, 'Classifier.availability() is callable and returns "unavailable" in the stub implementation');Don't call out the implementation in the WPT names. These test the API.
Done
try {
await Classifier.create({
signal: AbortSignal.timeout(1000)
});
assert_unreached('create() should not resolve because the backend is a stub.');
} catch (e) {
}So since you always return a `unavailable`, you should update your `create` function to return [a rejected promise](https://webmachinelearning.github.io/writing-assistance-apis/#create-an-ai-model-object). Then for this test, you should get the availability and assert that create should reject if it is unavailable. Also use [`promise_rejects_dom`](https://web-platform-tests.org/writing-tests/testharness-api.html#promise-rejection).
Done
}, 'Classifier.create() executes the C++ layer without crashing');The implementation might not necessarily be written in C++.
Done
<!DOCTYPE HTML>
<meta charset="utf-8">
<script src="/resources/testdriver.js"></script>
<script src="/resources/testdriver-vendor.js"></script>
<body></body>
<script>
test_driver.set_test_context(parent);
window.onmessage = async message => {
const { id, type } = message.data;
try {
switch (type) {
case 'ClassifierCreate':
await test_driver.bless('Classifier.create', Classifier.create, window);
parent.postMessage({ id, success: 'Success' }, '*');
break;
case 'ClassifierAvailability':
const availability = await Classifier.availability();
parent.postMessage({ id, success: availability }, '*');
break;
}
} catch (err) {
parent.postMessage({ id, err: err }, '*');
}
};
</script>
- Jingyun Liu
- Nathan Memmott
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nathan Memmott (Gerrit)
Nathan Memmott added 4 comments![]( "Open in Gerrit")
'NotAllowedError',This should be 'NotSupportedError'.
Classifier.create({ signal: AbortSignal.timeout(1000) })Remove this signal and return a rejected promise from create().
const classifier = await Classifier.create({ signal: AbortSignal.timeout(1000) });
assert_true(!!classifier, 'Classifier was successfully created');Remove the else case.
}, 'Classifier.create() behavior depends on availability');How about 'Classifier.create() rejects when unavailable'?
Related details
- Devin Cabillo
- Jingyun Liu
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. |
Devin Cabillo (Gerrit)
Devin Cabillo added 4 comments![]( "Open in Gerrit")
This should be 'NotSupportedError'.
Done
Classifier.create({ signal: AbortSignal.timeout(1000) })Remove this signal and return a rejected promise from create().
Done
const classifier = await Classifier.create({ signal: AbortSignal.timeout(1000) });
assert_true(!!classifier, 'Classifier was successfully created');Devin CabilloRemove the else case.
Done
}, 'Classifier.create() behavior depends on availability');How about 'Classifier.create() rejects when unavailable'?
- Jingyun Liu
- Nathan Memmott
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nathan Memmott (Gerrit)
Nathan Memmott voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nathan Memmott
| Code-Review | +1 |
1 comment
await promise_rejects_dom(Sorry by remove the else case, I meant to keep the true case. So check availability and if 'unavailable', assert that it rejects with a DOM error. Also can you add a TODO comment to update this to NotSupportedError with bug http://crbug.com/487291285.
Related details
- Devin Cabillo
- Jingyun Liu
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. |
Devin Cabillo (Gerrit)
Devin Cabillo added 1 comment![]( "Open in Gerrit")
Sorry by remove the else case, I meant to keep the true case. So check availability and if 'unavailable', assert that it rejects with a DOM error. Also can you add a TODO comment to update this to NotSupportedError with bug http://crbug.com/487291285.
- Jingyun Liu
- Nathan Memmott
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nathan Memmott (Gerrit)
Nathan Memmott voted Code-Review+1![]( "Open in Gerrit")
- Devin Cabillo
- Jingyun Liu
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: elly...@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): elly...@chromium.org
Reviewer source(s):
elly...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Elly FJ
- Jeremy Roman
- Jingyun Liu
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jingyun Liu (Gerrit)
Jingyun Liu voted Code-Review+1![]( "Open in Gerrit")
- Devin Cabillo
- Elly FJ
- Jeremy Roman
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jeremy Roman (Gerrit)
Jeremy Roman voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Devin Cabillo
- Elly FJ
- Nico Weber
Elly FJ (Gerrit)
Elly FJ added 3 comments![]( "Open in Gerrit")
hi!
for ipc review we generally prefer to see the interface and its browser-side implementation together - it's often impossible to assess the security impact of an IPC method without knowing at least roughly how it's going to work.
Create renderer side code for ai taxonomy API
Feature currently disabled and returns an empty promise
pending browser side implementation and linkthere isn't enough info either here or on the linked bug for me to do a meaningful IPC review. In particular:
- What is this API supposed to do?
- Who will call it and who will implement it?
- What data will be passed where and by whom?
- If text is being parsed (it sounds like it is), where does that occur?
// Defined locally to avoid touching ai_manager.mojom for now.why is it desirable to avoid touching ai_manager.mojom?
Related details
- Devin Cabillo
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Devin Cabillo (Gerrit)
Devin Cabillo added 2 comments![]( "Open in Gerrit")
Create renderer side code for ai taxonomy API
Feature currently disabled and returns an empty promise
pending browser side implementation and linkthere isn't enough info either here or on the linked bug for me to do a meaningful IPC review. In particular:
- What is this API supposed to do?
- Who will call it and who will implement it?
- What data will be passed where and by whom?
- If text is being parsed (it sounds like it is), where does that occur?
Hey Elly, here is the PRD and the Design:
https://docs.google.com/document/d/1O9bLaEjw2yPHJbaCj3hpJEsJv6AyG4ImoZ7GS1n_PZU/edit?resourcekey=0-inRjASFWJ8TQJitvYB3pmg&tab=t.8apz1rfusva5#heading=h.kfr7kq8gx8kd
What is this API supposed to do?
- It will be used as in devTrial for adds to perform Taxonomy on Webpages
Who will call it and who will implement it?
- It should be an open endpoint eventually for developers, but for now it will be a built in API to pass text to and provide IAB Taxonomies. Most of the logic will is stubbed out right now. I am working in parallel with some pending work and will begin implementing the browser process which will then pass the logic to optimization_guide
What data will be passed where and by whom?
- For the DevTrial we will be focused on the developer(Ads) passing a simple text string (the input to be classified) and an optional dictionary of settings
If text is being parsed (it sounds like it is), where does that occur?
- In the Blink (Renderer) layer, the text is treated strictly as a string. No linguistic parsing or tokenization occurs in the rendering engine. The string is serialized and passed across the Mojo IPC boundary to the Browser process. The actual "parsing" (tokenization, embedding, and model inference) happens safely inside the underlying AI execution engine
// Defined locally to avoid touching ai_manager.mojom for now.why is it desirable to avoid touching ai_manager.mojom?
The goal is to keep this CL strictly scoped to the Blink/Renderer API surface (IDL and bindings) and land the feature incrementally.
Touching ai_manager.mojom requires crossing the IPC boundary, which would force us to introduce browser-process boilerplate and stub implementations for the AIManager receivers just to satisfy the compiler. By keeping the stub entirely within Blink for now, we can land the web-facing API shape and unblock the Web Platform Tests independently. The Mojom changes and browser-process plumbing will be introduced in a focused follow-up cl alongside the actual backend integration.
Related details
- Elly FJ
- Nico Weber
Elly FJ (Gerrit)
Elly FJ added 1 comment![]( "Open in Gerrit")
// Defined locally to avoid touching ai_manager.mojom for now.Devin Cabillowhy is it desirable to avoid touching ai_manager.mojom?
The goal is to keep this CL strictly scoped to the Blink/Renderer API surface (IDL and bindings) and land the feature incrementally.
Touching ai_manager.mojom requires crossing the IPC boundary, which would force us to introduce browser-process boilerplate and stub implementations for the AIManager receivers just to satisfy the compiler. By keeping the stub entirely within Blink for now, we can land the web-facing API shape and unblock the Web Platform Tests independently. The Mojom changes and browser-process plumbing will be introduced in a focused follow-up cl alongside the actual backend integration.
Okay - this is fine, but please mark the methods with:
```
[RuntimeFeature=mojo_base.mojom.kMojomWorkInProgress]
```
so that there is no chance this code will become callable without another IPC review once it's implemented.
Related details
- Devin Cabillo
- Nico Weber
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. |
Devin Cabillo (Gerrit)
Devin Cabillo added 2 comments![]( "Open in Gerrit")
Create renderer side code for ai taxonomy API
Feature currently disabled and returns an empty promise
pending browser side implementation and linkDevin Cabillothere isn't enough info either here or on the linked bug for me to do a meaningful IPC review. In particular:
- What is this API supposed to do?
- Who will call it and who will implement it?
- What data will be passed where and by whom?
- If text is being parsed (it sounds like it is), where does that occur?
Hey Elly, here is the PRD and the Design:
https://docs.google.com/document/d/1O9bLaEjw2yPHJbaCj3hpJEsJv6AyG4ImoZ7GS1n_PZU/edit?resourcekey=0-inRjASFWJ8TQJitvYB3pmg&tab=t.8apz1rfusva5#heading=h.kfr7kq8gx8kdWhat is this API supposed to do?
- It will be used as in devTrial for adds to perform Taxonomy on Webpages
Who will call it and who will implement it?
- It should be an open endpoint eventually for developers, but for now it will be a built in API to pass text to and provide IAB Taxonomies. Most of the logic will is stubbed out right now. I am working in parallel with some pending work and will begin implementing the browser process which will then pass the logic to optimization_guide
What data will be passed where and by whom?
- For the DevTrial we will be focused on the developer(Ads) passing a simple text string (the input to be classified) and an optional dictionary of settings
If text is being parsed (it sounds like it is), where does that occur?
- In the Blink (Renderer) layer, the text is treated strictly as a string. No linguistic parsing or tokenization occurs in the rendering engine. The string is serialized and passed across the Mojo IPC boundary to the Browser process. The actual "parsing" (tokenization, embedding, and model inference) happens safely inside the underlying AI execution engine
Done
// Defined locally to avoid touching ai_manager.mojom for now.Devin Cabillowhy is it desirable to avoid touching ai_manager.mojom?
Elly FJThe goal is to keep this CL strictly scoped to the Blink/Renderer API surface (IDL and bindings) and land the feature incrementally.
Touching ai_manager.mojom requires crossing the IPC boundary, which would force us to introduce browser-process boilerplate and stub implementations for the AIManager receivers just to satisfy the compiler. By keeping the stub entirely within Blink for now, we can land the web-facing API shape and unblock the Web Platform Tests independently. The Mojom changes and browser-process plumbing will be introduced in a focused follow-up cl alongside the actual backend integration.
Okay - this is fine, but please mark the methods with:
```
[RuntimeFeature=mojo_base.mojom.kMojomWorkInProgress]
```so that there is no chance this code will become callable without another IPC review once it's implemented.
- Elly FJ
- Jeremy Roman
- Jingyun Liu
- Nathan Memmott
- Nico Weber
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: dch...@chromium.org, elly...@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): dch...@chromium.org, elly...@chromium.org
Reviewer source(s):
dch...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Daniel Cheng
- Elly FJ
- Jeremy Roman
- Jingyun Liu
- Nathan Memmott
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Elly FJ (Gerrit)
Elly FJ voted and added 1 comment![]( "Open in Gerrit")
- Daniel Cheng
- Devin Cabillo
- Jeremy Roman
- Jingyun Liu
- Nathan Memmott
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Nathan Memmott (Gerrit)
Nathan Memmott voted Code-Review+1![]( "Open in Gerrit")
- Daniel Cheng
- Devin Cabillo
- Jeremy Roman
- Jingyun Liu
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Devin Cabillo (Gerrit)
Devin Cabillo voted Commit-Queue+2![]( "Open in Gerrit")
- Daniel Cheng
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/58120.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Daniel Cheng (Gerrit)
Daniel Cheng added 8 comments![]( "Open in Gerrit")
void remoteExecute(Is this actually an IDL-exposed method? Its naming suggests that it is... but it seems like it's an implementation detail (in which case the proper name is `RemoteExecute`).
AIWritingAssistanceBase<Classifier,Not introduced by this CL, but this class could benefit from some high-level comments. I had to do a lot of background reading to start to understand the Blink plumbing.
// TODO(crbug.com/483707607): INTENTIONALLY EMPTY, Browser side code forI don't quite understand why this references the browser here; this code is in the renderer?
std::string function_name) {Don't pass std::string by value unless you're planning on taking ownership of the std::string here.
Unfortunately it's really hard to figure out what the right thing is here because so much of the functionality is missing in this CL...
// No-opI can't tell–is this a no-op because it's not implemented yet, or because it's always going to be a no-op?
if (!ai_manager_remote.is_connected()) {How can this happen? It's not clear to me why other places in Blink are checking if this is connected either. Mojo message pipes don't just randomly become disconnected.
task_runner,Nit:
```suggestion
std::move(task_runner),
```
// Intentionally empty.Similarly here: is this empty because it's a "to implement" kind of thing, or something else?
Related details
- Devin Cabillo
- Jeremy Roman
- Jingyun Liu
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Daniel Cheng (Gerrit)
Daniel Cheng added 2 comments![]( "Open in Gerrit")
Also, is there an intent to experiment/prototype/et cetera given that this is a web-exposed change?
const_cast<ClassifierCreateOptions*>(options),Just don't mark the parameter const so we don't need to cast away constness.
Devin Cabillo (Gerrit)
Devin Cabillo added 10 comments![]( "Open in Gerrit")
Also, is there an intent to experiment/prototype/et cetera given that this is a web-exposed change?
There is no Intent to Prototype (I2P) thread yet. Right now, we are strictly in an early technical exploration phase. The immediate goal is just to land the foundational plumbing behind a disabled-by-default runtime flag so we can plug in a model, evaluate its performance/viability locally, and finalize the API shape. Once we have validated the model and are actually ready to propose this for wider experimentation or an Origin Trial, we will absolutely send out the formal I2P to blink-dev.
Is this actually an IDL-exposed method? Its naming suggests that it is... but it seems like it's an implementation detail (in which case the proper name is `RemoteExecute`).
This is a virtual method inherited from AIWritingAssistanceBase. I've kept it as remoteExecute here to satisfy the override, but I will file a follow-up bug to rename it across all the AI APIs (Summarizer, Writer, etc.) so we don't balloon the scope of this specific CL.
Not introduced by this CL, but this class could benefit from some high-level comments. I had to do a lot of background reading to start to understand the Blink plumbing.
Done
// TODO(crbug.com/483707607): INTENTIONALLY EMPTY, Browser side code forI don't quite understand why this references the browser here; this code is in the renderer?
Done
Don't pass std::string by value unless you're planning on taking ownership of the std::string here.
Unfortunately it's really hard to figure out what the right thing is here because so much of the functionality is missing in this CL...
Also inherited from WritingAssistantBase, creating bug to address updates to WebAI apis using WritingAssistantBase.
I can't tell–is this a no-op because it's not implemented yet, or because it's always going to be a no-op?
Done
How can this happen? It's not clear to me why other places in Blink are checking if this is connected either. Mojo message pipes don't just randomly become disconnected.
Done
task_runner,Devin CabilloNit:
```suggestion
std::move(task_runner),
```
Done
Similarly here: is this empty because it's a "to implement" kind of thing, or something else?
Done
Just don't mark the parameter const so we don't need to cast away constness.
- Daniel Cheng
- Elly FJ
- Jeremy Roman
- Jingyun Liu
- Nathan Memmott
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nathan Memmott (Gerrit)
Nathan Memmott voted Code-Review+1![]( "Open in Gerrit")
- Daniel Cheng
- Devin Cabillo
- Elly FJ
- Jeremy Roman
- Jingyun Liu
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Jingyun Liu (Gerrit)
Jingyun Liu voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Daniel Cheng
- Devin Cabillo
- Elly FJ
- Jeremy Roman
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
Devin CabilloAlso, is there an intent to experiment/prototype/et cetera given that this is a web-exposed change?
There is no Intent to Prototype (I2P) thread yet. Right now, we are strictly in an early technical exploration phase. The immediate goal is just to land the foundational plumbing behind a disabled-by-default runtime flag so we can plug in a model, evaluate its performance/viability locally, and finalize the API shape. Once we have validated the model and are actually ready to propose this for wider experimentation or an Origin Trial, we will absolutely send out the formal I2P to blink-dev.
I talked offline with rbyers@ and we're OK to proceed as-is. Thanks.
Related details
Devin Cabillo (Gerrit)
Devin Cabillo voted Commit-Queue+2![]( "Open in Gerrit")
Nathan Memmott (Gerrit)
Nathan Memmott voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nathan Memmott
| Code-Review | -1 |
1 comment
Pausing this until further review.
Related details
- Devin Cabillo
- Elly FJ
- Jeremy Roman
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Nathan Memmott (Gerrit)
Nathan Memmott voted Code-Review+0![]( "Open in Gerrit")
| Code-Review | +0 |
Related details
- Devin Cabillo
- Elly FJ
- Jeremy Roman
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement