New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

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

Blocking:
issue 556939



Sign in to add a comment
link

Issue 914401: Extensions: Get rid of linked_ptr usage in ManifestHandler et al

Reported by rdevlin....@chromium.org, Dec 12 Project Member

Issue description

linked_ptr is deprecated; we should remove its usages from manifest handler code.
 

Comment 1 by bugdroid1@chromium.org, Dec 14

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

commit cd24bd88f1eed423ee5ccf01e59da89fa38b781c
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Fri Dec 14 03:36:08 2018

[Extensions] Don't allow overwriting ManifestHandlers

Currently, ManifestHandler::Register() and
ManifestHandlerRegistry::RegisterManifestHandler() allow registering a
new ManifestHandler with a key that has already been associated with
another ManifestHandler, overwriting the current entry.

Remove this behavior, and make ManifestHandlerRegistry DCHECK if a
handler is registered with an already-associated key. This is for a few
reasons:
- We never actually overwrite keys currently
- If we did overwrite a key, it would likely be a bug
- It's conceptually simpler
- It will make removing linked_ptrs from ManifestHandlerRegistry
  simpler.

Bug:  914401 

Change-Id: Ie15e9598cc65b8ce3c6b95a666addb41e54d8e0d
Reviewed-on: https://chromium-review.googlesource.com/c/1374115
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616576}
[modify] https://crrev.com/cd24bd88f1eed423ee5ccf01e59da89fa38b781c/extensions/common/manifest_handler.cc
[modify] https://crrev.com/cd24bd88f1eed423ee5ccf01e59da89fa38b781c/extensions/common/manifest_handler.h
[modify] https://crrev.com/cd24bd88f1eed423ee5ccf01e59da89fa38b781c/extensions/common/manifest_handler_unittest.cc

Comment 2 by bugdroid1@chromium.org, Dec 14

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2033d608299d63987c046f7d048793ee8d50855e

commit 2033d608299d63987c046f7d048793ee8d50855e
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Fri Dec 14 23:54:49 2018

[Extensions] Remove linked_ptr from ManifestHandler[Registry]

Remove linked_ptrs from ManifestHandler and ManifestHandlerRegistry,
and tweak the ManifestHandlerRegistry interface to be more friendly
about passing ownership.
- Make ManifestHandlerRegistry::RegisterHandler() public, and explicitly
  take ownership of the ManifestHandler.
- Register ManifestHandlers with
  ManifestHandlerRegistry::RegisterHandler() directly, rather than going
  through ManifestHandler::Register().
(The combination of these two changes makes for a less surprising
interface, where we do registry->RegisterHandler(make_unique<Handler>())
rather than (new Handler())->Register())
- Store ManifestHandlers in an unsorted vector in
  ManifestHandlerRegistry, which serves only to maintain ownership.
  Update the maps to only retain raw pointer references.

TBR=halliwell@chromium.org (mechanical change to cast_extensions_api_provider.cc)
Bug:  914401 

Change-Id: Ib53c44095e8b91ef768422ab9ecd8d71726da4a5
Reviewed-on: https://chromium-review.googlesource.com/c/1374389
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616880}
[modify] https://crrev.com/2033d608299d63987c046f7d048793ee8d50855e/chrome/common/extensions/chrome_manifest_handlers.cc
[modify] https://crrev.com/2033d608299d63987c046f7d048793ee8d50855e/chromecast/common/cast_extensions_api_provider.cc
[modify] https://crrev.com/2033d608299d63987c046f7d048793ee8d50855e/extensions/browser/content_verifier_unittest.cc
[modify] https://crrev.com/2033d608299d63987c046f7d048793ee8d50855e/extensions/common/common_manifest_handlers.cc
[modify] https://crrev.com/2033d608299d63987c046f7d048793ee8d50855e/extensions/common/manifest_handler.cc
[modify] https://crrev.com/2033d608299d63987c046f7d048793ee8d50855e/extensions/common/manifest_handler.h
[modify] https://crrev.com/2033d608299d63987c046f7d048793ee8d50855e/extensions/common/manifest_handler_unittest.cc
[modify] https://crrev.com/2033d608299d63987c046f7d048793ee8d50855e/extensions/common/scoped_testing_manifest_handler_registry.h

Comment 3 by rdevlin....@chromium.org, Dec 21

Status: Fixed (was: Started)

Sign in to add a comment