Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
71153005:
Pull implementation of CertStore out to be reused. (Closed)
|
Created:
7 years, 1 month ago by alcutter Modified:
7 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, Eran M. (Google) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionPull implementation of CertStore out so that it can be re-used for an SCTStore by upcoming changes for Certificate Transparency.
(See https://codereview.chromium.org/71633002/ for the overview of what builds on this CL.)
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236026
Patch Set 1 #Patch Set 2 : Correct gypi file. #
Total comments: 4
Patch Set 3 : clang-format item-store.h #
Total comments: 16
Patch Set 4 : fixes. #Patch Set 5 : Rename & change to composition. #Patch Set 6 : update gypi too #
Total comments: 36
Patch Set 7 : Fixes for wtc. #
Total comments: 2
Patch Set 8 : Fixes for avi. #
Total comments: 6
Messages
Total messages: 19 (0 generated)
Hi Wan-Teh, this is the first of a series of CLs with more Certificate Transparency support. The bigger picture of where this series of CLs is going is over in https://codereview.chromium.org/71633002/ From there it'll all feed back in to Eran's work. Cheers, Al.
Hi Adam, here's the first in a series of patches for CT in chrome. This series will be a piecewise drip of the monster change in https://codereview.chromium.org/71633002/ .
https://codereview.chromium.org/71153005/diff/40002/content/browser/item_store.h File content/browser/item_store.h (right): https://codereview.chromium.org/71153005/diff/40002/content/browser/item_stor... content/browser/item_store.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013 https://codereview.chromium.org/71153005/diff/40002/content/browser/item_stor... content/browser/item_store.h:35: ItemStore() : next_item_id_(1) { this should be indented two spaces and then the body fixed up likewise. https://codereview.chromium.org/71153005/diff/150001/content/browser/item_sto... File content/browser/item_store.h (right): https://codereview.chromium.org/71153005/diff/150001/content/browser/item_sto... content/browser/item_store.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013 https://codereview.chromium.org/71153005/diff/150001/content/browser/item_sto... content/browser/item_store.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Note, this file name is very generic. See comments below. https://codereview.chromium.org/71153005/diff/150001/content/browser/item_sto... content/browser/item_store.h:18: template <typename T> This pollutes the global name space here. I think it can be made a private, sub-class of ItemStore. https://codereview.chromium.org/71153005/diff/150001/content/browser/item_sto... content/browser/item_store.h:32: class ItemStore : public NotificationObserver { ItemStore is an incredibly generic name. RendererDataMemoizingStore? https://codereview.chromium.org/71153005/diff/150001/content/browser/item_sto... content/browser/item_store.h:32: class ItemStore : public NotificationObserver { This needs a fairly large comment explaining it: // RendererDataMemoizingStore is a thread-safe container that retains reference counted objects that are associated with one or more render processes. Objects are identified by an int and only a single instance of a given object is retained. RendererDataMemoizingStore watches for render process termination and releases objects that are no longer associated with any render process. https://codereview.chromium.org/71153005/diff/150001/content/browser/item_sto... content/browser/item_store.h:33: protected: You are inheriting from this object. Are you sure that you don't want to include an instance of it instead? (That would need the members to be sorted into public and private.) https://codereview.chromium.org/71153005/diff/150001/content/browser/item_sto... content/browser/item_store.h:62: int Store(T* item, int process_id) { // Store adds |item| to this collection, associates it with the given render process id and returns an opaque identifier for it. If |item| is already known, the same identifier will be returned. https://codereview.chromium.org/71153005/diff/150001/content/browser/item_sto... content/browser/item_store.h:104: bool Retrieve(int item_id, scoped_refptr<T>* item) { // Retrieve ...
https://codereview.chromium.org/71153005/diff/40002/content/browser/item_store.h File content/browser/item_store.h (right): https://codereview.chromium.org/71153005/diff/40002/content/browser/item_stor... content/browser/item_store.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/11/14 16:20:37, agl wrote: > 2013 Done. https://codereview.chromium.org/71153005/diff/40002/content/browser/item_stor... content/browser/item_store.h:35: ItemStore() : next_item_id_(1) { On 2013/11/14 16:20:37, agl wrote: > this should be indented two spaces and then the body fixed up likewise. I think this should've already been done with the follow up clang-format patch. https://codereview.chromium.org/71153005/diff/150001/content/browser/item_sto... File content/browser/item_store.h (right): https://codereview.chromium.org/71153005/diff/150001/content/browser/item_sto... content/browser/item_store.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/11/14 16:20:37, agl wrote: > Note, this file name is very generic. See comments below. Ack. https://codereview.chromium.org/71153005/diff/150001/content/browser/item_sto... content/browser/item_store.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/11/14 16:20:37, agl wrote: > 2013 Done. https://codereview.chromium.org/71153005/diff/150001/content/browser/item_sto... content/browser/item_store.h:18: template <typename T> On 2013/11/14 16:20:37, agl wrote: > This pollutes the global name space here. I think it can be made a private, > sub-class of ItemStore. Done. https://codereview.chromium.org/71153005/diff/150001/content/browser/item_sto... content/browser/item_store.h:32: class ItemStore : public NotificationObserver { On 2013/11/14 16:20:37, agl wrote: > ItemStore is an incredibly generic name. > > RendererDataMemoizingStore? Ok, I'll rename once you're happy with everything else to avoid causing too much churn mid-review. https://codereview.chromium.org/71153005/diff/150001/content/browser/item_sto... content/browser/item_store.h:32: class ItemStore : public NotificationObserver { On 2013/11/14 16:20:37, agl wrote: > This needs a fairly large comment explaining it: > > // RendererDataMemoizingStore is a thread-safe container that retains reference > counted objects that are associated with one or more render processes. Objects > are identified by an int and only a single instance of a given object is > retained. RendererDataMemoizingStore watches for render process termination and > releases objects that are no longer associated with any render process. Fair point - this file is really just the old contents of cert_store.cc so I didn't mess with it more than I needed to. Comment added. https://codereview.chromium.org/71153005/diff/150001/content/browser/item_sto... content/browser/item_store.h:33: protected: On 2013/11/14 16:20:37, agl wrote: > You are inheriting from this object. Are you sure that you don't want to include > an instance of it instead? (That would need the members to be sorted into public > and private.) I don't really mind either way, I'm happy to change to composition if you prefer ? https://codereview.chromium.org/71153005/diff/150001/content/browser/item_sto... content/browser/item_store.h:62: int Store(T* item, int process_id) { On 2013/11/14 16:20:37, agl wrote: > // Store adds |item| to this collection, associates it with the given render > process id and returns an opaque identifier for it. If |item| is already known, > the same identifier will be returned. Done. https://codereview.chromium.org/71153005/diff/150001/content/browser/item_sto... content/browser/item_store.h:104: bool Retrieve(int item_id, scoped_refptr<T>* item) { On 2013/11/14 16:20:37, agl wrote: > // Retrieve ... Done.
Please do the rename and switch to composition then ping this thread.
On 2013/11/14 17:04:14, agl wrote: > Please do the rename and switch to composition then ping this thread. Done.
Patch set 6 LGTM. High-level comments: 1. This seems like a good way to share code. 2. I think renderer_data_memoizing_store.h should have a .cc file where the methods are defined. 3. There are some violations of Style Guide recommendations. https://codereview.chromium.org/71153005/diff/290001/content/browser/cert_sto... File content/browser/cert_store_impl.cc (right): https://codereview.chromium.org/71153005/diff/290001/content/browser/cert_sto... content/browser/cert_store_impl.cc:16: #include "content/public/browser/notification_types.h" Please trim the header list. Some of them are not necessary (for example, the notification-related headers and base/bind.h). https://codereview.chromium.org/71153005/diff/290001/content/browser/cert_sto... content/browser/cert_store_impl.cc:21: CertStore* CertStore::GetInstance() { return CertStoreImpl::GetInstance(); } Nit: in general it's not necessary to reformat code like this, unless it is suggested by some automated tool. https://codereview.chromium.org/71153005/diff/290001/content/browser/cert_sto... File content/browser/cert_store_impl.h (right): https://codereview.chromium.org/71153005/diff/290001/content/browser/cert_sto... content/browser/cert_store_impl.h:16: #include "net/cert/x509_certificate.h" Please trim the header list. For example, the notification-related headers and base/synchronization/lock.h should be unnecessary now. https://codereview.chromium.org/71153005/diff/290001/content/browser/cert_sto... content/browser/cert_store_impl.h:36: RendererDataMemoizingStore<net::X509Certificate> store_; This should be reordered to following the Style Guide: private: friend struct DefaultSingletonTraits<CertStoreImpl>; RendererDataMemoizingStore<net::X509Certificate> store_; DISALLOW_COPY_AND_ASSIGN(CertStoreImpl); See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order Note that the Style Guide does not say exactly where friend declarations should be relative to the others. I usually see friend declarations in the typedefs section, at the top. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... File content/browser/renderer_data_memoizing_store.h (right): https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:26: // are identified by an int and only a single instance of a given object is Nit: a single instance => a single reference? https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:29: // releases objects that are no longer associated with any render process. The comment block should be reformatted. For example, lines 25 and 28 have only one word. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:31: class RendererDataMemoizingStore : public NotificationObserver { Is it better to define the class inline in the header file? In general it's better to have a .cc file with the method definitions. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:32: private: The three "private" sections should be merged into one, and be listed after the "public" section. This is recommended by the Style Guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:41: M value; Nit: add a blank line before this line. (I know you copied this from the original code.) https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:67: typename ReverseItemMap::iterator item_iter = item_to_id_.find(item); Is it necessary to use "typename" here? (There are other occurrences of "typename" in this file. I won't repeat this question.) https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:85: process_ids.second, Nit: I would list the first two arguments on the same line as the original code does, unless you have strong preference for this style. (Similarly for lines 93-94 and 152-153.) https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:132: void RemoveInternal(int item_id) { Please copy the original comment (after updating it to remove references to "cert" and to use the new member names): // Remove the specified cert from id_to_cert_ and cert_to_id_. // NOTE: the caller (RemoveCertsForRenderProcesHost) must hold cert_lock_. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:145: void RemoveForRenderProcessHost(int process_id) { Please copy the original comment (after updating it to remove references to "certs"): // Removes all the certs associated with the specified process from the store. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:185: void Observe(int type, Add the comment // NotificationObserver implementation. before this method. This is the convention used in the Chromium source tree. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:187: const NotificationDetails& details) { Add "OVERRIDE" between ')' and '{'. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:198: typedef std::map<T*, int, typename T::LessThan> ReverseItemMap; I am not familiar with templates. Is it necessary to use 'typename' here? https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:210: // This lock protects: process_to_item_id_, item_id_to_process_id_, Typo: process_to_item_id_ => process_id_to_item_id_
https://codereview.chromium.org/71153005/diff/290001/content/browser/cert_sto... File content/browser/cert_store_impl.cc (right): https://codereview.chromium.org/71153005/diff/290001/content/browser/cert_sto... content/browser/cert_store_impl.cc:21: CertStore* CertStore::GetInstance() { return CertStoreImpl::GetInstance(); } On 2013/11/14 22:04:58, wtc wrote: > > Nit: in general it's not necessary to reformat code like this, unless it is > suggested by some automated tool. (p.s. this is my fault. One of the files had numerous style issues and I asked for it to be run through clang-format. It looks like this one was also processed.) https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... File content/browser/renderer_data_memoizing_store.h (right): https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:31: class RendererDataMemoizingStore : public NotificationObserver { On 2013/11/14 22:04:58, wtc wrote: > > Is it better to define the class inline in the header file? In general it's > better to have a .cc file with the method definitions. I think, because it's a template, it must be in the header.
https://codereview.chromium.org/71153005/diff/290001/content/browser/cert_sto... File content/browser/cert_store_impl.cc (right): https://codereview.chromium.org/71153005/diff/290001/content/browser/cert_sto... content/browser/cert_store_impl.cc:16: #include "content/public/browser/notification_types.h" On 2013/11/14 22:04:58, wtc wrote: > > Please trim the header list. Some of them are not necessary (for example, the > notification-related headers and base/bind.h). Done. https://codereview.chromium.org/71153005/diff/290001/content/browser/cert_sto... content/browser/cert_store_impl.cc:21: CertStore* CertStore::GetInstance() { return CertStoreImpl::GetInstance(); } On 2013/11/14 22:04:58, wtc wrote: > > Nit: in general it's not necessary to reformat code like this, unless it is > suggested by some automated tool. Undone! https://codereview.chromium.org/71153005/diff/290001/content/browser/cert_sto... File content/browser/cert_store_impl.h (right): https://codereview.chromium.org/71153005/diff/290001/content/browser/cert_sto... content/browser/cert_store_impl.h:16: #include "net/cert/x509_certificate.h" On 2013/11/14 22:04:58, wtc wrote: > > Please trim the header list. For example, the notification-related headers and > base/synchronization/lock.h should be unnecessary now. Done. https://codereview.chromium.org/71153005/diff/290001/content/browser/cert_sto... content/browser/cert_store_impl.h:36: RendererDataMemoizingStore<net::X509Certificate> store_; On 2013/11/14 22:04:58, wtc wrote: > > This should be reordered to following the Style Guide: > > private: > friend struct DefaultSingletonTraits<CertStoreImpl>; > > RendererDataMemoizingStore<net::X509Certificate> store_; > > DISALLOW_COPY_AND_ASSIGN(CertStoreImpl); > > See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order > > Note that the Style Guide does not say exactly where friend declarations should > be relative to the others. I usually see friend declarations in the typedefs > section, at the top. Done. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... File content/browser/renderer_data_memoizing_store.h (right): https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:26: // are identified by an int and only a single instance of a given object is On 2013/11/14 22:04:58, wtc wrote: > > Nit: a single instance => a single reference? Done. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:29: // releases objects that are no longer associated with any render process. On 2013/11/14 22:04:58, wtc wrote: > > The comment block should be reformatted. For example, lines 25 and 28 have only > one word. Not sure what happened there, done. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:31: class RendererDataMemoizingStore : public NotificationObserver { On 2013/11/14 22:04:58, wtc wrote: > > Is it better to define the class inline in the header file? In general it's > better to have a .cc file with the method definitions. As Adams says, it has to be in the header because it's a template. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:32: private: On 2013/11/14 22:04:58, wtc wrote: > > The three "private" sections should be merged into one, and be listed after the > "public" section. This is recommended by the Style Guide: > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order > Done. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:41: M value; On 2013/11/14 22:04:58, wtc wrote: > > Nit: add a blank line before this line. (I know you copied this from the > original code.) Done. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:67: typename ReverseItemMap::iterator item_iter = item_to_id_.find(item); On 2013/11/14 22:04:58, wtc wrote: > > Is it necessary to use "typename" here? > > (There are other occurrences of "typename" in this file. I won't repeat this > question.) Yes, because ReverseItemMap (==std::map<T*, int, T::LessThan>) is dependent on T. Same answer for the other occurrences too. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:85: process_ids.second, On 2013/11/14 22:04:58, wtc wrote: > > Nit: I would list the first two arguments on the same line as the original code > does, unless you have strong preference for this style. (Similarly for lines > 93-94 and 152-153.) Done. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:132: void RemoveInternal(int item_id) { On 2013/11/14 22:04:58, wtc wrote: > > Please copy the original comment (after updating it to remove references to > "cert" and to use the new member names): > > // Remove the specified cert from id_to_cert_ and cert_to_id_. > // NOTE: the caller (RemoveCertsForRenderProcesHost) must hold cert_lock_. Whoops, done. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:145: void RemoveForRenderProcessHost(int process_id) { On 2013/11/14 22:04:58, wtc wrote: > > Please copy the original comment (after updating it to remove references to > "certs"): > > // Removes all the certs associated with the specified process from the store. Done. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:185: void Observe(int type, On 2013/11/14 22:04:58, wtc wrote: > > Add the comment > // NotificationObserver implementation. > before this method. > > This is the convention used in the Chromium source tree. Done. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:187: const NotificationDetails& details) { On 2013/11/14 22:04:58, wtc wrote: > > Add "OVERRIDE" between ')' and '{'. Done. https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:198: typedef std::map<T*, int, typename T::LessThan> ReverseItemMap; On 2013/11/14 22:04:58, wtc wrote: > > I am not familiar with templates. Is it necessary to use 'typename' here? Yep, same story here (it's because someone could create a class where "LessThan" was, e.g. a member variable or something unexpected, and pass that in to the template as T. Having this typedef shows the intent that it should be restricted to types only, and in fact it won't compile without it.) https://codereview.chromium.org/71153005/diff/290001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:210: // This lock protects: process_to_item_id_, item_id_to_process_id_, On 2013/11/14 22:04:58, wtc wrote: > > Typo: process_to_item_id_ => process_id_to_item_id_ Good catch! Done.
LGTM +avi for OWNERS review.
Avi: <poke/>
LGTM https://codereview.chromium.org/71153005/diff/390001/content/browser/renderer... File content/browser/renderer_data_memoizing_store.h (right): https://codereview.chromium.org/71153005/diff/390001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. No (c) in new files, just "Copyright 2013..."
https://codereview.chromium.org/71153005/diff/390001/content/browser/renderer... File content/browser/renderer_data_memoizing_store.h (right): https://codereview.chromium.org/71153005/diff/390001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/11/19 16:38:05, Avi wrote: > No (c) in new files, just "Copyright 2013..." Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alcutter@google.com/71153005/630001
Message was sent while issue was closed.
Change committed as 236026
Message was sent while issue was closed.
Patch set 8 LGTM. Please write a follow-up CL to address my new comments. Thanks. https://codereview.chromium.org/71153005/diff/630001/content/browser/cert_sto... File content/browser/cert_store_impl.cc (right): https://codereview.chromium.org/71153005/diff/630001/content/browser/cert_sto... content/browser/cert_store_impl.cc:19: CertStoreImpl::CertStoreImpl() : store_() {} Nit: the store_() initializer can be omitted because we're just invoking the default constructor of the member (i.e., we're not passing any arguments to the member constructor). https://codereview.chromium.org/71153005/diff/630001/content/browser/renderer... File content/browser/renderer_data_memoizing_store.h (right): https://codereview.chromium.org/71153005/diff/630001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:28: template <typename T> It's too bad that RendererDataMemoizingStore has to be defined as a template. I think this is caused by base::RefCountedThreadSafe being a template, so net::X509Certificate and net::ct::SignedCertificateTimestamp don't have a common base class, even though both are derived from base::RefCountedThreadSafe. https://codereview.chromium.org/71153005/diff/630001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:60: item->AddRef(); Can you explain what this AddRef() is for? I suspect it isn't necessary in the new code because the next line, id_to_item_[item_id] = item; will acquire a reference to |item|. id_to_item_[item_id] is a scoped_refptr<T>. https://codereview.chromium.org/71153005/diff/630001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:132: // Remove the item specified by |item_id| from id_to_item and item_to_id_. Typo: id_to_item is missing the trailing underscore. https://codereview.chromium.org/71153005/diff/630001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:143: item_iter->second->Release(); Similarly, I suspect this Release() call isn't necessary in the new code because the next line, id_to_item_.erase(item_iter); should release a reference to item_iter->second.
Message was sent while issue was closed.
https://codereview.chromium.org/71153005/diff/630001/content/browser/renderer... File content/browser/renderer_data_memoizing_store.h (right): https://codereview.chromium.org/71153005/diff/630001/content/browser/renderer... content/browser/renderer_data_memoizing_store.h:60: item->AddRef(); On 2013/11/19 19:11:08, wtc wrote: > > Can you explain what this AddRef() is for? Al, I researched this a bit. I found that the AddRef() call comes from the original code, and based on this code review (https://codereview.chromium.org/5024), it's already this way five years ago. I suspect that originally id_to_item_[item_id] was not a scoped_refptr, so we had to call AddRef() and Release() manually. So you can feel free to ignore this issue. I can write a CL to remove the AddRef() and Release() calls.
Message was sent while issue was closed.
On 2013/11/19 19:19:46, wtc wrote: > https://codereview.chromium.org/71153005/diff/630001/content/browser/renderer... > File content/browser/renderer_data_memoizing_store.h (right): > > https://codereview.chromium.org/71153005/diff/630001/content/browser/renderer... > content/browser/renderer_data_memoizing_store.h:60: item->AddRef(); > > On 2013/11/19 19:11:08, wtc wrote: > > > > Can you explain what this AddRef() is for? > > Al, I researched this a bit. I found that the AddRef() call comes from the > original code, and based on this code review > (https://codereview.chromium.org/5024), it's already this way five years ago. > > I suspect that originally id_to_item_[item_id] was not a scoped_refptr, so we > had to call AddRef() and Release() manually. > > So you can feel free to ignore this issue. I can write a CL to remove the > AddRef() and Release() calls. I believe there's nothing for me to do here since you very kindly fixed these on my behalf last night in 76663002.
Message was sent while issue was closed.
On 2013/11/20 10:38:42, alcutter wrote: > > I believe there's nothing for me to do here since you very kindly fixed these on > my behalf last night in 76663002. Yes, I found the answers to my questions and made the changes I suggested. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
