New issue
Advanced search Search tips

Issue 908536 link

Starred by 1 user
Status: Started
Owner:
rdevlin....@chromium.org
Cc:
karandeepb@chromium.org
lazyboy@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

Extensions: Permissions-related objects should be std::movable

Project Member Reported by rdevlin....@chromium.org, Nov 26 
Currently, we copy a bunch of our permissions-related objects (including APIPermissionSets, ManifestPermissionSets, URLPatternSets, etc).  These should instead be std::move()-able.  While we're at it, we should also make PermissionSet std::move-able, to potentially cut down on some of the unique_ptr wrapping.

Depending on common usage, we may also be able to restrict copying to deliberate Clone() methods to encourage std::move()ing.
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 5 

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

commit 32708b07f9d8acc1c5d5add3e3d86650cab25674
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Wed Dec 05 17:58:04 2018

[Extensions] Make BaseSetOperators<T> movable

Make BaseSetOperators<T> (the base class for APIPermissionSet and
ManifestPermissionSet) std::move-able. Also make copying the set require
an explicit method call (Clone()) to ensure that it is only used when a
copy is really required, encouraging callers to use std::move when
possible. Finally, have the PermissionSet constructor accept
APIPermissionSet and ManifestPermissionSet by value instead of by
const& to cut down on copies.

By a simple test, this cuts down on BaseSetOperators<T> copies by around
2x in the browser process on startup.

Bug: 908536

Change-Id: I367dfbd4b4daee4e63475bf781e4eafb52b14d19
Reviewed-on: https://chromium-review.googlesource.com/c/1355844
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614022}
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/browser/chromeos/extensions/public_session_permission_helper.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/browser/extensions/active_tab_permission_granter.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/browser/extensions/api/developer_private/extension_info_generator.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/browser/extensions/api/permissions/permissions_api.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/browser/extensions/api/permissions/permissions_api_helpers_unittest.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/browser/extensions/api/permissions/permissions_apitest.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/browser/extensions/extension_install_prompt_unittest.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/browser/extensions/extension_management.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/browser/extensions/extension_management_internal.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/browser/extensions/extension_management_unittest.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/browser/extensions/extension_prefs_unittest.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/browser/extensions/permissions_updater.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/browser/extensions/permissions_updater_unittest.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/browser/extensions/scripting_permissions_modifier.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/common/extensions/permissions/permission_set_unittest.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/chrome/common/extensions/permissions/permissions_data_unittest.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/extensions/browser/extension_prefs.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/extensions/common/extension_messages.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/extensions/common/extension_messages.h
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/extensions/common/extension_messages_unittest.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/extensions/common/manifest_handlers/permissions_parser.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/extensions/common/permissions/base_set_operators.h
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/extensions/common/permissions/base_set_operators_unittest.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/extensions/common/permissions/permission_set.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/extensions/common/permissions/permission_set.h
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/extensions/renderer/dispatcher.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/extensions/renderer/native_extension_bindings_system_unittest.cc
[modify] https://crrev.com/32708b07f9d8acc1c5d5add3e3d86650cab25674/extensions/renderer/script_context.h
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 14 

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

commit 11860c98a32865e91dce5cbce1d76611961202d7
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Fri Dec 14 20:18:27 2018

[Extensions] Enforce URLPatternSet is moved or deliberately copied

Remove the ability for URLPatternSet to be implicitly copied, and
instead require it to be either std::move()'d or Clone()'d. This
helps avoid unintentional or unnecessary copies, which can be
costly.

Followups will adjust more sites to accept URLPatternSet as a value
and encourage callers to std::move() the set in.

Bug: 908536

Change-Id: I3c6a51037202b213aaf6cea8762746f9eee5adab
Reviewed-on: https://chromium-review.googlesource.com/c/1362494
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616804}
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.cc
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/chrome/browser/extensions/active_tab_permission_granter.cc
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/chrome/browser/extensions/extension_management_unittest.cc
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/chrome/browser/extensions/menu_manager.h
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/chrome/browser/extensions/permissions_updater.cc
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/chrome/common/extensions/api/url_handlers/url_handlers_parser.cc
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/chrome/common/extensions/api/url_handlers/url_handlers_parser.h
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/chrome/common/extensions/permissions/permissions_data_unittest.cc
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/extensions/browser/api/declarative_net_request/declarative_net_request_api.cc
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/extensions/browser/api/web_request/web_request_api.cc
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/extensions/browser/api/web_request/web_request_api.h
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/extensions/browser/renderer_startup_helper.cc
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/extensions/common/extension_messages.cc
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/extensions/common/manifest_handlers/automation.cc
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/extensions/common/manifest_handlers/permissions_parser.cc
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/extensions/common/permissions/permission_set.cc
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/extensions/common/permissions/permission_set.h
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/extensions/common/permissions/permissions_data.cc
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/extensions/common/url_pattern_set.cc
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/extensions/common/url_pattern_set.h
[modify] https://crrev.com/11860c98a32865e91dce5cbce1d76611961202d7/extensions/common/user_script.cc
►

Sign in to add a comment