New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 787563 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Remove the callback from ExtensionService::UninstallExtension

Project Member Reported by rdevlin....@chromium.org, Nov 21 2017

Issue description

ExtensionService::UninstallExtension() takes a callback for when deletion completes.  There are a few problems:
1. Only one caller uses it [1]; everything else passes in base::Bind(&base::DoNothing) or base::Closure().
2. It's making the migration of the cookie store to network service ( issue 721395 ) difficult.
3. It's not really accurate anyway - it doesn't wait for file deletion, which is probably more important.

We should remove it.

[1] https://chromium.googlesource.com/chromium/src/+/ad327bf8d4a353f9fb2d014d67c1bc3a0a9e9419/chrome/browser/extensions/webstore_reinstaller.cc#102
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9

commit 218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Tue Nov 21 21:41:31 2017

[Extensions] Remove callback from ExtensionService::UninstallExtension

ExtensionService::UninstallExtension takes a callback to call when the
data is removed for the extension's origin. Of the 60+ callers to this
method, the vast majority passed in base::Bind(&base::DoNothing), with
a handful more passing in an empty base::Closure. Only a single caller
(WebstoreReinstaller) passed in a "real" callback, and it appears to
be unnecessary. Additionally, the callback is already misleading - it
corresponds to removing storage partition data, but not chrome.storage
data.

This callback is also making a refactor on the cookies interface more
complicated, and may not be possible to maintain.

Remove the callback from the method interface.

Bug:  721395 ,  787563 
TBR=benwells@chromium.org (c/b/apps)
TBR=atwilson@chromium.org (c/b/background, c/b/policy)
TBR=derat@chromium.org (c/b/chromeos)
TBR=treib@chromium.org (c/b/search)
TBR=estade@chromium.org (c/b/themes)
TBR=tapted@chromium.org (c/b/ui)

Change-Id: I1cbffd781fb90a40ff597e20ae80812b8646c5ef
Reviewed-on: https://chromium-review.googlesource.com/778040
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Randy Smith <rdsmith@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518402}
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/apps/drive/drive_app_provider.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/background/background_application_list_model_unittest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/chromeos/extensions/gfx_utils_unittest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/chromeos/note_taking_helper_unittest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/activity_log/activity_log_enabled_unittest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/api/management/chrome_management_api_delegate.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/api/management/chrome_management_api_delegate.h
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/extension_browsertest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/extension_gcm_app_handler_unittest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/extension_message_bubble_controller_unittest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/extension_service.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/extension_service.h
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/extension_service_test_with_install.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/extension_service_unittest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/extension_uninstall_dialog.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/external_install_error.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/service_worker_apitest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/shared_module_service.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/shared_module_service_unittest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/webstore_inline_installer_browsertest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/webstore_reinstaller.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/extensions/webstore_reinstaller.h
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/policy/policy_browsertest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/search/hotword_service.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/themes/theme_service.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/themes/theme_service_unittest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/ui/app_list/extension_app_model_builder_unittest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/ui/app_list/search/app_search_provider_unittest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/ui/browser_browsertest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/ui/extensions/extension_message_bubble_browsertest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/chrome/browser/ui/webui/ntp/app_launcher_handler.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/extensions/browser/api/management/management_api.cc
[modify] https://crrev.com/218df7fcaf5c9ebe6bf15dfd4110ded91792f8e9/extensions/browser/api/management/management_api_delegate.h

Project Member

Comment 2 by bugdroid1@chromium.org, Nov 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a68c2a85cd2e3a033eaf35918db17a04379ea3f9

commit a68c2a85cd2e3a033eaf35918db17a04379ea3f9
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Wed Nov 29 19:11:26 2017

[Extensions + Storage] Remove callback from ClearDataForOrigin

DataDeleter::StartDeleting takes a callback to be fired when the
deletion is done or scheduled, but no callers use this. Additionally,
the callback is making life difficult for a cookies interface refactor.

Remove the callback from DataDeleter::StartDeleting(). Since this was
the only use of the callback in StoragePartition::ClearDataForOrigin(),
remove the callback from that interface as well.

Bug:  721395 ,  787563 
TBR=michaelbai@chromium.org (android_webview)

Change-Id: I6e90a4275703d4e356beee11f445ee4c9deaa720
Reviewed-on: https://chromium-review.googlesource.com/790754
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Randy Smith <rdsmith@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520185}
[modify] https://crrev.com/a68c2a85cd2e3a033eaf35918db17a04379ea3f9/android_webview/browser/aw_quota_manager_bridge.cc
[modify] https://crrev.com/a68c2a85cd2e3a033eaf35918db17a04379ea3f9/chrome/browser/extensions/data_deleter.cc
[modify] https://crrev.com/a68c2a85cd2e3a033eaf35918db17a04379ea3f9/chrome/browser/extensions/data_deleter.h
[modify] https://crrev.com/a68c2a85cd2e3a033eaf35918db17a04379ea3f9/chrome/browser/extensions/extension_service.cc
[modify] https://crrev.com/a68c2a85cd2e3a033eaf35918db17a04379ea3f9/content/browser/browsing_data/browsing_data_remover_impl_unittest.cc
[modify] https://crrev.com/a68c2a85cd2e3a033eaf35918db17a04379ea3f9/content/browser/devtools/protocol/storage_handler.cc
[modify] https://crrev.com/a68c2a85cd2e3a033eaf35918db17a04379ea3f9/content/browser/storage_partition_impl.cc
[modify] https://crrev.com/a68c2a85cd2e3a033eaf35918db17a04379ea3f9/content/browser/storage_partition_impl.h
[modify] https://crrev.com/a68c2a85cd2e3a033eaf35918db17a04379ea3f9/content/public/browser/storage_partition.h
[modify] https://crrev.com/a68c2a85cd2e3a033eaf35918db17a04379ea3f9/content/public/test/test_storage_partition.cc
[modify] https://crrev.com/a68c2a85cd2e3a033eaf35918db17a04379ea3f9/content/public/test/test_storage_partition.h

Status: Fixed (was: Started)

Sign in to add a comment