Magdalena SkarbińskaI made some comments about splitting things to separate CLs. in general let's avoid making refactors while moving/renaming files as it's harder to review.
Done
Move IwaKeyDistributionInfoProvider and installer policy to //componentsMagdalena Skarbińskaneeds update, the installer policy is still in //chrome
also below
Done
BASE_DECLARE_FEATURE(kIwaKeyDistributionComponent);Magdalena SkarbińskaThis flag logically belongs to the installer and to the `component_updater` namespace, so it has to stay with the installer.
Done
void IwaKeyDistributionComponentInstallerPolicy::SetUp() {Magdalena Skarbińskait seems you create the class and immediately call `SetUp`.
can `SetUp` be implemented in the ctor instead of a separate function?
Done
return web_app::IwaKeyDistributionInfoProvider::GetInstance();Magdalena Skarbińskanit: it's much easier to just inline these calls, no need for a separate func
Done
// No custom install.Magdalena Skarbińskarevert
Done
class ChromeIwaRuntimeDataProvider;Magdalena Skarbińskarevert?
Done
Magdalena Skarbińskashould this be moved too?
I left it here bc I use component installer and test utils from //chrome in these tests. Is that ok then to leave it as is?
class IwaKeyDistributionInfoProviderReadinessTestMagdalena Skarbińskacan you split this rename to a separate CL?
Done
class ChromeIwaRuntimeDataProvider : public IwaRuntimeDataProvider {Magdalena Skarbińskacan this be split to a separate CL?
Done
// A builder-style class to help create and update the key distributionMagdalena Skarbińskalet's keep those comments
Done
struct IwaComponentMetadata {Edman AnjosI pointed this out in a separate CL -- this testing infra is intended to be used in very specific user journeys that require actually loading the component data. Do you really need it in `//components`?
Magdalena SkarbińskaNo strong opinion on my side but this sounds fair to me. The helpers are related to key distribution so it makes sense to colocate them, which seems possible as they don't have `//chrome` dependencies.
Acknowledged
BASE_FEATURE(kIwaKeyDistributionDevMode, base::FEATURE_DISABLED_BY_DEFAULT);Magdalena SkarbińskaThis is only used in `//chrome` and doesn't have to be moved. Feel free to inline it in an anon namespace in `chrome/browser/ui/webui/web_app_internals/web_app_internals_ui.cc`
Done
static IwaKeyDistributionInfoProvider& GetInstance();Magdalena Skarbińskacan you split this to a separate CL?
Done
if (!is_setup_) {Magdalena Skarbińskathe previous code seems good enough. you can rely on the `queue_on_demand_update_` callback being null before `SetUp` is called, so we don't need the new `is_setup_` flag
you could call `.is_null()` if you want to be explicit
Done
if (queue_on_demand_update_) {Magdalena Skarbińskashould the code below this `if` block still run when `queue_on_demand_update_` is null?
I'm wondering if this check should be moved to L528 as an early exit like `if (!queue_on_demand_update_) return;`
i reverted the check to be present again
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
thanks Magda, LGTM % few comments and questions
TAG=agy CONV=8a5fe3f3-89f8-427b-8e45-62c0d5e3d48dI think you can remove this line, AFAIK this is meant for google3
base::PassKey<web_app::IwaKeyDistributionInfoProvider>);is it intentional that the passkey was removed in this CL?
Magdalena Skarbińskashould this be moved too?
I left it here bc I use component installer and test utils from //chrome in these tests. Is that ok then to leave it as is?
Acknowledged, thanks for explaining
if (queue_on_demand_update_) {Magdalena Skarbińskashould the code below this `if` block still run when `queue_on_demand_update_` is null?
I'm wondering if this check should be moved to L528 as an early exit like `if (!queue_on_demand_update_) return;`
i reverted the check to be present again
if you have `CHECK` above you can remove the `if (queue_on_demand_update_)` here.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
#include "base/memory/weak_ptr.h"Is this needed?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Commit-Queue | +1 |
I think you can remove this line, AFAIK this is meant for google3
Done
add a `Bug: number` please
Done
#include "base/memory/weak_ptr.h"Magdalena SkarbińskaIs this needed?
Done
base::PassKey<web_app::IwaKeyDistributionInfoProvider>);is it intentional that the passkey was removed in this CL?
it came back right now :)
if (queue_on_demand_update_) {Magdalena Skarbińskashould the code below this `if` block still run when `queue_on_demand_update_` is null?
I'm wondering if this check should be moved to L528 as an early exit like `if (!queue_on_demand_update_) return;`
Edman Anjosi reverted the check to be present again
if you have `CHECK` above you can remove the `if (queue_on_demand_update_)` here.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shadowed: ks...@chromium.org
Reviewer source(s):
ks...@chromium.org is from context(googleclient/chrome/chromium_gwsq/chrome/browser/ui/web_applications/config.gwsq,googleclient/chrome/chromium_gwsq/chrome/browser/web_applications/config.gwsq)
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
#include "components/webapps/isolated_web_apps/key_distribution/iwa_key_distribution_info_provider.h"I believe this is not needed here anymore?
| Code-Review | +1 |
LGTM! Assigning to an owner so that they can allow the code to land.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
#include "components/webapps/isolated_web_apps/key_distribution/iwa_key_distribution_info_provider.h"I believe this is not needed here anymore?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |