Create renderer side code for ai taxonomy API [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
Taxonomy(ScriptState* script_state,Blink Style Guide: Don't mix Create () factory methods and public constructors in one class. Since this class has a `create()` factory method, the constructor should be private or protected. If `MakeGarbageCollected` needs access, consider using the PassKey pattern or friendship.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
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")
Blink Style Guide: Don't mix Create () factory methods and public constructors in one class. Since this class has a `create()` factory method, the constructor should be private or protected. If `MakeGarbageCollected` needs access, consider using the PassKey pattern or friendship.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
OK But Won't Fix: The constructor must remain public because it is instantiated via std::make_unique<ContextBoundObjectType> within the AIManager template methods. Making it private breaks these instantiations.
Related details
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 14 comments![]( "Open in Gerrit")
Thanks for doing this! This mostly looks good despite all my nit picking. It really is just some minor things.
Don't forget to do a dry run.
Nit: Add a comment that links to the explainer similar to the other mojom files in this directory.
// Interface for AITaxonomy that can output a IAB categoryMy understanding is that IAB is just the default but there may be more taxonomies in the future.
// classifies text to IAB taxonomy categoriesNit: Capitalize
Classify(string input, string context, pending_remote<ModelStreamingResponder> pending_responder);Nit: Don't exceed 80chars
MeasureUsage(string input, string context) => (uint32? number_of_tokens);Remove
static ScriptPromise<Taxonomy> create(ScriptState* script_state,
const TaxonomyCreateOptions* options,
ExceptionState& exception_state);I believe this can be removed since its defined on the base class. Same with destroy.
// INTENTIONALLY EMPTY: Do not want linking errors with the browser.For all these comments marking remaining work, add TODO comments. Either filing individual bugs using an umbrella bug.
// Availability currently set to unavailableNit: Comments should end with punctuation.
AIMetrics::AISessionType TaxonomyBase::GetSessionType() {Optional nit: So I'm used to putting `// static` before all static functions in the source file. But I checked whether this is recommended and found http://go/c-readability-advice#static-comments. It says to prefer not to use them but be consistent with surrounding code. I'm not sure how to interpret that, but our "surrounding code" might include all the files in this folder which do this. Personally I see the value in having them.
DOMString context;Do you know if we want to have a context option? Or a TaxonomyCreateOptions sharedContext option?
[Can you file a bug to add use counter metrics to these functions? That way we can keep track of remaining work.
Promise<DOMString> classify(The explainer has this as categorize?
{
name: "AITaxonomyAPIForWorkers",
public: true,
},I think we can leave it out for now until we know its needed.
Related details
- Devin Cabillo
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 13 comments![]( "Open in Gerrit")
Nit: Add a comment that links to the explainer similar to the other mojom files in this directory.
Done
// Interface for AITaxonomy that can output a IAB categoryMy understanding is that IAB is just the default but there may be more taxonomies in the future.
Done
// classifies text to IAB taxonomy categoriesDevin CabilloNit: Capitalize
Done
Classify(string input, string context, pending_remote<ModelStreamingResponder> pending_responder);Nit: Don't exceed 80chars
Done
MeasureUsage(string input, string context) => (uint32? number_of_tokens);Devin CabilloRemove
Done
static ScriptPromise<Taxonomy> create(ScriptState* script_state,
const TaxonomyCreateOptions* options,
ExceptionState& exception_state);I believe this can be removed since its defined on the base class. Same with destroy.
Done
// INTENTIONALLY EMPTY: Do not want linking errors with the browser.For all these comments marking remaining work, add TODO comments. Either filing individual bugs using an umbrella bug.
Done
Nit: Comments should end with punctuation.
Done
AIMetrics::AISessionType TaxonomyBase::GetSessionType() {Optional nit: So I'm used to putting `// static` before all static functions in the source file. But I checked whether this is recommended and found http://go/c-readability-advice#static-comments. It says to prefer not to use them but be consistent with surrounding code. I'm not sure how to interpret that, but our "surrounding code" might include all the files in this folder which do this. Personally I see the value in having them.
Done
Do you know if we want to have a context option? Or a TaxonomyCreateOptions sharedContext option?
Done
Can you file a bug to add use counter metrics to these functions? That way we can keep track of remaining work.
Done
The explainer has this as categorize?
Done
name: "AITaxonomyAPIForWorkers",
public: true,
},I think we can leave it out for now until we know its needed.
- Nathan Memmott
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 added 5 comments![]( "Open in Gerrit")
// Return kWriter to avoid modifying ai_metrics.h (renderer-only hack)nit: since you're reusing stuff from other API, can you add a TODO here and in GetPermissionsPolicy, so we don't forget later to update those?
dictionary TaxonomizerCreateCoreOptions {
sequence<DOMString> expectedInputLanguages;
sequence<DOMString> expectedContextLanguages;
DOMString outputLanguage;
};
let's remove these and keep options minimal since we're unclear what this API will look like in the future?
For now we only need CreateOptions with ` AbortSignal signal; CreateMonitorCallback monitor;` ( sharedContext might be needed in the next iteration so it's okay to keep, as long as we know it's not actually implemented yet)
DOMString sharedContext;nit: sharedContext and context are not needed for dev trial. Maybe it's clearer if we don't include them in the *Options at all (vs. declare them but acutally no-op)
Promise<DOMString> categorize(the output would be an array of structured output. e.g,
```
[
{ id: "602", confidence: 0.98 }, // Represents "Consumer Electronics"
{ id: "597", confidence: 0.95 }, // Represents "Technology & Computing"
{ id: "45", confidence: 0.82 } // Represents "Automotive"
]
```
But that requires some work on the ModelStreamingResponder to parse the buffered outputs into a json object and return.
So I think for dev trial perhaps it's okay to simply add a note saying the output will be a json string, and ask the client manually do `JSON.parse()` over the output. For OT we might want to parse and return the object on our side.
Related details
- Devin Cabillo
- Nathan Memmott
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. |
Nathan Memmott (Gerrit)
Nathan Memmott voted and added 5 comments![]( "Open in Gerrit")
Votes added by Nathan Memmott
| Code-Review | +1 |
5 comments
It looks like you'll need to update webexposed/global-interface-listing-expected.txt to get the CQ to pass.
// Defined locally to avoid touching ai_manager.mojom for nowNit: End sentences with punctuation here and elsewhere.
// `quota_error_info` may provide additional info for k an AITaxonomizer.Nit: Copy mistake? Looks like others have this as `kInitialInputTooLarge`.
// Availability currently set to unavailable.Nit: Add a TODO here and for the other comments in this file that mark remaining work.
// TODO(crbug.com/484080220): create api metrics.Nit: Capitalize
Related details
- Devin Cabillo
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. |
Jingyun Liu (Gerrit)
Jingyun Liu added 1 comment![]( "Open in Gerrit")
struct AITaxonomizerCreateOptions {if you're adding this file, might as well update third_party/blink/public/mojom/ai/ai_manager.mojom to include CanCreateTaxonomizer and CreateTaxonomizer.
Devin Cabillo (Gerrit)
Devin Cabillo added 9 comments![]( "Open in Gerrit")
if you're adding this file, might as well update third_party/blink/public/mojom/ai/ai_manager.mojom to include CanCreateTaxonomizer and CreateTaxonomizer.
I am not doing that in the scope of this cl since that would trigger tests that require the browser side code.
// Defined locally to avoid touching ai_manager.mojom for nowNit: End sentences with punctuation here and elsewhere.
Done
// `quota_error_info` may provide additional info for k an AITaxonomizer.Nit: Copy mistake? Looks like others have this as `kInitialInputTooLarge`.
Done
Nit: Add a TODO here and for the other comments in this file that mark remaining work.
Done
// Return kWriter to avoid modifying ai_metrics.h (renderer-only hack)nit: since you're reusing stuff from other API, can you add a TODO here and in GetPermissionsPolicy, so we don't forget later to update those?
Done
dictionary TaxonomizerCreateCoreOptions {
sequence<DOMString> expectedInputLanguages;
sequence<DOMString> expectedContextLanguages;
DOMString outputLanguage;
};
let's remove these and keep options minimal since we're unclear what this API will look like in the future?
For now we only need CreateOptions with ` AbortSignal signal; CreateMonitorCallback monitor;` ( sharedContext might be needed in the next iteration so it's okay to keep, as long as we know it's not actually implemented yet)
Since we are currently using the AIWritingAssistantBase, all of these createOptions are necessary for creating a Taxonomizer instance.
nit: sharedContext and context are not needed for dev trial. Maybe it's clearer if we don't include them in the *Options at all (vs. declare them but acutally no-op)
Acknowledged
// TODO(crbug.com/484080220): create api metrics.Devin CabilloNit: Capitalize
Done
the output would be an array of structured output. e.g,
```
[
{ id: "602", confidence: 0.98 }, // Represents "Consumer Electronics"
{ id: "597", confidence: 0.95 }, // Represents "Technology & Computing"
{ id: "45", confidence: 0.82 } // Represents "Automotive"
]
```But that requires some work on the ModelStreamingResponder to parse the buffered outputs into a json object and return.
So I think for dev trial perhaps it's okay to simply add a note saying the output will be a json string, and ask the client manually do `JSON.parse()` over the output. For OT we might want to parse and return the object on our side.
- 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 3 comments![]( "Open in Gerrit")
// Availability currently set to unavailable.Nit: This comment would be removed once the TODO below is resolved, so we don't need it.
The TODO is also inaccurate since our goal isn't to set it to available, it's to report the correct availability status.
Similarly for the permissions TODO below.
dictionary TaxonomizerCreateCoreOptions {
sequence<DOMString> expectedInputLanguages;
sequence<DOMString> expectedContextLanguages;
DOMString outputLanguage;
};
Devin Cabillolet's remove these and keep options minimal since we're unclear what this API will look like in the future?
For now we only need CreateOptions with ` AbortSignal signal; CreateMonitorCallback monitor;` ( sharedContext might be needed in the next iteration so it's okay to keep, as long as we know it's not actually implemented yet)
Since we are currently using the AIWritingAssistantBase, all of these createOptions are necessary for creating a Taxonomizer instance.
Hmm then that might mean that we shouldn't be using the `AIWritingAssistantBase` or we need to revise it so that these aren't required.
DOMString sharedContext;Devin Cabillonit: sharedContext and context are not needed for dev trial. Maybe it's clearer if we don't include them in the *Options at all (vs. declare them but acutally no-op)
Acknowledged
Can you address this comment? I think it would make sense to remove it as well.
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")
DOMString sharedContext;Devin Cabillonit: sharedContext and context are not needed for dev trial. Maybe it's clearer if we don't include them in the *Options at all (vs. declare them but acutally no-op)
Nathan MemmottAcknowledged
Can you address this comment? I think it would make sense to remove it as well.
It is the same as the comment above. It is required if we are using the AIWritingAssistantBase
Related details
- Jingyun Liu
- Nathan Memmott
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. |
Jingyun Liu (Gerrit)
Jingyun Liu added 1 comment![]( "Open in Gerrit")
DOMString sharedContext;Devin Cabillonit: sharedContext and context are not needed for dev trial. Maybe it's clearer if we don't include them in the *Options at all (vs. declare them but acutally no-op)
Nathan MemmottAcknowledged
Devin CabilloCan you address this comment? I think it would make sense to remove it as well.
It is the same as the comment above. It is required if we are using the AIWritingAssistantBase
hmm now I'm starting to wonder if we should stick to WritingAssistantBase at all.
Alternatively, we could do smth similar to what [Proofreader](https://crsrc.org/c/third_party/blink/public/mojom/ai/ai_proofreader.mojom?q=third_party%2Fblink%2Fpublic%2Fmojom%2Fai%2Fai_proofreader.mojom&ss=chromium%2Fchromium%2Fsrc) does (still uses streamResponder, [parse text into dict in blink](https://crsrc.org/c/third_party/blink/renderer/modules/ai/proofreader.cc;drc=9014449dead52c636db6ca4a4e5b8a62be883e45;l=540), and doesn't use WritingAssistantBase)
Related details
- Devin Cabillo
- Nathan Memmott
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. |
Jingyun Liu (Gerrit)
Jingyun Liu added 1 comment![]( "Open in Gerrit")
dictionary TaxonomizerCreateCoreOptions {
sequence<DOMString> expectedInputLanguages;
sequence<DOMString> expectedContextLanguages;
DOMString outputLanguage;
};
Devin Cabillolet's remove these and keep options minimal since we're unclear what this API will look like in the future?
For now we only need CreateOptions with ` AbortSignal signal; CreateMonitorCallback monitor;` ( sharedContext might be needed in the next iteration so it's okay to keep, as long as we know it's not actually implemented yet)
Nathan MemmottSince we are currently using the AIWritingAssistantBase, all of these createOptions are necessary for creating a Taxonomizer instance.
Hmm then that might mean that we shouldn't be using the `AIWritingAssistantBase` or we need to revise it so that these aren't required.
+1, we could start with not using it, then if after dev trial or OT we need to support those common parameters/functionalities, we can come back and refactor WritingAssistantBase to support taxonomy.
`AIWritingAssistanceCreateClient` and `CreateModelExecutionResponder` can still be reused I think.
Devin Cabillo (Gerrit)
Devin Cabillo added 3 comments![]( "Open in Gerrit")
Nit: This comment would be removed once the TODO below is resolved, so we don't need it.
The TODO is also inaccurate since our goal isn't to set it to available, it's to report the correct availability status.
Similarly for the permissions TODO below.
Done
dictionary TaxonomizerCreateCoreOptions {
sequence<DOMString> expectedInputLanguages;
sequence<DOMString> expectedContextLanguages;
DOMString outputLanguage;
};
Devin Cabillolet's remove these and keep options minimal since we're unclear what this API will look like in the future?
For now we only need CreateOptions with ` AbortSignal signal; CreateMonitorCallback monitor;` ( sharedContext might be needed in the next iteration so it's okay to keep, as long as we know it's not actually implemented yet)
Nathan MemmottSince we are currently using the AIWritingAssistantBase, all of these createOptions are necessary for creating a Taxonomizer instance.
Jingyun LiuHmm then that might mean that we shouldn't be using the `AIWritingAssistantBase` or we need to revise it so that these aren't required.
+1, we could start with not using it, then if after dev trial or OT we need to support those common parameters/functionalities, we can come back and refactor WritingAssistantBase to support taxonomy.
`AIWritingAssistanceCreateClient` and `CreateModelExecutionResponder` can still be reused I think.
What is your opinion of the new changes? I was able to take out the unnecessary language configurations and stub out the core options for the AIWritingAssistant template
DOMString sharedContext;Devin Cabillonit: sharedContext and context are not needed for dev trial. Maybe it's clearer if we don't include them in the *Options at all (vs. declare them but acutally no-op)
Nathan MemmottAcknowledged
Devin CabilloCan you address this comment? I think it would make sense to remove it as well.
Jingyun LiuIt is the same as the comment above. It is required if we are using the AIWritingAssistantBase
hmm now I'm starting to wonder if we should stick to WritingAssistantBase at all.
Alternatively, we could do smth similar to what [Proofreader](https://crsrc.org/c/third_party/blink/public/mojom/ai/ai_proofreader.mojom?q=third_party%2Fblink%2Fpublic%2Fmojom%2Fai%2Fai_proofreader.mojom&ss=chromium%2Fchromium%2Fsrc) does (still uses streamResponder, [parse text into dict in blink](https://crsrc.org/c/third_party/blink/renderer/modules/ai/proofreader.cc;drc=9014449dead52c636db6ca4a4e5b8a62be883e45;l=540), and doesn't use WritingAssistantBase)
- Jingyun Liu
- Nathan Memmott
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. |
Jingyun Liu (Gerrit)
Jingyun Liu added 3 comments![]( "Open in Gerrit")
thank you! lgtm, minus the extra blink mojo options,
RE API renaming (https://github.com/explainers-by-googlers/taxonomy-api/issues/4)
once confirmed with Kenji i think we can s/taxonomizer/classifier everywhere including the filenames?
string? shared_context;
array<AILanguageCode> expected_input_languages;
array<AILanguageCode> expected_context_languages;do we still need these, now that they're removed from IDL?
DOMString sharedContext;Devin Cabillonit: sharedContext and context are not needed for dev trial. Maybe it's clearer if we don't include them in the *Options at all (vs. declare them but acutally no-op)
Nathan MemmottAcknowledged
Devin CabilloCan you address this comment? I think it would make sense to remove it as well.
Jingyun LiuIt is the same as the comment above. It is required if we are using the AIWritingAssistantBase
Devin Cabillohmm now I'm starting to wonder if we should stick to WritingAssistantBase at all.
Alternatively, we could do smth similar to what [Proofreader](https://crsrc.org/c/third_party/blink/public/mojom/ai/ai_proofreader.mojom?q=third_party%2Fblink%2Fpublic%2Fmojom%2Fai%2Fai_proofreader.mojom&ss=chromium%2Fchromium%2Fsrc) does (still uses streamResponder, [parse text into dict in blink](https://crsrc.org/c/third_party/blink/renderer/modules/ai/proofreader.cc;drc=9014449dead52c636db6ca4a4e5b8a62be883e45;l=540), and doesn't use WritingAssistantBase)
I was able to remove unnecessary options
- Devin Cabillo
- Nathan Memmott
Nathan Memmott (Gerrit)
Nathan Memmott added 7 comments![]( "Open in Gerrit")
class MODULES_EXPORT Taxonomizer finalWhy is this needed?
TaxonomizerCreateCoreOptionsStub* unused_options,Do `/*options*/` instead: https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions
if (!execution_context->IsFeatureEnabled(GetPermissionsPolicy())) {Add `// Return unavailable if the Permission Policy is not enabled.` back.
if (!execution_context->IsFeatureEnabled(GetPermissionsPolicy())) {Add back comment `// Block access if the Permission Policy is not enabled.`
optional TaxonomizerCreateOptions options = {}I would just get rid of `options` altogether. We don't want to pass `TaxonomizerCreateOptions` cause it has an abort signal which allows you to abort the operation, and a monitor which tracks the download progress. Neither apply to checking availability.
Related details
- Devin Cabillo
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 8 comments![]( "Open in Gerrit")
string? shared_context;
array<AILanguageCode> expected_input_languages;
array<AILanguageCode> expected_context_languages;do we still need these, now that they're removed from IDL?
Done
class MODULES_EXPORT Taxonomizer finalDevin CabilloWhy is this needed?
Done
Do `/*options*/` instead: https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions
Done
if (!execution_context->IsFeatureEnabled(GetPermissionsPolicy())) {Add `// Return unavailable if the Permission Policy is not enabled.` back.
Done
if (!execution_context->IsFeatureEnabled(GetPermissionsPolicy())) {Add back comment `// Block access if the Permission Policy is not enabled.`
Done
Exposed=(Window,Worker),Devin CabilloRemove Worker.
Done
I would just get rid of `options` altogether. We don't want to pass `TaxonomizerCreateOptions` cause it has an abort signal which allows you to abort the operation, and a monitor which tracks the download progress. Neither apply to checking availability.
Done
Related details
- Jingyun Liu
- Nathan Memmott
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")
thank you! lgtm, minus the extra blink mojo options,
RE API renaming (https://github.com/explainers-by-googlers/taxonomy-api/issues/4)
once confirmed with Kenji i think we can s/taxonomizer/classifier everywhere including the filenames?
Done
dictionary TaxonomizerCreateCoreOptions {
sequence<DOMString> expectedInputLanguages;
sequence<DOMString> expectedContextLanguages;
DOMString outputLanguage;
};
Devin Cabillolet's remove these and keep options minimal since we're unclear what this API will look like in the future?
For now we only need CreateOptions with ` AbortSignal signal; CreateMonitorCallback monitor;` ( sharedContext might be needed in the next iteration so it's okay to keep, as long as we know it's not actually implemented yet)
Nathan MemmottSince we are currently using the AIWritingAssistantBase, all of these createOptions are necessary for creating a Taxonomizer instance.
Jingyun LiuHmm then that might mean that we shouldn't be using the `AIWritingAssistantBase` or we need to revise it so that these aren't required.
Devin Cabillo+1, we could start with not using it, then if after dev trial or OT we need to support those common parameters/functionalities, we can come back and refactor WritingAssistantBase to support taxonomy.
`AIWritingAssistanceCreateClient` and `CreateModelExecutionResponder` can still be reused I think.
What is your opinion of the new changes? I was able to take out the unnecessary language configurations and stub out the core options for the AIWritingAssistant template
Done
Related details
- 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 |
- 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. |
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
Nathan Memmott (Gerrit)
Nathan Memmott voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Devin Cabillo (Gerrit)
Devin Cabillo voted Commit-Queue+2![]( "Open in Gerrit")
Devin Cabillo (Gerrit)
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Create renderer side code for ai taxonomy API
Implementation stubbed out in accordance to go/chrome-ipc-stubs-trial
Feature currently disabled and returns an empty promise
pending browser side implementation and link
What is this API supposed to do?
- It will be used in devTrial for adds to perform Taxonomy on Webpages. Strings will be passed through this layer (blink) and then end up on the ai engine within optimization_guide to classify text into IAB taxonomies using a tinyGemma model.
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. Browser process will eventually pass the logic to optimization_guide to perform inference
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
Bug: 483707964
Change-Id: I93f53ebb5d06c86a84a21f104b2e53ff78d8dedf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7568249
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Commit-Queue: Devin Cabillo <devinc...@google.com>
Reviewed-by: Jingyun Liu <jin...@google.com>
Reviewed-by: Nathan Memmott <mem...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1595586}
- M third_party/blink/public/mojom/BUILD.gn
- A third_party/blink/public/mojom/ai/ai_classifier.mojom
- M third_party/blink/renderer/bindings/generated_in_modules.gni
- M third_party/blink/renderer/bindings/idl_in_modules.gni
- M third_party/blink/renderer/modules/ai/BUILD.gn
- A third_party/blink/renderer/modules/ai/classifier.cc
- A third_party/blink/renderer/modules/ai/classifier.h
- A third_party/blink/renderer/modules/ai/classifier.idl
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- A third_party/blink/web_tests/external/wpt/ai/classifier/classifier-availability.tentative.https.window.js
- A third_party/blink/web_tests/external/wpt/ai/classifier/classifier-create.tentative.https.window.js
- M third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt
Code-Review: +1 by Nathan Memmott, +1 by Daniel Cheng, +1 by Jingyun Liu
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/58120
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |