Move WebStorePrivate API to //extensions [chromium/src : main]
Miyoung Shin (Gerrit)
Miyoung Shin added 2 comments![]( "Open in Gerrit")
Hi, Devline, I have a question that's somewhat related to the broader migration effort, although not directly related to this patch.
Recently, I noticed that all extension API tests, including browser tests that were previously under `//extensions`, have been moved into the `//chrome` layer. I also noticed that the `app_shell` target was removed.
From the perspective of this migration tasks, these changes seem somewhat disconnected from the goal of making extension support available outside of the Chrome layer.
My concern is that embedders who want to support extensions without depending on `//chrome` may no longer have a lightweight shell environment to reference, nor an easy way to validate extension functionality independently of Chrome.
Could you share some background on the reasoning behind these changes and the intended direction going forward?
Also, do you think it would be possible to restore some form of extension-focused shell or testing environment, either by bringing back the shell itself or by providing an alternative mechanism for validation?
ExtensionAllowlist* GetExtensionAllowlist(
content::BrowserContext* context) override;
signin::IdentityManager* GetIdentityManager(
content::BrowserContext* context) override;
void ShowExtensionInstallBlockedDialog(
content::WebContents* web_contents,
const Extension* extension,
const std::u16string& custom_error_message,
const gfx::ImageSkia& icon,
base::OnceClosure done_callback) override;
void ShowExtensionInstallFrictionDialog(
content::WebContents* web_contents,
base::OnceCallback<void(bool)> callback) override;
#if BUILDFLAG(IS_ANDROID)
void ShowExtensionInstallAskParentDialog(
content::WebContents* web_contents,
base::OnceClosure cancel_callback,
base::OnceClosure approve_callback) override;
#endif // BUILDFLAG(IS_ANDROID)
void ReportFrictionAcceptedEvent(content::BrowserContext* context) override;
#if BUILDFLAG(SAFE_BROWSING_AVAILABLE)
bool IsSafeBrowsingEnabledAndReady(content::BrowserContext* context) override;
safe_browsing::SafeBrowsingNavigationObserverManager*
GetSafeBrowsingNavigationObserverManager(
content::BrowserContext* context) override;
#endif
std::unique_ptr<enterprise_promotion::PromotionEligibilityChecker>
CreatePromotionEligibilityChecker(content::BrowserContext* context,
bool dismissed_banner_pref,
bool feature_enabled) override;Miyoung Shinthis is a lot of new methods that are very likely only ever going to be used by the WebstorePrivateAPI.
Can we introduce a WebstorePrivateAPIDelegate and expose a getter for that class on ExtensionAPIClient? That might be more targeted than dropping these here, since they won't have external callers.
Done. Thanks for suggestion!
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 2 comments![]( "Open in Gerrit")
Hi, Devline, I have a question that's somewhat related to the broader migration effort, although not directly related to this patch.
Recently, I noticed that all extension API tests, including browser tests that were previously under `//extensions`, have been moved into the `//chrome` layer. I also noticed that the `app_shell` target was removed.
From the perspective of this migration tasks, these changes seem somewhat disconnected from the goal of making extension support available outside of the Chrome layer.
My concern is that embedders who want to support extensions without depending on `//chrome` may no longer have a lightweight shell environment to reference, nor an easy way to validate extension functionality independently of Chrome.
Could you share some background on the reasoning behind these changes and the intended direction going forward?
Also, do you think it would be possible to restore some form of extension-focused shell or testing environment, either by bringing back the shell itself or by providing an alternative mechanism for validation?
Great question!
TL;DR: The extension browser tests running in //extensions didn't test real logic and could hide bugs.
App shell was written ~10 years ago. Back in the day, the goal was to have it as an independent environment from Chrome so that ChromeOS could run platform apps without needing to spin up the whole Chrome browser. It was spun down (and platform apps are deprecated), but it was then used as, like you said, a lightweight way to test extension APIs that don't depend on Chrome.
Unfortunately, for *browser* tests, this really wasn't sufficient. Browser tests (and extension API tests) are meant to be more "end-to-end", "integration", "real-world" tests -- that is, the goal of them is to exercise the API in the Chrome browser in a fairly robust and realistic manner. We should have confidence that if an API succeeds in an API test, it will succeed in a real instance of the Chrome browser (obviously, with some caveats around e.g. channels and feature restrictions).
This wasn't true for the shell tests, though, for two main reasons:
1) Many APIs delegate out calls to the embedding layer (e.g., ExtensionsApiClient, etc). If we then only test the API at the //extensions layer, the embedding logic isn't tested. We've had cases where the bug was in delegated logic and would have been caught if it were exercised, but instead, only the "stub" implementation in the shell client was used.
2) Extensions fundamentally depend on a *lot* of other functionality, from the UI to the networking stack to the rendering and process management. These, in turn, are largely impacted by the Chrome layer (as a concrete example, the ChromeContentBrowserClient has a *lot* of implications for how processes are managed, which then impacts when / where extensions code is running and how other APIs process changes). By running these browser tests without this setup, the environment in which the APIs were exercised becomes fundamentally different from the environment in which they normally run.
Point 1) is, at it's core, an architectural / coding oversight: it basically just meant that the delegated implementations weren't tested, and should have been. That's fixable. Point 2) is much more fundamental.
To this end, we want these API tests, which are meant to serve as "real-world" tests, to use the "real" chrome browser setup, so we can have the highest level of confidence in an API's behavior. Hence, they are moved to the //chrome layer.
I don't think we want to bring back any kind of "shell" environment for these -- we worked hard to get rid of it, since it was both technical debt and was the source of real bugs (insofar as we had inaccurate testing). However, there are a few points worth mentioning here:
- Unit tests can (and usually should) continue living next to the class being tested. Unit tests already exercise logic in more of a vacuum and aren't meant to be end-to-end / integration tests, so having them run without the whole chrome layer makes sense (and is desirable).
- We're working on adding support for extension-based Web Platform Tests. These would run with the real chrome browser, but would be much easier for other embedders to leverage, because they won't have direct knowledge of the //chrome layer (they're designed to be used in any browser, including ones that aren't chromium-based). So, if an embedder can support that, that seems like the cleanest path towards something like this.
Does that make sense and sound reasonable?
Thanks, Miyoung! Just replying to your open question; I'll review this one once the upstream is updated and mostly LG
Related details
- Miyoung Shin
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. |
Miyoung Shin (Gerrit)
Miyoung Shin added 1 comment![]( "Open in Gerrit")
Devlin CroninHi, Devline, I have a question that's somewhat related to the broader migration effort, although not directly related to this patch.
Recently, I noticed that all extension API tests, including browser tests that were previously under `//extensions`, have been moved into the `//chrome` layer. I also noticed that the `app_shell` target was removed.
From the perspective of this migration tasks, these changes seem somewhat disconnected from the goal of making extension support available outside of the Chrome layer.
My concern is that embedders who want to support extensions without depending on `//chrome` may no longer have a lightweight shell environment to reference, nor an easy way to validate extension functionality independently of Chrome.
Could you share some background on the reasoning behind these changes and the intended direction going forward?
Also, do you think it would be possible to restore some form of extension-focused shell or testing environment, either by bringing back the shell itself or by providing an alternative mechanism for validation?
Great question!
TL;DR: The extension browser tests running in //extensions didn't test real logic and could hide bugs.
App shell was written ~10 years ago. Back in the day, the goal was to have it as an independent environment from Chrome so that ChromeOS could run platform apps without needing to spin up the whole Chrome browser. It was spun down (and platform apps are deprecated), but it was then used as, like you said, a lightweight way to test extension APIs that don't depend on Chrome.
Unfortunately, for *browser* tests, this really wasn't sufficient. Browser tests (and extension API tests) are meant to be more "end-to-end", "integration", "real-world" tests -- that is, the goal of them is to exercise the API in the Chrome browser in a fairly robust and realistic manner. We should have confidence that if an API succeeds in an API test, it will succeed in a real instance of the Chrome browser (obviously, with some caveats around e.g. channels and feature restrictions).
This wasn't true for the shell tests, though, for two main reasons:
1) Many APIs delegate out calls to the embedding layer (e.g., ExtensionsApiClient, etc). If we then only test the API at the //extensions layer, the embedding logic isn't tested. We've had cases where the bug was in delegated logic and would have been caught if it were exercised, but instead, only the "stub" implementation in the shell client was used.
2) Extensions fundamentally depend on a *lot* of other functionality, from the UI to the networking stack to the rendering and process management. These, in turn, are largely impacted by the Chrome layer (as a concrete example, the ChromeContentBrowserClient has a *lot* of implications for how processes are managed, which then impacts when / where extensions code is running and how other APIs process changes). By running these browser tests without this setup, the environment in which the APIs were exercised becomes fundamentally different from the environment in which they normally run.Point 1) is, at it's core, an architectural / coding oversight: it basically just meant that the delegated implementations weren't tested, and should have been. That's fixable. Point 2) is much more fundamental.
To this end, we want these API tests, which are meant to serve as "real-world" tests, to use the "real" chrome browser setup, so we can have the highest level of confidence in an API's behavior. Hence, they are moved to the //chrome layer.
I don't think we want to bring back any kind of "shell" environment for these -- we worked hard to get rid of it, since it was both technical debt and was the source of real bugs (insofar as we had inaccurate testing). However, there are a few points worth mentioning here:
- Unit tests can (and usually should) continue living next to the class being tested. Unit tests already exercise logic in more of a vacuum and aren't meant to be end-to-end / integration tests, so having them run without the whole chrome layer makes sense (and is desirable).
- We're working on adding support for extension-based Web Platform Tests. These would run with the real chrome browser, but would be much easier for other embedders to leverage, because they won't have direct knowledge of the //chrome layer (they're designed to be used in any browser, including ones that aren't chromium-based). So, if an embedder can support that, that seems like the cleanest path towards something like this.
Does that make sense and sound reasonable?
Thanks for the detailed explanation!! That makes a lot of sense, and extension-based Web Platform Tests sound like a really promising direction.
I have one follow-up question, though. Setting testing aside, what do you think about having a lightweight shell purely as a reference implementation for embedders?
My thinking is that embedders who want to support extensions could benefit from having a minimal shell that implements the required delegates and demonstrates how to wire everything together, rather than serving as a testing environment.
Do you think something like that would be valuable, or do you think it's better for embedders to build their own integration from scratch?
Related details
- Devlin Cronin
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. |
Devlin Cronin (Gerrit)
Devlin Cronin added 1 comment![]( "Open in Gerrit")
My suspicion is that we probably wouldn't support an embedder like that in Chromium, at least not at the moment. It's not something that's immediately aligned with any of our projects and I think it would be difficult to have an embedder that demonstrated "proper" or "realistic" embedding (for all the same reasons as the tests were problematic). I wouldn't necessarily want the shell implementation to serve as an example for other embedders / implementers, since it wouldn't be the source of truth: if there were a divergence between it and Chromium's integration, Chromium, and not the shell, should be the trusted one. I think that reduces the utility and complicates the story around what the expected behavior is.
I'm happy to chat about it more, if you think it's something that would fit well directionally for Chromium, but at the moment I'd lean towards having WPT provide the consistency guarantees, and otherwise aassuming that the browsers themselves are "example" implementations.
Related details
- Miyoung Shin
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. |
Miyoung Shin (Gerrit)
Miyoung Shin added 1 comment![]( "Open in Gerrit")
Thanks for the detailed explanation. I agree with your point. From Chromium's perspective, I don't think there's a strong enough case to officially support such an embedder at the moment.
It feels a bit like a chicken-and-egg problem. Embedders may want to know whether they can run the extensions they care about using only the `//extensions` layer, but if the barrier to validating that is too high, it becomes difficult for them to even evaluate the feasibility.
So, I'm thinking about providing a lightweight reference shell in a separate public repository rather than upstreaming it into Chromium. The goal wouldn't be to define the "correct" implementation, but simply to lower the barrier for people who want to experiment with extension support and understand the minimum embedder integration required.
And I completely agree that supporting extension APIs through WPT is a great direction. I think that will provide a much stronger and more maintainable foundation for interoperability and validation in the long run.
BTW, could you take a look at this CL?
Related details
- Devlin Cronin
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. |
Devlin Cronin (Gerrit)
Devlin Cronin added 8 comments![]( "Open in Gerrit")
BTW, could you take a look at this CL?
Sorry, didn't realize this was ready for a pass. Done
Thanks, Miyoung!
dependencies.push_back(ManagementAPI::GetFactoryInstance());nit: since this one is already in the //extensions layer, can the webstore_private_api.cc file handle it? (And maybe phrase this method as "Get*Additional*WebstorePrivateAPIFactoryDependencies")
std::vector<KeyedServiceBaseFactory*>
ChromeExtensionsAPIClient::GetWebStoreAPIFactoryDependencies() {
std::vector<KeyedServiceBaseFactory*> dependencies;
dependencies.push_back(ExtensionAllowlistFactory::GetInstance());
dependencies.push_back(IdentityManagerFactory::GetInstance());
dependencies.push_back(InstallTrackerFactory::GetInstance());
dependencies.push_back(ManagementAPI::GetFactoryInstance());
dependencies.push_back(
safe_browsing::SafeBrowsingMetricsCollectorFactory::GetInstance());
dependencies.push_back(
safe_browsing::SafeBrowsingNavigationObserverManagerFactory::
GetInstance());
return dependencies;
}nit: could we move the body of this into a static method on WebstorePrivateAPIDelegate?
#include "chrome/browser/extensions/extension_allowlist_factory.h"nit: these includes are no longer necessary, right?
#include "chrome/browser/ui/extensions/extensions_dialogs.h"ditto
#include "extensions/browser/api/webstore_private/webstore_private_api.h"Can this now go in extensions/browser/api/api_browser_context_keyed_service_factories.cc?
#include "components/safe_browsing/buildflags.h"needed?
Related details
- Miyoung Shin
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. |
Miyoung Shin (Gerrit)
Miyoung Shin added 8 comments![]( "Open in Gerrit")
Done
dependencies.push_back(ManagementAPI::GetFactoryInstance());nit: since this one is already in the //extensions layer, can the webstore_private_api.cc file handle it? (And maybe phrase this method as "Get*Additional*WebstorePrivateAPIFactoryDependencies")
Done
std::vector<KeyedServiceBaseFactory*>
ChromeExtensionsAPIClient::GetWebStoreAPIFactoryDependencies() {
std::vector<KeyedServiceBaseFactory*> dependencies;
dependencies.push_back(ExtensionAllowlistFactory::GetInstance());
dependencies.push_back(IdentityManagerFactory::GetInstance());
dependencies.push_back(InstallTrackerFactory::GetInstance());
dependencies.push_back(ManagementAPI::GetFactoryInstance());
dependencies.push_back(
safe_browsing::SafeBrowsingMetricsCollectorFactory::GetInstance());
dependencies.push_back(
safe_browsing::SafeBrowsingNavigationObserverManagerFactory::
GetInstance());
return dependencies;
}nit: could we move the body of this into a static method on WebstorePrivateAPIDelegate?
Done
#include "chrome/browser/extensions/extension_allowlist_factory.h"nit: these includes are no longer necessary, right?
Done
#include "chrome/browser/ui/extensions/extensions_dialogs.h"Miyoung Shinditto
Done
#include "extensions/browser/api/webstore_private/webstore_private_api.h"Can this now go in extensions/browser/api/api_browser_context_keyed_service_factories.cc?
Done
#include "components/safe_browsing/buildflags.h"Miyoung Shinneeded?
Done
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 2 comments![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
2 comments
#include "extensions/browser/api/management/management_api.h"nit: no longer necessary
Related details
- Miyoung Shin
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. |
Miyoung Shin (Gerrit)
Miyoung Shin added 2 comments![]( "Open in Gerrit")
thakis@ for //chrome/browser/supervised_user/
#include "extensions/browser/api/management/management_api.h"Miyoung Shinnit: no longer necessary
Done
Related details
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Miyoung Shin (Gerrit)
Miyoung Shin added 1 comment![]( "Open in Gerrit")
thakis@, friendly ping