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

Issue 582001 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 21
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 581625
issue 556939



Sign in to add a comment

//extensions/browser/declarative uses linked_ptr like shared_ptr

Project Member Reported by dcheng@chromium.org, Jan 28 2016

Issue description

It's really hard to figure out how to rewrite this API using scoped_ptr instead of linked_ptr. There are times when it's not clear how ownership is supposed to move between different members. Who owns this API and understands how it works?
 

Comment 1 by dcheng@chromium.org, Jan 28 2016

Blocking: chromium:581625
Cc: jyasskin@chromium.org
Jeffrey is the lucky OWNER of declarativeContent, and I think this falls into that category.
Cc: battre@chromium.org
For things like https://code.google.com/p/chromium/codesearch/#chromium/src/extensions/browser/api/declarative/rules_registry.cc&l=240, you have to decide between copying the Rule object or handing out a pointer to the data owned by the RulesRegistry. You may want to start by making bindings-built dictionaries move-only (http://chromium-cpp.appspot.com/#core-whitelist) and giving them a Copy() function.
Project Member

Comment 4 by sheriffbot@chromium.org, Jan 30 2017

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been available for more than 365 days, and should be re-evaluated. Please re-triage this issue.
The Hotlist-Recharge-Cold label is applied for tracking purposes, and should not be removed after re-triaging the issue.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 5 by a...@chromium.org, Jul 29 2017

Blocking: 556939
Labels: -Hotlist-Recharge-Cold
Status: Available (was: Untriaged)
Project Member

Comment 7 by sheriffbot@chromium.org, Aug 6

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
FYI, extensionistas:

//extensions/browser/declarative is now basically the last large remaining user of linked_ptr. Can you please take a look?
Status: Available (was: Untriaged)
I'll see if I can take a look this week, but resources are tight, and IIRC, this one was a bit messy.

One easy fix, ugly as it is, is to just make these use RefCounted (because that's essentially what they're doing).  It doesn't make the code cleaner (they ideally wouldn't be), but it's not really much worse than what we have, and would let us get rid of linked_ptr.  Avi, does that work for you?
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 7

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

commit 38d4c8616dd560586a6e29165b77eadd5a2ca1ed
Author: Avi Drissman <avi@chromium.org>
Date: Fri Dec 07 07:21:09 2018

Remove linked_ptr from the extensions trackers.

BUG= 582001 

Change-Id: I4141f3fe268d34e5d078740b5ef12b4f02b5efb1
Reviewed-on: https://chromium-review.googlesource.com/c/1364550
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614633}
[modify] https://crrev.com/38d4c8616dd560586a6e29165b77eadd5a2ca1ed/chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.cc
[modify] https://crrev.com/38d4c8616dd560586a6e29165b77eadd5a2ca1ed/chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.h
[modify] https://crrev.com/38d4c8616dd560586a6e29165b77eadd5a2ca1ed/chrome/browser/extensions/api/declarative_content/declarative_content_is_bookmarked_condition_tracker.cc
[modify] https://crrev.com/38d4c8616dd560586a6e29165b77eadd5a2ca1ed/chrome/browser/extensions/api/declarative_content/declarative_content_is_bookmarked_condition_tracker.h
[modify] https://crrev.com/38d4c8616dd560586a6e29165b77eadd5a2ca1ed/chrome/browser/extensions/api/declarative_content/declarative_content_page_url_condition_tracker.cc
[modify] https://crrev.com/38d4c8616dd560586a6e29165b77eadd5a2ca1ed/chrome/browser/extensions/api/declarative_content/declarative_content_page_url_condition_tracker.h

Project Member

Comment 11 by bugdroid1@chromium.org, Dec 11

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

commit 8af53cdb32ffaf09f3372f3cf4d271bec9336647
Author: Avi Drissman <avi@chromium.org>
Date: Tue Dec 11 22:43:18 2018

Remove linked_ptr from the extensions rules registry.

BUG= 582001 

Change-Id: Ia2abb1393718c8c0aa54a350ccc065077e362870
Reviewed-on: https://chromium-review.googlesource.com/c/1363797
Commit-Queue: Avi Drissman <avi@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615702}
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/chrome/browser/extensions/api/declarative/declarative_apitest.cc
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/chrome/browser/extensions/api/declarative/rules_registry_service_unittest.cc
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/chrome/browser/extensions/api/declarative/rules_registry_with_cache_unittest.cc
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.cc
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.h
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry_unittest.cc
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/chrome/test/data/extensions/api_test/declarative/api/background.js
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/extensions/browser/api/declarative/declarative_api.cc
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/extensions/browser/api/declarative/declarative_rule.h
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/extensions/browser/api/declarative/declarative_rule_unittest.cc
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/extensions/browser/api/declarative/rules_registry.cc
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/extensions/browser/api/declarative/rules_registry.h
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/extensions/browser/api/declarative/rules_registry_unittest.cc
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/extensions/browser/api/declarative/test_rules_registry.cc
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/extensions/browser/api/declarative/test_rules_registry.h
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/extensions/browser/api/declarative_webrequest/webrequest_rules_registry.cc
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/extensions/browser/api/declarative_webrequest/webrequest_rules_registry.h
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/extensions/common/api/declarative/declarative_manifest_data.cc
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/extensions/common/api/declarative/declarative_manifest_data.h
[modify] https://crrev.com/8af53cdb32ffaf09f3372f3cf4d271bec9336647/extensions/common/api/declarative/declarative_manifest_unittest.cc

Owner: a...@chromium.org
Status: Started (was: Available)
Project Member

Comment 13 by bugdroid1@chromium.org, Dec 19

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

commit 07b343809e9387aa41ced7fe119aaba511bbe3da
Author: Avi Drissman <avi@chromium.org>
Date: Wed Dec 19 18:30:49 2018

Make ParsedRequestCookie comprise strings.

For safety's sake, make a string copy rather than try to be
very sure about whether the string backing a StringPiece
will outlive the StringPiece.

BUG= 582001 

Change-Id: I92b1d7e541173f0622c3791cf0e1b751d1018b06
Reviewed-on: https://chromium-review.googlesource.com/c/1383216
Commit-Queue: Avi Drissman <avi@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617888}
[modify] https://crrev.com/07b343809e9387aa41ced7fe119aaba511bbe3da/extensions/browser/api/web_request/web_request_api_helpers.cc
[modify] https://crrev.com/07b343809e9387aa41ced7fe119aaba511bbe3da/net/cookies/cookie_util.cc
[modify] https://crrev.com/07b343809e9387aa41ced7fe119aaba511bbe3da/net/cookies/cookie_util.h
[modify] https://crrev.com/07b343809e9387aa41ced7fe119aaba511bbe3da/net/cookies/cookie_util_unittest.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Dec 20

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

commit 6a20ff51239b390bfbd23b7342119529392d0a18
Author: Avi Drissman <avi@chromium.org>
Date: Thu Dec 20 23:04:56 2018

Update declarative web request classes.

This switches them to use Optional, and makes them
clonable in preparation for linked_ptr removal.

BUG= 582001 

Change-Id: I1ba6bb559b6fec703f41ad22ad670799ec61c91d
Reviewed-on: https://chromium-review.googlesource.com/c/1380819
Commit-Queue: Avi Drissman <avi@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618368}
[modify] https://crrev.com/6a20ff51239b390bfbd23b7342119529392d0a18/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
[modify] https://crrev.com/6a20ff51239b390bfbd23b7342119529392d0a18/extensions/browser/api/declarative_webrequest/webrequest_action.cc
[modify] https://crrev.com/6a20ff51239b390bfbd23b7342119529392d0a18/extensions/browser/api/declarative_webrequest/webrequest_action.h
[modify] https://crrev.com/6a20ff51239b390bfbd23b7342119529392d0a18/extensions/browser/api/web_request/web_request_api.cc
[modify] https://crrev.com/6a20ff51239b390bfbd23b7342119529392d0a18/extensions/browser/api/web_request/web_request_api.h
[modify] https://crrev.com/6a20ff51239b390bfbd23b7342119529392d0a18/extensions/browser/api/web_request/web_request_api_helpers.cc
[modify] https://crrev.com/6a20ff51239b390bfbd23b7342119529392d0a18/extensions/browser/api/web_request/web_request_api_helpers.h

Project Member

Comment 15 by bugdroid1@chromium.org, Dec 21

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

commit 2d47b917314a9008902c7c8f3f9e6a7410ef6a1b
Author: Avi Drissman <avi@chromium.org>
Date: Fri Dec 21 21:00:22 2018

Remove linked_ptr from extensions declarative web requests.

BUG= 582001 

Change-Id: I15b7783fa2a409e25ed9764a0b274e403db5808b
Reviewed-on: https://chromium-review.googlesource.com/c/1368688
Commit-Queue: Avi Drissman <avi@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618596}
[modify] https://crrev.com/2d47b917314a9008902c7c8f3f9e6a7410ef6a1b/chrome/browser/extensions/api/declarative_webrequest/webrequest_action_unittest.cc
[modify] https://crrev.com/2d47b917314a9008902c7c8f3f9e6a7410ef6a1b/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc
[modify] https://crrev.com/2d47b917314a9008902c7c8f3f9e6a7410ef6a1b/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
[modify] https://crrev.com/2d47b917314a9008902c7c8f3f9e6a7410ef6a1b/extensions/browser/api/declarative/declarative_rule.h
[modify] https://crrev.com/2d47b917314a9008902c7c8f3f9e6a7410ef6a1b/extensions/browser/api/declarative_webrequest/webrequest_action.cc
[modify] https://crrev.com/2d47b917314a9008902c7c8f3f9e6a7410ef6a1b/extensions/browser/api/declarative_webrequest/webrequest_action.h
[modify] https://crrev.com/2d47b917314a9008902c7c8f3f9e6a7410ef6a1b/extensions/browser/api/declarative_webrequest/webrequest_rules_registry.cc
[modify] https://crrev.com/2d47b917314a9008902c7c8f3f9e6a7410ef6a1b/extensions/browser/api/declarative_webrequest/webrequest_rules_registry.h
[modify] https://crrev.com/2d47b917314a9008902c7c8f3f9e6a7410ef6a1b/extensions/browser/api/web_request/web_request_api.cc
[modify] https://crrev.com/2d47b917314a9008902c7c8f3f9e6a7410ef6a1b/extensions/browser/api/web_request/web_request_api_helpers.cc
[modify] https://crrev.com/2d47b917314a9008902c7c8f3f9e6a7410ef6a1b/extensions/browser/api/web_request/web_request_api_helpers.h

Status: Fixed (was: Started)

Sign in to add a comment