[Built-In AI APIs] Enable AI Origin Trial APIs in DevTools. [chromium/src : main]
Isaac Ahouma (Gerrit)
Isaac Ahouma added 1 comment![]( "Open in Gerrit")
BASE_FEATURE(kDevToolsAiOriginTrialsApis, base::FEATURE_DISABLED_BY_DEFAULT);Isaac AhoumaWhy is the `kDevToolsAiOriginTrialsApis` here in devtools required? (For our end state, we don't want the developers to have to go to devtools, turn on a flag to enable this behaviour). Is it just because we are flag guarding this? If yes, should we make it true?
Etienne NoëlThere are a couple of reasons why I think the flag is required:
1. To control when the OT APIs are enabled. From what I understood, we only want to enable them for some origins but not all.
2. Testability. Using a base::Feature flag allows us to easily control the enablement state in browser tests (devtools_browsertest.cc) using base::test::ScopedFeatureList. This enables us to write tests that verify if APIs are present in the DevTools JavaScript context when the flag is on and absent when it's off.
3. IIUC third_party/blink/renderer/core/exported/web_view_impl.cc is used by all renderer processes,not just the DevTools renderer. So if the web preference is not guarder by a flag, then our change in web_view_impl.cc would enable the OT APIs in all renderers, not just DevTools (since in chrome/browser/chrome_content_browser_client.cc we would always set the preference to true).```Is it just because we are flag guarding this? If yes, should we make it true?```
This relate to 3. above, if we make this true by default then I think the OT APIs will be enabled for all renderers by default. (If my understanding of web_view_impl.cc and chrome_content_browser_client.cc is correct)
Isaac AhoumaThank you for the thorough investigation. So our goal, is for a Chrome Extension that creates a DevTools Panel (or any DevTools features), to have the OT enabled for them automatically. It was my understanding that the check on `if (web_contents->GetVisibleURL().SchemeIs(content::kChromeDevToolsScheme)) {` was ensuring that only renderers in DevTools would return `true` to this. Am I misunderstanding this?
Having developers to manually enable a flag in DevTools defeats the purpose of this CL since devs can already turn on the Chrome Feature Flag. The goal is to avoid having them do that. Not sure if the purpose or intent is clear, let me know.
Yes, you are right regarding your understanding of `if (web_contents->GetVisibleURL().SchemeIs(content::kChromeDevToolsScheme)) {`. I didn't understand that we wanted to make the APIs available in all devtools schemes by default, hence why the flight was required.
I have removed the flag and updated the tests accordingly. I have tested locally but need for online tryjobs to finish to confirm everything is working.
One caveat with this approach is that we are unable to add browser tests to verify when the APIs should be disabled.
Related details
- Etienne Noël
- Mike Wasserman
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. |
Etienne Noël (Gerrit)
Etienne Noël added 1 comment![]( "Open in Gerrit")
BASE_FEATURE(kDevToolsAiOriginTrialsApis, base::FEATURE_DISABLED_BY_DEFAULT);Isaac AhoumaWhy is the `kDevToolsAiOriginTrialsApis` here in devtools required? (For our end state, we don't want the developers to have to go to devtools, turn on a flag to enable this behaviour). Is it just because we are flag guarding this? If yes, should we make it true?
Etienne NoëlThere are a couple of reasons why I think the flag is required:
1. To control when the OT APIs are enabled. From what I understood, we only want to enable them for some origins but not all.
2. Testability. Using a base::Feature flag allows us to easily control the enablement state in browser tests (devtools_browsertest.cc) using base::test::ScopedFeatureList. This enables us to write tests that verify if APIs are present in the DevTools JavaScript context when the flag is on and absent when it's off.
3. IIUC third_party/blink/renderer/core/exported/web_view_impl.cc is used by all renderer processes,not just the DevTools renderer. So if the web preference is not guarder by a flag, then our change in web_view_impl.cc would enable the OT APIs in all renderers, not just DevTools (since in chrome/browser/chrome_content_browser_client.cc we would always set the preference to true).```Is it just because we are flag guarding this? If yes, should we make it true?```
This relate to 3. above, if we make this true by default then I think the OT APIs will be enabled for all renderers by default. (If my understanding of web_view_impl.cc and chrome_content_browser_client.cc is correct)
Isaac AhoumaThank you for the thorough investigation. So our goal, is for a Chrome Extension that creates a DevTools Panel (or any DevTools features), to have the OT enabled for them automatically. It was my understanding that the check on `if (web_contents->GetVisibleURL().SchemeIs(content::kChromeDevToolsScheme)) {` was ensuring that only renderers in DevTools would return `true` to this. Am I misunderstanding this?
Having developers to manually enable a flag in DevTools defeats the purpose of this CL since devs can already turn on the Chrome Feature Flag. The goal is to avoid having them do that. Not sure if the purpose or intent is clear, let me know.
Yes, you are right regarding your understanding of `if (web_contents->GetVisibleURL().SchemeIs(content::kChromeDevToolsScheme)) {`. I didn't understand that we wanted to make the APIs available in all devtools schemes by default, hence why the flight was required.
I have removed the flag and updated the tests accordingly. I have tested locally but need for online tryjobs to finish to confirm everything is working.
One caveat with this approach is that we are unable to add browser tests to verify when the APIs should be disabled.
Ok thank you. I think this looks good but need people with better understanding of the code to review.
Why can't we add browser test to check that the API is disabled? Do you mean we can test that it's disabled inside `devtools` schemes?
Do we already ahve a test that checks that without an OT token, the APIs are disabled (in non `devtools` schemes)?
Related details
- Isaac Ahouma
- Mike Wasserman
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. |
Isaac Ahouma (Gerrit)
Isaac Ahouma added 1 comment![]( "Open in Gerrit")
BASE_FEATURE(kDevToolsAiOriginTrialsApis, base::FEATURE_DISABLED_BY_DEFAULT);Isaac AhoumaWhy is the `kDevToolsAiOriginTrialsApis` here in devtools required? (For our end state, we don't want the developers to have to go to devtools, turn on a flag to enable this behaviour). Is it just because we are flag guarding this? If yes, should we make it true?
Etienne NoëlThere are a couple of reasons why I think the flag is required:
1. To control when the OT APIs are enabled. From what I understood, we only want to enable them for some origins but not all.
2. Testability. Using a base::Feature flag allows us to easily control the enablement state in browser tests (devtools_browsertest.cc) using base::test::ScopedFeatureList. This enables us to write tests that verify if APIs are present in the DevTools JavaScript context when the flag is on and absent when it's off.
3. IIUC third_party/blink/renderer/core/exported/web_view_impl.cc is used by all renderer processes,not just the DevTools renderer. So if the web preference is not guarder by a flag, then our change in web_view_impl.cc would enable the OT APIs in all renderers, not just DevTools (since in chrome/browser/chrome_content_browser_client.cc we would always set the preference to true).```Is it just because we are flag guarding this? If yes, should we make it true?```
This relate to 3. above, if we make this true by default then I think the OT APIs will be enabled for all renderers by default. (If my understanding of web_view_impl.cc and chrome_content_browser_client.cc is correct)
Isaac AhoumaThank you for the thorough investigation. So our goal, is for a Chrome Extension that creates a DevTools Panel (or any DevTools features), to have the OT enabled for them automatically. It was my understanding that the check on `if (web_contents->GetVisibleURL().SchemeIs(content::kChromeDevToolsScheme)) {` was ensuring that only renderers in DevTools would return `true` to this. Am I misunderstanding this?
Having developers to manually enable a flag in DevTools defeats the purpose of this CL since devs can already turn on the Chrome Feature Flag. The goal is to avoid having them do that. Not sure if the purpose or intent is clear, let me know.
Etienne NoëlYes, you are right regarding your understanding of `if (web_contents->GetVisibleURL().SchemeIs(content::kChromeDevToolsScheme)) {`. I didn't understand that we wanted to make the APIs available in all devtools schemes by default, hence why the flight was required.
I have removed the flag and updated the tests accordingly. I have tested locally but need for online tryjobs to finish to confirm everything is working.
One caveat with this approach is that we are unable to add browser tests to verify when the APIs should be disabled.
Ok thank you. I think this looks good but need people with better understanding of the code to review.
Why can't we add browser test to check that the API is disabled? Do you mean we can test that it's disabled inside `devtools` schemes?
Do we already ahve a test that checks that without an OT token, the APIs are disabled (in non `devtools` schemes)?
Yes, I meant that with this approach its impossible to test that the APIs are disabled inside `devtools` and that's actually not necessary, since they should never be disabled. Our current test validates that the APIs are always enabled within `devtools`. (I was just reflecting on other tests as I usually like to cover all possible cases for testing, but this is really not relevant here and I shouldn't have mentioned that in my previous comment).
I would think we have tests for non-devtools. I know of our web platform tests and some of them should cover this, but I don't know enough to confirm. @m...@chromium.org Do you agree?
FWIW, this cl does not change the behavior of OT APIs in non-devtools renderers, so that behavior shouldn't be tested in this CL if it's not already done.
Related details
- Etienne Noël
- Mike Wasserman
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. |
Mike Wasserman (Gerrit)
Mike Wasserman added 9 comments![]( "Open in Gerrit")
several AI-related RuntimeEnabledFeatures are enabled in Blink, making
APIs like Proofreader, Writer, Rewriter, and LanguageModel available
within the DevTools frontend. The existing `ai_prompt_api_enabled`IIUC, this permits extension devtools panels to use the APIs even without an OT token? I would prefer if we could somehow still require OT tokens for extension panels. Otherwise, we're "shipping" these OT APIs to extensions, even though they might change shape or never ship.
if (base::FeatureList::IsEnabled(::features::kDevToolsAiPromptApi)) {
web_prefs->ai_prompt_api_enabled = true;
}
web_prefs->ai_ot_apis_enabled = true;If devtools folks are okay with the overall plan, I'd suggest:
- Remove ai_prompt_api_enabled and its flag, since the new pref is a superset
- Add an enabled-by-default feature flag for the new pref (as a killswitch)
ASSERT_TRUE(site_instance);nit: no need to assert before deref in tests
// Verifies that all AI OT APIs are enabled in DevTools when
// kDevToolsAiPromptApi is enabled.nit: Verifies the web preference is...
// Verifies that the AI OT APIs are enabled in DevTools by default.nit: Verifies the web preference is set true...
// Verifies that the OT APIs are not enabled for non-DevTools schemes.nit: Verifies the web preference is set false...
#include "third_party/blink/public/common/features.h"Move include to top of file
class DevToolsOriginTrialsApisEnabledBrowserTest : public InProcessBrowserTest {nit: replace subclass with `using DevToolsAIBrowserTest = InProcessBrowserTest` and just inline the ScopedFeatureList into the test fixture below.
// Enables the origin trial Built-in AI APIs, for use within DevTools.nit: add something about how this also pertains to devtools extension panels (even without OT tokens?).
Related details
- Etienne Noël
- Isaac Ahouma
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. |
Etienne Noël (Gerrit)
Etienne Noël added 1 comment![]( "Open in Gerrit")
several AI-related RuntimeEnabledFeatures are enabled in Blink, making
APIs like Proofreader, Writer, Rewriter, and LanguageModel available
within the DevTools frontend. The existing `ai_prompt_api_enabled`IIUC, this permits extension devtools panels to use the APIs even without an OT token? I would prefer if we could somehow still require OT tokens for extension panels. Otherwise, we're "shipping" these OT APIs to extensions, even though they might change shape or never ship.
I can test but I think we still need an OT since the scheme for extension isn't `devtools://`. I think this only enables it for the top level `devtools://` frame who can then propagate the permission policy to the children iframes. I might be wrong though.
Related details
- Isaac Ahouma
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. |
Isaac Ahouma (Gerrit)
Isaac Ahouma added 9 comments![]( "Open in Gerrit")
if (base::FeatureList::IsEnabled(::features::kDevToolsAiPromptApi)) {
web_prefs->ai_prompt_api_enabled = true;
}
web_prefs->ai_ot_apis_enabled = true;If devtools folks are okay with the overall plan, I'd suggest:
- Remove ai_prompt_api_enabled and its flag, since the new pref is a superset
- Add an enabled-by-default feature flag for the new pref (as a killswitch)
I applied the changes you suggested. Once I do get your approval, I will send the cl to the DevTools team so that we can get their opinion.
nit: no need to assert before deref in tests
Removed.
// Verifies that all AI OT APIs are enabled in DevTools when
// kDevToolsAiPromptApi is enabled.nit: Verifies the web preference is...
Done
// Verifies that the AI OT APIs are enabled in DevTools by default.nit: Verifies the web preference is set true...
Done
// Verifies that the OT APIs are not enabled for non-DevTools schemes.nit: Verifies the web preference is set false...
Done
#include "third_party/blink/public/common/features.h"Move include to top of file
Done
class DevToolsOriginTrialsApisEnabledBrowserTest : public InProcessBrowserTest {nit: replace subclass with `using DevToolsAIBrowserTest = InProcessBrowserTest` and just inline the ScopedFeatureList into the test fixture below.
The test crashes because the scoped feature list must be initialized in a fixture's constructor, hence why we need the class.
This is the error log:
```
[616102:616102:1209/182503.868134:FATAL:base/feature_list.cc:141] DCHECK failed: feature_overrides_allowed. FeatureList overrides must happen in the test constructor, before BrowserTestBase::SetUp() has run.
```
https://paste.googleplex.com/6166479475179520
@m...@chromium.org Can you confirm so that I can resolve this thread?
// Enables the origin trial Built-in AI APIs, for use within DevTools.nit: add something about how this also pertains to devtools extension panels (even without OT tokens?).
Done. Although as per Etienne, this doesn't apply to extentions without OT tokens, right?
Related details
- Etienne Noël
- Mike Wasserman
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. |
Isaac Ahouma (Gerrit)
Isaac Ahouma added 1 comment![]( "Open in Gerrit")
several AI-related RuntimeEnabledFeatures are enabled in Blink, making
APIs like Proofreader, Writer, Rewriter, and LanguageModel available
within the DevTools frontend. The existing `ai_prompt_api_enabled`Etienne NoëlIIUC, this permits extension devtools panels to use the APIs even without an OT token? I would prefer if we could somehow still require OT tokens for extension panels. Otherwise, we're "shipping" these OT APIs to extensions, even though they might change shape or never ship.
I can test but I think we still need an OT since the scheme for extension isn't `devtools://`. I think this only enables it for the top level `devtools://` frame who can then propagate the permission policy to the children iframes. I might be wrong though.
@etien...@chromium.org Were you able to confirm this change wouldn't change anything wrt OTs?
Mike Wasserman (Gerrit)
Mike Wasserman added 1 comment![]( "Open in Gerrit")
several AI-related RuntimeEnabledFeatures are enabled in Blink, making
APIs like Proofreader, Writer, Rewriter, and LanguageModel available
within the DevTools frontend. The existing `ai_prompt_api_enabled`Etienne NoëlIIUC, this permits extension devtools panels to use the APIs even without an OT token? I would prefer if we could somehow still require OT tokens for extension panels. Otherwise, we're "shipping" these OT APIs to extensions, even though they might change shape or never ship.
Isaac AhoumaI can test but I think we still need an OT since the scheme for extension isn't `devtools://`. I think this only enables it for the top level `devtools://` frame who can then propagate the permission policy to the children iframes. I might be wrong though.
@etien...@chromium.org Were you able to confirm this change wouldn't change anything wrt OTs?
I think this might be moot given https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/7232915
Related details
- Etienne Noël
- Isaac Ahouma
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. |
Etienne Noël (Gerrit)
Etienne Noël added 1 comment![]( "Open in Gerrit")
several AI-related RuntimeEnabledFeatures are enabled in Blink, making
APIs like Proofreader, Writer, Rewriter, and LanguageModel available
within the DevTools frontend. The existing `ai_prompt_api_enabled`Etienne NoëlIIUC, this permits extension devtools panels to use the APIs even without an OT token? I would prefer if we could somehow still require OT tokens for extension panels. Otherwise, we're "shipping" these OT APIs to extensions, even though they might change shape or never ship.
Isaac AhoumaI can test but I think we still need an OT since the scheme for extension isn't `devtools://`. I think this only enables it for the top level `devtools://` frame who can then propagate the permission policy to the children iframes. I might be wrong though.
Mike Wasserman@etien...@chromium.org Were you able to confirm this change wouldn't change anything wrt OTs?
I think this might be moot given https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/7232915
No it's necessary on top of. Else we can never activate OT because we never activate OT for devtools scheme, therefore it cannot pass the permission policy. The CL allows Summarizer and other to work since they are GA
Related details
- Isaac Ahouma
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. |
Isaac Ahouma (Gerrit)
Isaac Ahouma added 1 comment![]( "Open in Gerrit")
several AI-related RuntimeEnabledFeatures are enabled in Blink, making
APIs like Proofreader, Writer, Rewriter, and LanguageModel available
within the DevTools frontend. The existing `ai_prompt_api_enabled`Etienne NoëlIIUC, this permits extension devtools panels to use the APIs even without an OT token? I would prefer if we could somehow still require OT tokens for extension panels. Otherwise, we're "shipping" these OT APIs to extensions, even though they might change shape or never ship.
Isaac AhoumaI can test but I think we still need an OT since the scheme for extension isn't `devtools://`. I think this only enables it for the top level `devtools://` frame who can then propagate the permission policy to the children iframes. I might be wrong though.
Mike Wasserman@etien...@chromium.org Were you able to confirm this change wouldn't change anything wrt OTs?
Etienne NoëlI think this might be moot given https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/7232915
No it's necessary on top of. Else we can never activate OT because we never activate OT for devtools scheme, therefore it cannot pass the permission policy. The CL allows Summarizer and other to work since they are GA
Hi @d...@chromium.org, Elizabeth(esweeney@) mentioned that you might be a good POC on this issue. Can you please advise if this CL would work (or refer us to someone on the Devtools team)? Thanks!
Related details
- Danil Somsikov
- Etienne Noël
- Mike Wasserman
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. |
Danil Somsikov (Gerrit)
Danil Somsikov added 1 comment![]( "Open in Gerrit")
several AI-related RuntimeEnabledFeatures are enabled in Blink, making
APIs like Proofreader, Writer, Rewriter, and LanguageModel available
within the DevTools frontend. The existing `ai_prompt_api_enabled`Etienne NoëlIIUC, this permits extension devtools panels to use the APIs even without an OT token? I would prefer if we could somehow still require OT tokens for extension panels. Otherwise, we're "shipping" these OT APIs to extensions, even though they might change shape or never ship.
Isaac AhoumaI can test but I think we still need an OT since the scheme for extension isn't `devtools://`. I think this only enables it for the top level `devtools://` frame who can then propagate the permission policy to the children iframes. I might be wrong though.
Mike Wasserman@etien...@chromium.org Were you able to confirm this change wouldn't change anything wrt OTs?
Etienne NoëlI think this might be moot given https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/7232915
Isaac AhoumaNo it's necessary on top of. Else we can never activate OT because we never activate OT for devtools scheme, therefore it cannot pass the permission policy. The CL allows Summarizer and other to work since they are GA
Hi @d...@chromium.org, Elizabeth(esweeney@) mentioned that you might be a good POC on this issue. Can you please advise if this CL would work (or refer us to someone on the Devtools team)? Thanks!
I'd like Wolfgang to take a look when he's back unless this is very urgent.
Related details
- Etienne Noël
- Isaac Ahouma
- Mike Wasserman
- Wolfgang Beyer
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. |
Isaac Ahouma (Gerrit)
Isaac Ahouma added 1 comment![]( "Open in Gerrit")
several AI-related RuntimeEnabledFeatures are enabled in Blink, making
APIs like Proofreader, Writer, Rewriter, and LanguageModel available
within the DevTools frontend. The existing `ai_prompt_api_enabled`Etienne NoëlIIUC, this permits extension devtools panels to use the APIs even without an OT token? I would prefer if we could somehow still require OT tokens for extension panels. Otherwise, we're "shipping" these OT APIs to extensions, even though they might change shape or never ship.
Isaac AhoumaI can test but I think we still need an OT since the scheme for extension isn't `devtools://`. I think this only enables it for the top level `devtools://` frame who can then propagate the permission policy to the children iframes. I might be wrong though.
Mike Wasserman@etien...@chromium.org Were you able to confirm this change wouldn't change anything wrt OTs?
Etienne NoëlI think this might be moot given https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/7232915
Isaac AhoumaNo it's necessary on top of. Else we can never activate OT because we never activate OT for devtools scheme, therefore it cannot pass the permission policy. The CL allows Summarizer and other to work since they are GA
Danil SomsikovHi @d...@chromium.org, Elizabeth(esweeney@) mentioned that you might be a good POC on this issue. Can you please advise if this CL would work (or refer us to someone on the Devtools team)? Thanks!
I'd like Wolfgang to take a look when he's back unless this is very urgent.
Ack. Thanks Danil. I think we should be able to wait until @wo...@chromium.org is back from OOO.
Related details
- Danil Somsikov
- Etienne Noël
- Mike Wasserman
- Wolfgang Beyer