[Summarizer API] Remove SummarizerAPI feature flag. [chromium/src : main]
Mike Wasserman (Gerrit)
Mike Wasserman added 10 comments![]( "Open in Gerrit")
// Returns whether the API is enabled by default.
bool IsAPIEnabledByDefault(std::string_view name) {
return name == "Summarizer";
}
I'd just keep this and optionally rename it to IsAPIStable or similar?
EXPECT_TRUE(content::ExecJs(tab, base::StringPrintf(kScript, token.data())));was this broken?
if (!kill_switch_active) {
// Handle standard feature enablement
if (IsAPIFlagEnabled(GetParam())) {The test somewhat intended to capture a config closer to production where users might enable flags while the kill switch is triggered. That's no longer happening with this conditional nesting.
enable_blink_features.push_back("AIPromptAPI");optional nit: consider GetAPIFlags and GetAPIWorkerFlags to parallel GetAPINames?
// Note: "AISummarizationAPI" is not added to kDisableBlinkFeatures
// because it's marked as "stable" and cannot be disabled this way.My understanding is that even APIs with runtime enabled feature entries can still be disabled by a finch killswitch config on their autogenerated base::Feature. Can you help resolve a conclusive answer here?
command_line->AppendSwitchASCII(
switches::kDisableBlinkFeatures,
base::JoinString(disable_blink_features, ","));The featurelist disablement without injecting a commandline flag was meant to match production finch killswitch behavior. Can we retain that pattern, or align even better with an actual finch disablement pattern?
// Stable feature, always exposed to window.As above, I believe a kill switch should take precedence over this.
if (!ExpectExposedToWindow(name)) {I think this should apply to summarizer too? If the feature is disabled, workers also shouldn't have access. This might have been incorrectly changed by removing the `RuntimeEnabled=AISummarizationAPI` idl guard
depends_on: ["AISummarizationAPI"],I hope an embedder granting this permissions policy feature to an iframe wouldn't actually enable the API if the blink::Feature::kAISummarizationAPI killswitch were triggered. Maybe we should leave this here? (and restore the one for Language Detector?). We're essentially just trying to remove the chrome://flag entry in these CLs.
RuntimeEnabled=AISummarizationAPI,As with languagedetection, i think we can keep this so long as the runtime enabled feature entry remains, as a base::Feature killswitch
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 10 comments![]( "Open in Gerrit")
// Returns whether the API is enabled by default.
bool IsAPIEnabledByDefault(std::string_view name) {
return name == "Summarizer";
}
I'd just keep this and optionally rename it to IsAPIStable or similar?
Done
EXPECT_TRUE(content::ExecJs(tab, base::StringPrintf(kScript, token.data())));Isaac Ahoumawas this broken?
No. Reverted this
if (!kill_switch_active) {
// Handle standard feature enablement
if (IsAPIFlagEnabled(GetParam())) {The test somewhat intended to capture a config closer to production where users might enable flags while the kill switch is triggered. That's no longer happening with this conditional nesting.
Done. I have updated the CL try retaining the original structure as much as possible.
enable_blink_features.push_back("AIPromptAPI");optional nit: consider GetAPIFlags and GetAPIWorkerFlags to parallel GetAPINames?
Done
// Note: "AISummarizationAPI" is not added to kDisableBlinkFeatures
// because it's marked as "stable" and cannot be disabled this way.My understanding is that even APIs with runtime enabled feature entries can still be disabled by a finch killswitch config on their autogenerated base::Feature. Can you help resolve a conclusive answer here?
Yes, you are correct that runtime enabled features can be [disabled by a Finch killswitch](https://chromium.googlesource.com/chromium/src/third_party/+/HEAD/blink/renderer/platform/RuntimeEnabledFeatures.md).
However, flags that have graduated to stable can not be disabled using a killswitch, which is what I was trying to highlight here. I have also followed up on this in a separate reply to check my understanding on runtime features and feature flags.
command_line->AppendSwitchASCII(
switches::kDisableBlinkFeatures,
base::JoinString(disable_blink_features, ","));The featurelist disablement without injecting a commandline flag was meant to match production finch killswitch behavior. Can we retain that pattern, or align even better with an actual finch disablement pattern?
Removed this.
// Stable feature, always exposed to window.As above, I believe a kill switch should take precedence over this.
I tried moving the kill switch check above this, but [tests failed](https://ci.chromium.org/ui/p/chromium/builders/try/win-rel/1285655/test-results).
It looks like the summarizer API is always ON and can not be disabled via the kill switch. It also seems that there's "an algorithm" that controls the API status in this testing context.
if (!ExpectExposedToWindow(name)) {I think this should apply to summarizer too? If the feature is disabled, workers also shouldn't have access. This might have been incorrectly changed by removing the `RuntimeEnabled=AISummarizationAPI` idl guard
Thanks, you are correct.
depends_on: ["AISummarizationAPI"],I hope an embedder granting this permissions policy feature to an iframe wouldn't actually enable the API if the blink::Feature::kAISummarizationAPI killswitch were triggered. Maybe we should leave this here? (and restore the one for Language Detector?). We're essentially just trying to remove the chrome://flag entry in these CLs.
I am a little confused by this comment. Since I am removing blink::Feature::kAISummarizationAPI in this CL (in chrome/browser/about_flags.cc0, doesn't that mean that the kill switch will not be able to disable that feature as it will always be ON?
RuntimeEnabled=AISummarizationAPI,As with languagedetection, i think we can keep this so long as the runtime enabled feature entry remains, as a base::Feature killswitch
- 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")
depends_on: ["AISummarizationAPI"],Isaac AhoumaI hope an embedder granting this permissions policy feature to an iframe wouldn't actually enable the API if the blink::Feature::kAISummarizationAPI killswitch were triggered. Maybe we should leave this here? (and restore the one for Language Detector?). We're essentially just trying to remove the chrome://flag entry in these CLs.
I am a little confused by this comment. Since I am removing blink::Feature::kAISummarizationAPI in this CL (in chrome/browser/about_flags.cc0, doesn't that mean that the kill switch will not be able to disable that feature as it will always be ON?
I think I got it now. The Blink feature is not the same as a Base feature. We can remove the Blink feature (which is the purpose of this CL)(https://chromium.googlesource.com/chromium/src/+/HEAD/docs/flag_guarding_guidelines.md). However, the base feature is autogenerated at runtime using https://source.corp.google.com/h/chromium/chromium/src/+/main:third_party/blink/renderer/platform/runtime_enabled_features.json5;l=448-45.
Having the base feature is [required](https://g3doc.corp.google.com/analysis/uma/g3doc/finch/feature-api.md?cl=head) for using the [killswitch](https://chromium.googlesource.com/chromium/src/+/HEAD/docs/flag_guarding_guidelines.md#prefer-waterfall-rollout-for-platform-changes). Is my understanding correct?
Related details
- 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 1 comment![]( "Open in Gerrit")
// Note: "AISummarizationAPI" is not added to kDisableBlinkFeatures
// because it's marked as "stable" and cannot be disabled this way.Isaac AhoumaMy understanding is that even APIs with runtime enabled feature entries can still be disabled by a finch killswitch config on their autogenerated base::Feature. Can you help resolve a conclusive answer here?
Yes, you are correct that runtime enabled features can be [disabled by a Finch killswitch](https://chromium.googlesource.com/chromium/src/third_party/+/HEAD/blink/renderer/platform/RuntimeEnabledFeatures.md).
However, flags that have graduated to stable can not be disabled using a killswitch, which is what I was trying to highlight here. I have also followed up on this in a separate reply to check my understanding on runtime features and feature flags.
Thanks for continuing to look into this WRT the runtime enabled feature entry missing the base feature override. I'll wait to hear what you find.
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. |
Mike Wasserman (Gerrit)
Mike Wasserman added 1 comment![]( "Open in Gerrit")
depends_on: ["AISummarizationAPI"],Isaac AhoumaI hope an embedder granting this permissions policy feature to an iframe wouldn't actually enable the API if the blink::Feature::kAISummarizationAPI killswitch were triggered. Maybe we should leave this here? (and restore the one for Language Detector?). We're essentially just trying to remove the chrome://flag entry in these CLs.
Isaac AhoumaI am a little confused by this comment. Since I am removing blink::Feature::kAISummarizationAPI in this CL (in chrome/browser/about_flags.cc0, doesn't that mean that the kill switch will not be able to disable that feature as it will always be ON?
I think I got it now. The Blink feature is not the same as a Base feature. We can remove the Blink feature (which is the purpose of this CL)(https://chromium.googlesource.com/chromium/src/+/HEAD/docs/flag_guarding_guidelines.md). However, the base feature is autogenerated at runtime using https://source.corp.google.com/h/chromium/chromium/src/+/main:third_party/blink/renderer/platform/runtime_enabled_features.json5;l=448-45.
Having the base feature is [required](https://g3doc.corp.google.com/analysis/uma/g3doc/finch/feature-api.md?cl=head) for using the [killswitch](https://chromium.googlesource.com/chromium/src/+/HEAD/docs/flag_guarding_guidelines.md#prefer-waterfall-rollout-for-platform-changes). Is my understanding correct?
Sorry for the complexity here, my hope is that we're just removing the chrome://flags entry in this CL. We can keep the blink::Feature::kAISummarizationAPI flag autogenerated from the AISummarizationAPI entry in runtime_enabled_features.json5. We should also add the missing `copied_from_base_feature_if: "overridden"` line(s) there to hopefully fix apparent kill switch breakage in this CL (or separately if needed). We can remove the runtime_enabled_features entry sometime in the near future.
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. |
Mike Wasserman (Gerrit)
Mike Wasserman added 4 comments![]( "Open in Gerrit")
// Stable features, if not enabled via command line, are controlled
// by the kill switch.
if (IsAPIStable(name)) {
return !IsAPIKillSwitchTriggered(GetParam());
}
// Non-stable features, not command-line enabled:
// Kill switch is effective.
if (IsAPIKillSwitchTriggered(GetParam())) {
return false;
}I think this would be clearer as:
```suggestion
// Kill switches can disable any runtime enabled features.
if (IsAPIKillSwitchTriggered(GetParam())) {
return false;
}
// Stable features are available by default.
if (IsAPIStable(name)) {
return true;
}
```
if (IsAPIKillSwitchTriggered(GetParam())) {Perhaps this should take precedence over the two checks above
// Default to false other APIs not explicitly enabled for workers
// via command line and not killed by the kill switch.nit:
```suggestion
// APIs are generally not enabled for web workers.
```
base_feature_status: "enabled",Thanks for adding these back to restore kill switch support!
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. |
Mike Wasserman (Gerrit)
Mike Wasserman added 1 comment![]( "Open in Gerrit")
const base::FeatureParam<std::string> kAISummarizationAPILanguagesEnabled{Let's keep this for now; we might continue using this to develop and test further language expansions.
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 6 comments![]( "Open in Gerrit")
// Note: "AISummarizationAPI" is not added to kDisableBlinkFeatures
// because it's marked as "stable" and cannot be disabled this way.Isaac AhoumaMy understanding is that even APIs with runtime enabled feature entries can still be disabled by a finch killswitch config on their autogenerated base::Feature. Can you help resolve a conclusive answer here?
Mike WassermanYes, you are correct that runtime enabled features can be [disabled by a Finch killswitch](https://chromium.googlesource.com/chromium/src/third_party/+/HEAD/blink/renderer/platform/RuntimeEnabledFeatures.md).
However, flags that have graduated to stable can not be disabled using a killswitch, which is what I was trying to highlight here. I have also followed up on this in a separate reply to check my understanding on runtime features and feature flags.
Thanks for continuing to look into this WRT the runtime enabled feature entry missing the base feature override. I'll wait to hear what you find.
Done
// Stable features, if not enabled via command line, are controlled
// by the kill switch.
if (IsAPIStable(name)) {
return !IsAPIKillSwitchTriggered(GetParam());
}
// Non-stable features, not command-line enabled:
// Kill switch is effective.
if (IsAPIKillSwitchTriggered(GetParam())) {
return false;
}I think this would be clearer as:
```suggestion
// Kill switches can disable any runtime enabled features.
if (IsAPIKillSwitchTriggered(GetParam())) {
return false;
}// Stable features are available by default.
if (IsAPIStable(name)) {
return true;
}
```
Discussed offline, and this is actually obsolete now. Thanks
Perhaps this should take precedence over the two checks above
obsolete
// Default to false other APIs not explicitly enabled for workers
// via command line and not killed by the kill switch.nit:
```suggestion
// APIs are generally not enabled for web workers.
```
obsolete
const base::FeatureParam<std::string> kAISummarizationAPILanguagesEnabled{Let's keep this for now; we might continue using this to develop and test further language expansions.
Done
depends_on: ["AISummarizationAPI"],I hope an embedder granting this permissions policy feature to an iframe wouldn't actually enable the API if the blink::Feature::kAISummarizationAPI killswitch were triggered. Maybe we should leave this here? (and restore the one for Language Detector?). We're essentially just trying to remove the chrome://flag entry in these CLs.
Isaac AhoumaI am a little confused by this comment. Since I am removing blink::Feature::kAISummarizationAPI in this CL (in chrome/browser/about_flags.cc0, doesn't that mean that the kill switch will not be able to disable that feature as it will always be ON?
Mike WassermanI think I got it now. The Blink feature is not the same as a Base feature. We can remove the Blink feature (which is the purpose of this CL)(https://chromium.googlesource.com/chromium/src/+/HEAD/docs/flag_guarding_guidelines.md). However, the base feature is autogenerated at runtime using https://source.corp.google.com/h/chromium/chromium/src/+/main:third_party/blink/renderer/platform/runtime_enabled_features.json5;l=448-45.
Having the base feature is [required](https://g3doc.corp.google.com/analysis/uma/g3doc/finch/feature-api.md?cl=head) for using the [killswitch](https://chromium.googlesource.com/chromium/src/+/HEAD/docs/flag_guarding_guidelines.md#prefer-waterfall-rollout-for-platform-changes). Is my understanding correct?
Sorry for the complexity here, my hope is that we're just removing the chrome://flags entry in this CL. We can keep the blink::Feature::kAISummarizationAPI flag autogenerated from the AISummarizationAPI entry in runtime_enabled_features.json5. We should also add the missing `copied_from_base_feature_if: "overridden"` line(s) there to hopefully fix apparent kill switch breakage in this CL (or separately if needed). We can remove the runtime_enabled_features entry sometime in the near future.
- Mike Wasserman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike Wasserman (Gerrit)
Mike Wasserman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mike Wasserman
| Code-Review | +1 |
1 comment
Related details
- Isaac Ahouma
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber added 1 comment![]( "Open in Gerrit")
Adding tkent from third_party/blink/API_OWNERS for runtime_enabled_features.json5.
Related details
- Isaac Ahouma
- Kent Tamura
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kent Tamura (Gerrit)
Kent Tamura added 1 comment![]( "Open in Gerrit")
base_feature_status: "enabled",
copied_from_base_feature_if: "overridden",
},What's the intention of this change?
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")
base_feature_status: "enabled",
copied_from_base_feature_if: "overridden",
},What's the intention of this change?
Chrome has some API feature flags that are expiring in M145 as the corresponding APIs have graduated to stable. We are removing references to these flags in a series of CL. The intent of this particular CL is to remove references to the feature flag guarding the Summarization API
Related details
- Kent Tamura
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_status: "enabled",
copied_from_base_feature_if: "overridden",
},Isaac AhoumaWhat's the intention of this change?
Chrome has some API feature flags that are expiring in M145 as the corresponding APIs have graduated to stable. We are removing references to these flags in a series of CL. The intent of this particular CL is to remove references to the feature flag guarding the Summarization API
Regarding the change to third_party/blink/renderer/platform/runtime_enabled_features.json5, we are adding the options to the API to enable Finch override for the runtime enabled feature. This is to allow for allowing the feature to have a killswitch. We think this aligns with the overall CL, but let us know if you prefer if we do this in a separate CL.
Kent Tamura (Gerrit)
Kent Tamura added 1 comment![]( "Open in Gerrit")
base_feature_status: "enabled",
copied_from_base_feature_if: "overridden",
},Isaac AhoumaWhat's the intention of this change?
Isaac AhoumaChrome has some API feature flags that are expiring in M145 as the corresponding APIs have graduated to stable. We are removing references to these flags in a series of CL. The intent of this particular CL is to remove references to the feature flag guarding the Summarization API
Regarding the change to third_party/blink/renderer/platform/runtime_enabled_features.json5, we are adding the options to the API to enable Finch override for the runtime enabled feature. This is to allow for allowing the feature to have a killswitch. We think this aligns with the overall CL, but let us know if you prefer if we do this in a separate CL.
The killswitch for this flag should work by default. `base_feature_status` and `copied_from_base_feature_if` look unnecessary.
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")
base_feature_status: "enabled",
copied_from_base_feature_if: "overridden",
},Isaac AhoumaWhat's the intention of this change?
Isaac AhoumaChrome has some API feature flags that are expiring in M145 as the corresponding APIs have graduated to stable. We are removing references to these flags in a series of CL. The intent of this particular CL is to remove references to the feature flag guarding the Summarization API
Kent TamuraRegarding the change to third_party/blink/renderer/platform/runtime_enabled_features.json5, we are adding the options to the API to enable Finch override for the runtime enabled feature. This is to allow for allowing the feature to have a killswitch. We think this aligns with the overall CL, but let us know if you prefer if we do this in a separate CL.
The killswitch for this flag should work by default. `base_feature_status` and `copied_from_base_feature_if` look unnecessary.
You are right. Looks to be working without it.
Is this the default for every runtime enabled feature? It seemed like some can not be kill switched unless you explicitly added these options.
Thanks!
Related details
- Kent Tamura
- Mike Wasserman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike Wasserman (Gerrit)
Mike Wasserman voted Code-Review+1![]( "Open in Gerrit")
- Isaac Ahouma
- Kent Tamura
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |