Add support for installing / updating extensions #59

bbondy · 2016-09-20T06:32:05Z

bridiver · 2016-09-20T16:42:29Z

atom/browser/api/atom_api_update_client.cc

@@ -0,0 +1,172 @@
+// Copyright 2016 The Brave Authors. All rights reserved.


can we move this to brave/browser in a follow-up? Trying to keep non-upstreamable stuff separate so we can split out the repos

bridiver · 2016-09-20T16:46:18Z

atom/browser/api/atom_api_update_client.cc

+     const content::NotificationDetails& details) {
+  switch (type) {
+    case extensions::NOTIFICATION_EXTENSION_ENABLED: {
+      Emit("extension-enabled");


should probably have one for extension-disabled as well, but maybe both should go in atom_api_extensions? Can also be follow-up

Actually I don't even use this so I can remove it.

bridiver · 2016-09-20T17:15:55Z

atom/browser/api/atom_api_extension.cc

+void Extension::Remove(const std::string& extension_id) {
+  const extensions::Extension* extension =
+    GetInstance()->extensions_.GetByID(extension_id);
+  if (extension) {


shouldn't this just call Extension::Disable?

actually I would disable in AtomExtensionSystem because there is a check there to prevent uninstalling component extensions (like brave)

Isn't NOTIFICATION_DISABLE_USER_EXTENSION_REQUEST handled in AtomExtensionSystem to disable?

yes, but there are other checks that should happen and I think it's better to keep all of that within AtomExtensionSystem. See ExtensionService::UninstallExtension and ExtensionService::UnloadExtension. We don't need everything in there, but I think we should keep the basic structure the same because it's much easier to follow changes in libchromiumcontent that way

bridiver · 2016-09-20T17:21:07Z

atom/browser/api/atom_api_extension.cc

+
+  base::FilePath install_directory;
+  PathService::Get(component_updater::DIR_COMPONENT_USER, &install_directory);
+  extensions::file_util::UninstallExtension(install_directory, extension_id);


this should run on the file thread and should probably also happen in AtomExtensionSystem to prevent uninstall of component extensions. See ExtensionService::OnExtensionInstalled

k will move it.

bridiver · 2016-09-20T17:23:58Z

atom/browser/extensions/atom_extension_system.cc

+          &install_directory);
+      std::string extension_id = extension->id();
+      auto registry = ExtensionRegistry::Get(browser_context_);
+      registry->RemoveEnabled(extension_id);


not sure if these methods should be called or not, but ExtensionRegistry::TriggerOnUninstalled does. See ExtensionService::UninstallExtension

bridiver · 2016-09-20T17:33:46Z

atom/browser/api/atom_api_update_client.cc

+mate::Handle<UpdateClient> UpdateClient::Create(
+    v8::Isolate* isolate,
+    content::BrowserContext* browser_context) {
+  return mate::CreateHandle(isolate,


the update client should use chrome::GetBrowserContextRedirectedInIncognito to make sure that there is only one per user since the extensions are shared across private and session contexts

actually see above because I think using AtomBrowserCreateUpdateClient::CreateUpdateClient is the right way to handle this

bridiver · 2016-09-20T17:36:17Z

atom/browser/api/atom_api_update_client.cc

+UpdateClient::UpdateClient(v8::Isolate* isolate,
+                 content::BrowserContext* browser_context)
+      : browser_context_(browser_context) {
+  cus_ = component_updater::ComponentUpdateServiceFactory(


I think there is only supposed to be one component updater globally, but maybe this is ok for now with the change below that creates a shared update client for all contexts

it's only used for the default session, even know it can be used for others as well (there would be no point). Is there a way to get the default session browser context as a singleton or something?

well, there are two things going on here. The update client really should be retrieved from AtomBrowserCreateUpdateClient::CreateUpdateClient with something like

return update_client::UpdateClientFactory( make_scoped_refptr(new AtomUpdateClientConfig(GetOriginalContext(context))));

If you do that the UpdateServiceFactory::GetForBrowserContext(browser_context) will work correctly

the component updater service itself can be initialized with the default browser context and in chrome it's attached to the BrowserProcess

bbondy · 2016-09-24T15:23:33Z

@darkdh @bridiver this is ready for another review. You'll notice I didn't end up needing to uninstall to upgrade, so this adds some code for uninstalling that is not currently used, but I think it's useful to keep anyway.

bbondy · 2016-09-26T21:41:46Z

brave/browser/component_updater/brave_component_updater_configurator.cc

@@ -148,7 +154,8 @@ bool BraveConfigurator::UseBackgroundDownloader() const {
 }

 bool BraveConfigurator::UseCupSigning() const {
-  return configurator_impl_.UseCupSigning();
+  // return configurator_impl_.UseCupSigning();
+  return false;


Checked with Yan and she said this is ok.

bridiver

just a few small changes, otherwise ++

bridiver · 2016-09-27T01:20:37Z

atom/browser/api/atom_api_extension.cc

@@ -220,6 +220,26 @@ void Extension::Enable(const std::string& extension_id) {
 }

 // static
+void Extension::Remove(const std::string& extension_id) {


should we make this remove to be consistent?

bridiver · 2016-09-27T01:36:48Z

atom/browser/extensions/atom_extension_system.h

+    //
+    // If the return value is true, |deletion_done_callback| is invoked when
+    // data deletion is done or at least is scheduled.
+    bool UninstallExtension(const std::string& extension_id,


this method should also be a virtual method on the extension service so it can be called with

return extensions::ExtensionSystem::Get(context) ->extension_service() ->UninstallExtension(transient_extension_id, reason, deletion_done_callback, error);

from atom_extensions_api_client

I'll just remove uninstall and remove since they aren't used for now.

bridiver · 2016-09-27T02:52:38Z

atom/browser/extensions/atom_extension_system.cc

+      &install_directory);
+
+  // Tell the backend to start deleting installed extensions on the file thread.
+  BrowserThread::PostTask(


I think that it could be a problem here if we don't use the blocking thread pool because the remove could still be in progress when the new install starts on an update

bridiver · 2016-09-27T02:56:52Z

atom/browser/extensions/atom_extension_system.cc

+  // Notify interested parties that we've uninstalled this extension.
+  ExtensionRegistry::Get(browser_context_)
+      ->TriggerOnUninstalled(extension.get(), reason);
+


extension_prefs_->OnExtensionUninstalled( extension->id(), extension->location(), external_uninstall);

bridiver · 2016-09-27T03:07:55Z

atom/browser/extensions/atom_extension_system.cc

+        content::Source<content::BrowserContext>(browser_context_),
+        content::Details<const Extension>(extension));
+  } else {
+    // All apps that are displayed in the launcher are ordered by their ordinals


this section should be left out because it deals with chrome apps, not extensions and app_sorting() returns nullptr

bridiver · 2016-09-27T03:24:17Z

atom/browser/browser_context_keyed_service_factories.cc

@@ -59,6 +60,7 @@ void EnsureBrowserContextKeyedServiceFactoriesBuilt() {
  extensions::StorageFrontend::GetFactoryInstance();
  extensions::WebRequestAPI::GetFactoryInstance();
  extensions::AtomExtensionSystemFactory::GetInstance();
+  extensions::UpdateServiceFactory::GetInstance();


UpdateServiceFactory won't work correctly unless AtomExtensionBrowserClient implements ExtensionsBrowserClient::CreateUpdateClient

return update_client::UpdateClientFactory( make_scoped_refptr(new BraveUpdateClientConfig(context)));

actually this doesn't appear to be implemented in chrome so we should probably remove it because it will pass nullptr to the `UpdateService if it is called anywhere

Ya this was left over from when I did have CreateUpdateClient which I didn't end up needing, removing.

bridiver · 2016-09-27T04:32:19Z

++

bbondy mentioned this pull request Sep 20, 2016

Install / update extensions w/ component updater brave/browser-laptop#4105

Merged

bridiver reviewed Sep 20, 2016

View reviewed changes

bbondy force-pushed the component_updater53 branch 10 times, most recently from e547c0c to 67f2294 Compare September 24, 2016 15:20

bbondy force-pushed the component_updater53 branch 5 times, most recently from 563ed2e to b478e10 Compare September 26, 2016 20:26

bbondy commented Sep 26, 2016

View reviewed changes

bbondy force-pushed the component_updater53 branch from b478e10 to 65f790d Compare September 27, 2016 01:17

Add support for installing / updating extensions

e7a4e5c

bridiver reviewed Sep 27, 2016

View reviewed changes

bbondy force-pushed the component_updater53 branch 2 times, most recently from 1ea9e4b to 76fc5ae Compare September 27, 2016 04:29

Use Brave update server

d9e54c9

bbondy force-pushed the component_updater53 branch from 76fc5ae to d9e54c9 Compare September 27, 2016 04:30

bridiver merged commit 60da0fd into master Sep 27, 2016

bbondy deleted the component_updater53 branch September 27, 2016 12:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for installing / updating extensions #59

Add support for installing / updating extensions #59

bbondy commented Sep 20, 2016

bridiver Sep 20, 2016

bbondy Sep 20, 2016

bridiver Sep 20, 2016

bbondy Sep 20, 2016

bridiver Sep 20, 2016

bridiver Sep 20, 2016

bbondy Sep 20, 2016

bridiver Sep 20, 2016

bridiver Sep 20, 2016

bbondy Sep 20, 2016

bridiver Sep 20, 2016

bridiver Sep 20, 2016

bbondy Sep 20, 2016

bridiver Sep 20, 2016

bridiver Sep 20, 2016

bbondy Sep 20, 2016

bridiver Sep 20, 2016

bridiver Sep 20, 2016

bbondy commented Sep 24, 2016

bbondy Sep 26, 2016

bridiver left a comment

bridiver Sep 27, 2016

bridiver Sep 27, 2016

bbondy Sep 27, 2016

bridiver Sep 27, 2016

bridiver Sep 27, 2016 •

edited

Loading

bridiver Sep 27, 2016

bridiver Sep 27, 2016

bridiver Sep 27, 2016

bbondy Sep 27, 2016

bridiver commented Sep 27, 2016

		@@ -0,0 +1,172 @@
		// Copyright 2016 The Brave Authors. All rights reserved.

Add support for installing / updating extensions #59

Add support for installing / updating extensions #59

Conversation

bbondy commented Sep 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbondy commented Sep 24, 2016

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver commented Sep 27, 2016

bridiver Sep 27, 2016 •

edited

Loading